forked from electron/electron
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Allow partitions to have app set quotas OS-14197 #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
gecko19
wants to merge
9
commits into
upstream_main
Choose a base branch
from
OS-14197
base: upstream_main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c048b3f
OS-14197: Allow partitions to have app set quotas
gecko19 656ae65
OS-14197:Fixup as per review comments
gecko19 40e500d
Merge remote-tracking branch 'origin' into OS-14197
gecko19 8e8c965
Merge branch 'OS-14197' into QuotaMergePath
gecko19 4858ae5
OS-14197:Fixup to work on rebased origin/main
gecko19 8a62dea
Merge branch 'OS-14197_fixedup' into OS-14197
gecko19 0be15e9
Fixup! remove old test case and modify after()
gecko19 8c4c165
Fixup! remove quota in older tests
gecko19 c031581
Fixup! Word the API usage better
gecko19 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
234 changes: 234 additions & 0 deletions
234
patches/chromium/feat_add_storage_quota_capability_for_partitions.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
| From: George Kottackal <kottackal.george@gmail.com> | ||
| Date: Sun, 2 Apr 2023 23:04:40 +0100 | ||
| Subject: feat: add storage quota capability for partitions | ||
|
|
||
| This patch adds the capability for a storage quota | ||
| to be set on partitions.This capability will be | ||
| exercised only when physical storage based | ||
| partitions are used. | ||
|
|
||
| diff --git a/content/browser/storage_partition_impl.cc b/content/browser/storage_partition_impl.cc | ||
| index e5185febf2cc3bc28353a0ec3e2c57aa536a3303..80989ba382778819b912b5bb9904a583c958215d 100644 | ||
| --- a/content/browser/storage_partition_impl.cc | ||
| +++ b/content/browser/storage_partition_impl.cc | ||
| @@ -1285,6 +1285,10 @@ void StoragePartitionImpl::Initialize( | ||
| base::BindRepeating(&StoragePartitionImpl::GetQuotaSettings, | ||
| weak_factory_.GetWeakPtr())); | ||
| quota_manager_ = quota_context_->quota_manager(); | ||
| + | ||
| + if (config_.quota()) | ||
| + quota_manager_->SetQuota(config_.quota().value()); | ||
| + | ||
| scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy = | ||
| quota_manager_->proxy(); | ||
|
|
||
| diff --git a/content/public/browser/storage_partition_config.cc b/content/public/browser/storage_partition_config.cc | ||
| index 81013d6eb993a9a1bfbdf0bea388e249c9045c05..55c4770b8933afbc7ae7e2c4dd17f73f193a0225 100644 | ||
| --- a/content/public/browser/storage_partition_config.cc | ||
| +++ b/content/public/browser/storage_partition_config.cc | ||
| @@ -21,8 +21,10 @@ StoragePartitionConfig& StoragePartitionConfig::operator=( | ||
|
|
||
| // static | ||
| StoragePartitionConfig StoragePartitionConfig::CreateDefault( | ||
| - BrowserContext* browser_context) { | ||
| - return StoragePartitionConfig("", "", browser_context->IsOffTheRecord()); | ||
| + BrowserContext* browser_context, | ||
| + absl::optional<int> quota_size) { | ||
| + return StoragePartitionConfig("", "", browser_context->IsOffTheRecord(), | ||
| + quota_size); | ||
| } | ||
|
|
||
| // static | ||
| @@ -30,22 +32,26 @@ StoragePartitionConfig StoragePartitionConfig::Create( | ||
| BrowserContext* browser_context, | ||
| const std::string& partition_domain, | ||
| const std::string& partition_name, | ||
| - bool in_memory) { | ||
| + bool in_memory, | ||
| + absl::optional<int> quota_size) { | ||
| // If a caller tries to pass an empty partition_domain something is seriously | ||
| // wrong or the calling code is not explicitly signalling its desire to create | ||
| // a default partition by calling CreateDefault(). | ||
| CHECK(!partition_domain.empty()); | ||
| return StoragePartitionConfig(partition_domain, partition_name, | ||
| - in_memory || browser_context->IsOffTheRecord()); | ||
| + in_memory || browser_context->IsOffTheRecord(), | ||
| + quota_size); | ||
| } | ||
|
|
||
| StoragePartitionConfig::StoragePartitionConfig( | ||
| const std::string& partition_domain, | ||
| const std::string& partition_name, | ||
| - bool in_memory) | ||
| + bool in_memory, | ||
| + absl::optional<int> quota_size) | ||
| : partition_domain_(partition_domain), | ||
| partition_name_(partition_name), | ||
| - in_memory_(in_memory) {} | ||
| + in_memory_(in_memory), | ||
| + quota_size_(quota_size){} | ||
|
|
||
| absl::optional<StoragePartitionConfig> | ||
| StoragePartitionConfig::GetFallbackForBlobUrls() const { | ||
| @@ -55,7 +61,8 @@ StoragePartitionConfig::GetFallbackForBlobUrls() const { | ||
| return StoragePartitionConfig( | ||
| partition_domain_, "", | ||
| /*in_memory=*/fallback_to_partition_domain_for_blob_urls_ == | ||
| - FallbackMode::kFallbackPartitionInMemory); | ||
| + FallbackMode::kFallbackPartitionInMemory, | ||
| + quota_size_); | ||
| } | ||
|
|
||
| bool StoragePartitionConfig::operator<( | ||
| diff --git a/content/public/browser/storage_partition_config.h b/content/public/browser/storage_partition_config.h | ||
| index e765f81673b08f51f6b7c3370c535fae03d41362..786d53df985637560737230d4c1182daf196d3bc 100644 | ||
| --- a/content/public/browser/storage_partition_config.h | ||
| +++ b/content/public/browser/storage_partition_config.h | ||
| @@ -28,7 +28,8 @@ class CONTENT_EXPORT StoragePartitionConfig { | ||
|
|
||
| // Creates a default config for |browser_context|. If |browser_context| is an | ||
| // off-the-record profile, then the config will have |in_memory_| set to true. | ||
| - static StoragePartitionConfig CreateDefault(BrowserContext* browser_context); | ||
| + static StoragePartitionConfig CreateDefault(BrowserContext* browser_context, | ||
| + absl::optional<int> quota_size = absl::nullopt); | ||
|
|
||
| // Creates a config tied to a specific domain. | ||
| // The |partition_domain| is [a-z]* UTF-8 string, specifying the domain in | ||
| @@ -43,11 +44,13 @@ class CONTENT_EXPORT StoragePartitionConfig { | ||
| static StoragePartitionConfig Create(BrowserContext* browser_context, | ||
| const std::string& partition_domain, | ||
| const std::string& partition_name, | ||
| - bool in_memory); | ||
| + bool in_memory, | ||
| + absl::optional<int> quota_size = absl::nullopt); | ||
|
|
||
| std::string partition_domain() const { return partition_domain_; } | ||
| std::string partition_name() const { return partition_name_; } | ||
| bool in_memory() const { return in_memory_; } | ||
| + absl::optional<int> quota() const { return quota_size_; } | ||
|
|
||
| // Returns true if this config was created by CreateDefault() or is | ||
| // a copy of a config created with that method. | ||
| @@ -94,11 +97,13 @@ class CONTENT_EXPORT StoragePartitionConfig { | ||
|
|
||
| StoragePartitionConfig(const std::string& partition_domain, | ||
| const std::string& partition_name, | ||
| - bool in_memory); | ||
| + bool in_memory, | ||
| + absl::optional<int> quota_size = absl::nullopt); | ||
|
|
||
| std::string partition_domain_; | ||
| std::string partition_name_; | ||
| bool in_memory_ = false; | ||
| + absl::optional<int> quota_size_ = absl::nullopt; | ||
| FallbackMode fallback_to_partition_domain_for_blob_urls_ = | ||
| FallbackMode::kNone; | ||
| }; | ||
| diff --git a/storage/browser/quota/quota_manager_impl.cc b/storage/browser/quota/quota_manager_impl.cc | ||
| index a7cf23f4f4c1876f7b01b6275b218c4622a225e3..1a939a07d8bb205b8063ed8b4fb166672682624a 100644 | ||
| --- a/storage/browser/quota/quota_manager_impl.cc | ||
| +++ b/storage/browser/quota/quota_manager_impl.cc | ||
| @@ -2619,7 +2619,7 @@ void QuotaManagerImpl::GetStorageCapacity(StorageCapacityCallback callback) { | ||
| db_runner_->PostTaskAndReplyWithResult( | ||
| FROM_HERE, | ||
| base::BindOnce(&QuotaManagerImpl::CallGetVolumeInfo, get_volume_info_fn_, | ||
| - profile_path_), | ||
| + profile_path_,start_quota_), | ||
| base::BindOnce(&QuotaManagerImpl::DidGetStorageCapacity, | ||
| weak_factory_.GetWeakPtr())); | ||
| } | ||
| @@ -3038,16 +3038,26 @@ void QuotaManagerImpl::PostTaskAndReplyWithResultForDBThread( | ||
| std::move(reply)); | ||
| } | ||
|
|
||
| + | ||
| +void QuotaManagerImpl::SetQuota(int start_quota) { | ||
| + // Set the quota for a browser context */ | ||
| + // If an electron::BrowserWindow uses a partition path | ||
| + // with no existing browser context, then | ||
| + // this quota takes effect | ||
| + start_quota_ = start_quota; | ||
| +} | ||
| + | ||
| // static | ||
| QuotaAvailability QuotaManagerImpl::CallGetVolumeInfo( | ||
| GetVolumeInfoFn get_volume_info_fn, | ||
| - const base::FilePath& path) { | ||
| + const base::FilePath& path, | ||
| + const absl::optional<int>& quota_size) { | ||
| if (!base::CreateDirectory(path)) { | ||
| LOG(WARNING) << "Create directory failed for path" << path.value(); | ||
| return QuotaAvailability(0, 0); | ||
| } | ||
|
|
||
| - const QuotaAvailability quotaAvailability = get_volume_info_fn(path); | ||
| + const QuotaAvailability quotaAvailability = get_volume_info_fn(path,quota_size); | ||
| const auto total = quotaAvailability.total; | ||
| const auto available = quotaAvailability.available; | ||
|
|
||
| @@ -3069,9 +3079,16 @@ QuotaAvailability QuotaManagerImpl::CallGetVolumeInfo( | ||
| } | ||
|
|
||
| // static | ||
| -QuotaAvailability QuotaManagerImpl::GetVolumeInfo(const base::FilePath& path) { | ||
| - return QuotaAvailability(base::SysInfo::AmountOfTotalDiskSpace(path), | ||
| - base::SysInfo::AmountOfFreeDiskSpace(path)); | ||
| +QuotaAvailability QuotaManagerImpl::GetVolumeInfo( | ||
| + const base::FilePath& path, | ||
| + const absl::optional<int>& quota_size) { | ||
| + if (!quota_size) { | ||
| + return QuotaAvailability(base::SysInfo::AmountOfTotalDiskSpace(path), | ||
| + base::SysInfo::AmountOfFreeDiskSpace(path)); | ||
| + } else { | ||
| + return QuotaAvailability(base::SysInfo::AmountOfTotalDiskSpace(path), | ||
| + quota_size.value()); | ||
| + } | ||
| } | ||
|
|
||
| void QuotaManagerImpl::AddObserver( | ||
| diff --git a/storage/browser/quota/quota_manager_impl.h b/storage/browser/quota/quota_manager_impl.h | ||
| index 9598b8674ecf386bfcd0423b51dd8ac3f0d7e379..3f5ef0bd50253ba07cc50ffc6a2378bce88e542b 100644 | ||
| --- a/storage/browser/quota/quota_manager_impl.h | ||
| +++ b/storage/browser/quota/quota_manager_impl.h | ||
| @@ -159,7 +159,8 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl | ||
| // Function pointer type used to store the function which returns | ||
| // information about the volume containing the given FilePath. | ||
| // The value returned is the QuotaAvailability struct. | ||
| - using GetVolumeInfoFn = QuotaAvailability (*)(const base::FilePath&); | ||
| + using GetVolumeInfoFn = QuotaAvailability (*)(const base::FilePath&, | ||
| + const absl::optional<int>&); | ||
|
|
||
| static constexpr int64_t kGBytes = 1024 * 1024 * 1024; | ||
| static constexpr int64_t kNoLimit = INT64_MAX; | ||
| @@ -468,6 +469,8 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl | ||
| eviction_disabled_ = disable; | ||
| } | ||
|
|
||
| + void SetQuota(const int start_quota); | ||
| + | ||
| // Testing support for handling corruption in the underlying database. | ||
| // | ||
| // Runs `corrupter` on the same sequence used to do database I/O, | ||
| @@ -743,8 +746,10 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl | ||
| bool is_bootstrap_task = false); | ||
|
|
||
| static QuotaAvailability CallGetVolumeInfo(GetVolumeInfoFn get_volume_info_fn, | ||
| - const base::FilePath& path); | ||
| - static QuotaAvailability GetVolumeInfo(const base::FilePath& path); | ||
| + const base::FilePath& path, | ||
| + const absl::optional<int>& quota_size = absl::nullopt); | ||
| + static QuotaAvailability GetVolumeInfo(const base::FilePath& path, | ||
| + const absl::optional<int>& quota_size = absl::nullopt); | ||
|
|
||
| const bool is_incognito_; | ||
| const base::FilePath profile_path_; | ||
| @@ -838,6 +843,8 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl | ||
| // QuotaManagerImpl::GetVolumeInfo. | ||
| GetVolumeInfoFn get_volume_info_fn_; | ||
|
|
||
| + absl::optional<int> start_quota_ = absl::nullopt; | ||
| + | ||
| std::unique_ptr<EvictionRoundInfoHelper> eviction_helper_; | ||
| std::map<BucketSetDataDeleter*, std::unique_ptr<BucketSetDataDeleter>> | ||
| bucket_set_data_deleters_; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
You can then avoid modifying StoragePartitionConfig.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a try with these changes. To have SetQuotaSettings() callable, the header file storage/browser/quota/quota_manager.h had to be added.
Since the SetQuotaSettings call can be done only on the IOThread, I tried the snippet below, but then there was a DCHECK fault since the IOthread is not accessible within the electron layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the exact DCHECK error? ElectronBrowserContext::~ElectronBrowserContext() has these lines:
BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, std::move(resource_context_));