-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Allow partitions to have app set quotas OS-14197 #3
base: upstream_main
Are you sure you want to change the base?
Changes from 2 commits
c048b3f
656ae65
40e500d
8e8c965
4858ae5
8a62dea
0be15e9
8c4c165
c031581
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,239 @@ | ||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: George Joseph <[email protected]> | ||
Date: Tue, 27 Dec 2022 18:58:16 +0000 | ||
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 bdc6a64ae010180102ebda39f1612604017fdd9f..c250301c3cc112a8ed1b5e7de7d03f55e56bcacd 100644 | ||
--- a/content/browser/storage_partition_impl.cc | ||
+++ b/content/browser/storage_partition_impl.cc | ||
@@ -1221,6 +1221,11 @@ void StoragePartitionImpl::Initialize( | ||
base::BindRepeating(&StoragePartitionImpl::GetQuotaSettings, | ||
weak_factory_.GetWeakPtr())); | ||
quota_manager_ = quota_context_->quota_manager(); | ||
+ | ||
+ // Set a startup quota if present. | ||
+ if (config_.quota()) | ||
+ quota_manager_->SetStartupQuota(config_.quota()); | ||
+ | ||
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..e71ca3cb97f383aab02c64a6d96c6cd8c3fded66 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, | ||
+ 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, | ||
+ 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, | ||
+ 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..da1e168b4722c017f59ad65d90a48ecdf7a43238 100644 | ||
--- a/content/public/browser/storage_partition_config.h | ||
+++ b/content/public/browser/storage_partition_config.h | ||
@@ -28,7 +28,9 @@ 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); | ||
+ // A |quota_size| if specified will take effect. | ||
+ static StoragePartitionConfig CreateDefault(BrowserContext* browser_context, | ||
+ int quota_size = 0); | ||
|
||
// Creates a config tied to a specific domain. | ||
// The |partition_domain| is [a-z]* UTF-8 string, specifying the domain in | ||
@@ -43,11 +45,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, | ||
+ int quota_size = 0); | ||
|
||
std::string partition_domain() const { return partition_domain_; } | ||
std::string partition_name() const { return partition_name_; } | ||
bool in_memory() const { return in_memory_; } | ||
+ 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 +98,13 @@ class CONTENT_EXPORT StoragePartitionConfig { | ||
|
||
StoragePartitionConfig(const std::string& partition_domain, | ||
const std::string& partition_name, | ||
- bool in_memory); | ||
+ bool in_memory, | ||
+ int quota_size); | ||
|
||
std::string partition_domain_; | ||
std::string partition_name_; | ||
bool in_memory_ = false; | ||
+ int quota_size_ = 0; | ||
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 5e684c7ec82456fae4e5515c63cac28c5d928a39..d88dfea46f90d1980b087c3e2b6cd7645c52f5ac 100644 | ||
--- a/storage/browser/quota/quota_manager_impl.cc | ||
+++ b/storage/browser/quota/quota_manager_impl.cc | ||
@@ -2602,7 +2602,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())); | ||
} | ||
@@ -2924,16 +2924,26 @@ void QuotaManagerImpl::PostTaskAndReplyWithResultForDBThread( | ||
std::move(reply)); | ||
} | ||
|
||
+void QuotaManagerImpl::SetStartupQuota(int64_t start_quota) { | ||
+ // Set the startup quota */ | ||
+ // 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<int64_t>& 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; | ||
|
||
@@ -2955,9 +2965,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<int64_t>& 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()); | ||
+ } | ||
} | ||
|
||
} // namespace storage | ||
diff --git a/storage/browser/quota/quota_manager_impl.h b/storage/browser/quota/quota_manager_impl.h | ||
index 6941faf7f4ab294bc48618d1b9b45238c9a6e9a6..dc4da3233bce7c432382fe73a9054a2af8dd40e5 100644 | ||
--- a/storage/browser/quota/quota_manager_impl.h | ||
+++ b/storage/browser/quota/quota_manager_impl.h | ||
@@ -154,7 +154,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<int64_t>&); | ||
|
||
static constexpr int64_t kGBytes = 1024 * 1024 * 1024; | ||
static constexpr int64_t kNoLimit = INT64_MAX; | ||
@@ -468,6 +469,7 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl | ||
eviction_disabled_ = disable; | ||
} | ||
|
||
+ void SetStartupQuota(const int64_t start_quota); | ||
// Testing support for handling corruption in the underlying database. | ||
// | ||
// Runs `corrupter` on the same sequence used to do database I/O, | ||
@@ -729,9 +731,13 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl | ||
const base::Location& from_here = base::Location::Current(), | ||
bool is_bootstrap_task = false); | ||
|
||
- static QuotaAvailability CallGetVolumeInfo(GetVolumeInfoFn get_volume_info_fn, | ||
- const base::FilePath& path); | ||
- static QuotaAvailability GetVolumeInfo(const base::FilePath& path); | ||
+ static QuotaAvailability CallGetVolumeInfo( | ||
+ GetVolumeInfoFn get_volume_info_fn, | ||
+ const base::FilePath& path, | ||
+ const absl::optional<int64_t>& quota_size = absl::nullopt); | ||
+ static QuotaAvailability GetVolumeInfo( | ||
+ const base::FilePath& path, | ||
+ const absl::optional<int64_t>& quota_size = absl::nullopt); | ||
|
||
const bool is_incognito_; | ||
const base::FilePath profile_path_; | ||
@@ -828,6 +834,8 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl | ||
// QuotaManagerImpl::GetVolumeInfo. | ||
GetVolumeInfoFn get_volume_info_fn_; | ||
|
||
+ absl::optional<int64_t> start_quota_ = absl::nullopt; | ||
+ | ||
std::unique_ptr<EvictionRoundInfoHelper> eviction_helper_; | ||
std::map<BucketSetDataDeleter*, std::unique_ptr<BucketSetDataDeleter>> | ||
bucket_set_data_deleters_; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,30 @@ describe('session module', () => { | |
}); | ||
}); | ||
|
||
describe('session.fromPartition(partition, options)', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also need test to make sure default values are used when quotasize option is not defined. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did try to add a test which queries the disk free space of the specified path. However, this test then cannot be run on all platforms and hence would fail. The procedure I followed was to use the childProcess to run |
||
afterEach(closeAllWindows); | ||
it('assert that a set quota value is returned when a quota value is specified for a session', async () => { | ||
const quotasize = 256000; | ||
const localsession = session.fromPartition('persist:test', { cache: false, quota: quotasize }); | ||
const w = new BrowserWindow({ | ||
show: false, | ||
webPreferences: { | ||
session: localsession | ||
} | ||
}); | ||
|
||
const readQuotaSize: any = () => { | ||
return w.webContents.executeJavaScript(` | ||
navigator.storage.estimate().then(estimate => estimate.quota).catch(err => err.message); | ||
`); | ||
}; | ||
|
||
await w.loadURL('https://www.google.com'); | ||
const size = await readQuotaSize(); | ||
expect(size).to.equal(quotasize); | ||
}); | ||
}); | ||
|
||
describe('ses.cookies', () => { | ||
const name = '0'; | ||
const value = '0'; | ||
|
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.
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_));