CacheStorage: Wait for quota to update after writing to cache.
Currently there is a race where `navigator.storage.estimate()` called
immediately after a cache_storage write complete does not reflect any
size change caused by that write. This CL fixes the race by making
cache_storage wait for the quota state to be updated.
This CL also fixes some small inconsistencies in the test impacted by
this race condition.
Bug: 1176122,1177373
Change-Id: Id04776bc07f5ec52883a7aec276fb5d9baf52560
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2685380
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Auto-Submit: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jarryd Goodman <jarrydg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#854435}
diff --git a/content/browser/appcache/appcache_host_unittest.cc b/content/browser/appcache/appcache_host_unittest.cc
index 6ee5c60..1738e72d9 100644
--- a/content/browser/appcache/appcache_host_unittest.cc
+++ b/content/browser/appcache/appcache_host_unittest.cc
@@ -124,11 +124,17 @@
void NotifyStorageAccessed(const url::Origin& origin,
blink::mojom::StorageType type,
base::Time access_time) override {}
- void NotifyStorageModified(storage::QuotaClientType client_id,
- const url::Origin& origin,
- blink::mojom::StorageType type,
- int64_t delta,
- base::Time modification_time) override {}
+ void NotifyStorageModified(
+ storage::QuotaClientType client_id,
+ const url::Origin& origin,
+ blink::mojom::StorageType type,
+ int64_t delta,
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) override {
+ if (callback)
+ callback_task_runner->PostTask(FROM_HERE, std::move(callback));
+ }
void SetUsageCacheEnabled(storage::QuotaClientType client_id,
const url::Origin& origin,
blink::mojom::StorageType type,
diff --git a/content/browser/appcache/appcache_storage_impl_unittest.cc b/content/browser/appcache/appcache_storage_impl_unittest.cc
index 75431cd..0dd2370 100644
--- a/content/browser/appcache/appcache_storage_impl_unittest.cc
+++ b/content/browser/appcache/appcache_storage_impl_unittest.cc
@@ -189,16 +189,21 @@
last_origin_ = origin;
}
- void NotifyStorageModified(storage::QuotaClientType client_id,
- const url::Origin& origin,
- StorageType type,
- int64_t delta,
- base::Time modification_time) override {
+ void NotifyStorageModified(
+ storage::QuotaClientType client_id,
+ const url::Origin& origin,
+ StorageType type,
+ int64_t delta,
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) override {
EXPECT_EQ(storage::QuotaClientType::kAppcache, client_id);
EXPECT_EQ(StorageType::kTemporary, type);
++notify_storage_modified_count_;
last_origin_ = origin;
last_delta_ = delta;
+ if (callback)
+ callback_task_runner->PostTask(FROM_HERE, std::move(callback));
}
// Not needed for our tests.
diff --git a/content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc b/content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
index 58e1fc5..62c3937 100644
--- a/content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
+++ b/content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
@@ -2179,6 +2179,7 @@
void LegacyCacheStorageCache::CalculateCacheSize(
net::Int64CompletionOnceCallback callback) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
net::Int64CompletionRepeatingCallback got_size_callback =
AdaptCallbackForRepeating(std::move(callback));
int64_t rv = backend_->CalculateSizeOfAllEntries(got_size_callback);
@@ -2187,6 +2188,7 @@
}
void LegacyCacheStorageCache::UpdateCacheSize(base::OnceClosure callback) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (backend_state_ != BACKEND_OPEN)
return;
@@ -2202,6 +2204,7 @@
CacheStorageCacheHandle cache_handle,
base::OnceClosure callback,
int64_t current_cache_size) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_NE(current_cache_size, CacheStorage::kSizeUnknown);
cache_size_ = current_cache_size;
int64_t size_delta = PaddedCacheSize() - last_reported_size_;
@@ -2209,8 +2212,16 @@
quota_manager_proxy_->NotifyStorageModified(
CacheStorageQuotaClient::GetClientTypeFromOwner(owner_), origin_,
- blink::mojom::StorageType::kTemporary, size_delta, base::Time::Now());
+ blink::mojom::StorageType::kTemporary, size_delta, base::Time::Now(),
+ scheduler_task_runner_,
+ base::BindOnce(
+ &LegacyCacheStorageCache::UpdateCacheSizeNotifiedStorageModified,
+ weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+void LegacyCacheStorageCache::UpdateCacheSizeNotifiedStorageModified(
+ base::OnceClosure callback) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (cache_storage_)
cache_storage_->NotifyCacheContentChanged(cache_name_);
diff --git a/content/browser/cache_storage/legacy/legacy_cache_storage_cache.h b/content/browser/cache_storage/legacy/legacy_cache_storage_cache.h
index 0847dab..dd477c6 100644
--- a/content/browser/cache_storage/legacy/legacy_cache_storage_cache.h
+++ b/content/browser/cache_storage/legacy/legacy_cache_storage_cache.h
@@ -407,6 +407,7 @@
void UpdateCacheSizeGotSize(CacheStorageCacheHandle,
base::OnceClosure callback,
int64_t current_cache_size);
+ void UpdateCacheSizeNotifiedStorageModified(base::OnceClosure callback);
// GetAllMatchedEntries callbacks.
void GetAllMatchedEntriesImpl(blink::mojom::FetchAPIRequestPtr request,
diff --git a/storage/browser/database/database_tracker_unittest.cc b/storage/browser/database/database_tracker_unittest.cc
index 9810a86..03a534a 100644
--- a/storage/browser/database/database_tracker_unittest.cc
+++ b/storage/browser/database/database_tracker_unittest.cc
@@ -126,15 +126,20 @@
accesses_[origin] += 1;
}
- void NotifyStorageModified(QuotaClientType client_id,
- const url::Origin& origin,
- blink::mojom::StorageType type,
- int64_t delta,
- base::Time modification_time) override {
+ void NotifyStorageModified(
+ QuotaClientType client_id,
+ const url::Origin& origin,
+ blink::mojom::StorageType type,
+ int64_t delta,
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) override {
EXPECT_EQ(QuotaClientType::kDatabase, client_id);
EXPECT_EQ(blink::mojom::StorageType::kTemporary, type);
modifications_[origin].first += 1;
modifications_[origin].second += delta;
+ if (callback)
+ callback_task_runner->PostTask(FROM_HERE, std::move(callback));
}
// Not needed for our tests.
diff --git a/storage/browser/file_system/quota/quota_backend_impl_unittest.cc b/storage/browser/file_system/quota/quota_backend_impl_unittest.cc
index cdc4d12..84dc3a1a 100644
--- a/storage/browser/file_system/quota/quota_backend_impl_unittest.cc
+++ b/storage/browser/file_system/quota/quota_backend_impl_unittest.cc
@@ -57,14 +57,19 @@
blink::mojom::StorageType type,
bool enabled) override {}
- void NotifyStorageModified(QuotaClientType client_id,
- const url::Origin& origin,
- blink::mojom::StorageType type,
- int64_t delta,
- base::Time modification_time) override {
+ void NotifyStorageModified(
+ QuotaClientType client_id,
+ const url::Origin& origin,
+ blink::mojom::StorageType type,
+ int64_t delta,
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) override {
++storage_modified_count_;
usage_ += delta;
ASSERT_LE(usage_, quota_);
+ if (callback)
+ callback_task_runner->PostTask(FROM_HERE, std::move(callback));
}
void GetUsageAndQuota(
diff --git a/storage/browser/quota/quota_manager_impl.cc b/storage/browser/quota/quota_manager_impl.cc
index 3382235..7275126 100644
--- a/storage/browser/quota/quota_manager_impl.cc
+++ b/storage/browser/quota/quota_manager_impl.cc
@@ -1429,12 +1429,16 @@
const url::Origin& origin,
StorageType type,
int64_t delta,
- base::Time modification_time) {
+ base::Time modification_time,
+ base::OnceClosure callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
LazyInitialize();
DCHECK(GetUsageTracker(type));
GetUsageTracker(type)->UpdateUsageCache(client_id, origin, delta);
+ if (callback)
+ std::move(callback).Run();
+
if (db_disabled_)
return;
diff --git a/storage/browser/quota/quota_manager_impl.h b/storage/browser/quota/quota_manager_impl.h
index a44d169e..817f16e 100644
--- a/storage/browser/quota/quota_manager_impl.h
+++ b/storage/browser/quota/quota_manager_impl.h
@@ -209,7 +209,8 @@
const url::Origin& origin,
blink::mojom::StorageType type,
int64_t delta,
- base::Time modification_time);
+ base::Time modification_time,
+ base::OnceClosure callback);
// Called by storage backends via proxy.
//
diff --git a/storage/browser/quota/quota_manager_proxy.cc b/storage/browser/quota/quota_manager_proxy.cc
index b3938984..de6b66c 100644
--- a/storage/browser/quota/quota_manager_proxy.cc
+++ b/storage/browser/quota/quota_manager_proxy.cc
@@ -95,23 +95,42 @@
quota_manager_impl_->NotifyStorageAccessed(origin, type, access_time);
}
-void QuotaManagerProxy::NotifyStorageModified(QuotaClientType client_id,
- const url::Origin& origin,
- blink::mojom::StorageType type,
- int64_t delta,
- base::Time modification_time) {
+void QuotaManagerProxy::NotifyStorageModified(
+ QuotaClientType client_id,
+ const url::Origin& origin,
+ blink::mojom::StorageType type,
+ int64_t delta,
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) {
+ DCHECK(!callback || callback_task_runner);
if (!quota_manager_impl_task_runner_->RunsTasksInCurrentSequence()) {
quota_manager_impl_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(&QuotaManagerProxy::NotifyStorageModified, this,
- client_id, origin, type, delta, modification_time));
+ client_id, origin, type, delta, modification_time,
+ std::move(callback_task_runner), std::move(callback)));
return;
}
DCHECK_CALLED_ON_VALID_SEQUENCE(quota_manager_impl_sequence_checker_);
if (quota_manager_impl_) {
+ base::OnceClosure manager_callback;
+ if (callback) {
+ manager_callback = base::BindOnce(
+ [](scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) {
+ if (callback_task_runner->RunsTasksInCurrentSequence()) {
+ std::move(callback).Run();
+ return;
+ }
+ callback_task_runner->PostTask(FROM_HERE, std::move(callback));
+ },
+ std::move(callback_task_runner), std::move(callback));
+ }
quota_manager_impl_->NotifyStorageModified(client_id, origin, type, delta,
- modification_time);
+ modification_time,
+ std::move(manager_callback));
}
}
diff --git a/storage/browser/quota/quota_manager_proxy.h b/storage/browser/quota/quota_manager_proxy.h
index ce7649d..ac64cf62 100644
--- a/storage/browser/quota/quota_manager_proxy.h
+++ b/storage/browser/quota/quota_manager_proxy.h
@@ -78,11 +78,22 @@
virtual void NotifyStorageAccessed(const url::Origin& origin,
blink::mojom::StorageType type,
base::Time access_time);
- virtual void NotifyStorageModified(QuotaClientType client_id,
- const url::Origin& origin,
- blink::mojom::StorageType type,
- int64_t delta,
- base::Time modification_time);
+
+ // Notify the quota manager that storage has been modified for the given
+ // client. A |callback| may be optionally provided to be invoked on the
+ // given task runner when the quota system's state in memory has been
+ // updated. If a |callback| is provided then |callback_task_runner| must
+ // also be provided. If the quota manager runs on |callback_task_runner|,
+ // then the |callback| may be invoked synchronously.
+ virtual void NotifyStorageModified(
+ QuotaClientType client_id,
+ const url::Origin& origin,
+ blink::mojom::StorageType type,
+ int64_t delta,
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner = nullptr,
+ base::OnceClosure callback = base::OnceClosure());
+
virtual void NotifyOriginInUse(const url::Origin& origin);
virtual void NotifyOriginNoLongerInUse(const url::Origin& origin);
virtual void NotifyWriteFailed(const url::Origin& origin);
diff --git a/storage/browser/test/mock_quota_manager_proxy.cc b/storage/browser/test/mock_quota_manager_proxy.cc
index 0f7b0b2..390086c9 100644
--- a/storage/browser/test/mock_quota_manager_proxy.cc
+++ b/storage/browser/test/mock_quota_manager_proxy.cc
@@ -75,7 +75,9 @@
const url::Origin& origin,
blink::mojom::StorageType type,
int64_t delta,
- base::Time modification_time) {
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) {
++storage_modified_count_;
last_notified_origin_ = origin;
last_notified_type_ = type;
@@ -83,6 +85,8 @@
if (mock_quota_manager_) {
mock_quota_manager_->UpdateUsage(origin, type, delta);
}
+ if (callback)
+ callback_task_runner->PostTask(FROM_HERE, std::move(callback));
}
MockQuotaManagerProxy::~MockQuotaManagerProxy() {
diff --git a/storage/browser/test/mock_quota_manager_proxy.h b/storage/browser/test/mock_quota_manager_proxy.h
index 9879d908..8845d6e 100644
--- a/storage/browser/test/mock_quota_manager_proxy.h
+++ b/storage/browser/test/mock_quota_manager_proxy.h
@@ -71,11 +71,14 @@
// last_notified_type_ and last_notified_delta_ respecitvely.
// If non-null MockQuotaManager is given to the constructor this also
// updates the manager's internal usage information.
- void NotifyStorageModified(storage::QuotaClientType client_id,
- const url::Origin& origin,
- blink::mojom::StorageType type,
- int64_t delta,
- base::Time modification_time) override;
+ void NotifyStorageModified(
+ storage::QuotaClientType client_id,
+ const url::Origin& origin,
+ blink::mojom::StorageType type,
+ int64_t delta,
+ base::Time modification_time,
+ scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+ base::OnceClosure callback) override;
int notify_storage_accessed_count() const { return storage_accessed_count_; }
int notify_storage_modified_count() const { return storage_modified_count_; }
diff --git a/third_party/blink/web_tests/wpt_internal/cache_storage/padding.https.html b/third_party/blink/web_tests/wpt_internal/cache_storage/padding.https.html
index 4bbd38b..4c25707 100644
--- a/third_party/blink/web_tests/wpt_internal/cache_storage/padding.https.html
+++ b/third_party/blink/web_tests/wpt_internal/cache_storage/padding.https.html
@@ -105,8 +105,8 @@
}, 'Cache padding varies with request method.');
promise_test(async t => {
- const cache1 = await caches.open('foo');
- const cache2 = await caches.open('bar');
+ const cache1 = await caches.open('padding');
+ const cache2 = await caches.open('padding-2');
const net_response = await fetch(TARGET_URL, { mode: 'no-cors',
cache: 'reload' });
@@ -117,6 +117,7 @@
await cache2.put(TARGET_URL, cache_response);
await cache1.delete(TARGET_URL);
const usage2 = (await navigator.storage.estimate()).usageDetails.caches;
+ await cache2.delete(TARGET_URL);
assert_equals(usage1, usage2,
'The padding value should follow a response loaded from ' +
@@ -158,7 +159,7 @@
// Compute a padded usage value based on the response returned from our
// outer fetch.
- const cache = await caches.open('foo');
+ const cache = await caches.open('padding');
// Make sure to use the URL and not the full request. The main window
// and service worker requests may have different headers which can
// throw off the size comparison.
diff --git a/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-fetch-sw.js b/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-fetch-sw.js
index 017ec83b..65a585e4 100644
--- a/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-fetch-sw.js
+++ b/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-fetch-sw.js
@@ -8,7 +8,7 @@
// Before returning the response store a clone in cache_storage
// and compute its padded usage.
- const cache = await caches.open('foo');
+ const cache = await caches.open('padding');
// Make sure to use the URL and not the full request. The main window
// and service worker requests may have different headers which can
// throw off the size comparison.
diff --git a/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-install-sw.js b/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-install-sw.js
index fc902f6..5c2bbdf4 100644
--- a/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-install-sw.js
+++ b/third_party/blink/web_tests/wpt_internal/cache_storage/resources/padding-install-sw.js
@@ -6,7 +6,7 @@
self.addEventListener('install', evt => {
evt.waitUntil(async function() {
- const cache = await caches.open('foo');
+ const cache = await caches.open('padding');
const response = await fetch(TARGET_URL, { mode: 'no-cors',
cache: 'force-cache' });
await cache.put(TARGET_URL, response);