CacheStorage: Add test that drops ref during query.
Bug: 965087
Change-Id: I60a84ae30953f5dd72e84102331757382dfca649
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1621388
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661853}
diff --git a/content/browser/cache_storage/cache_storage_manager_unittest.cc b/content/browser/cache_storage/cache_storage_manager_unittest.cc
index 1ee61943..c5d6a67 100644
--- a/content/browser/cache_storage/cache_storage_manager_unittest.cc
+++ b/content/browser/cache_storage/cache_storage_manager_unittest.cc
@@ -33,6 +33,7 @@
#include "content/browser/cache_storage/cache_storage_cache_handle.h"
#include "content/browser/cache_storage/cache_storage_context_impl.h"
#include "content/browser/cache_storage/cache_storage_quota_client.h"
+#include "content/browser/cache_storage/cache_storage_scheduler.h"
#include "content/common/background_fetch/background_fetch_types.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/storage_partition.h"
@@ -124,6 +125,32 @@
bool paused_ = true;
};
+// Scheduler implementation that will invoke a callback after the
+// next operation has been started.
+class CallbackScheduler : public CacheStorageScheduler {
+ public:
+ explicit CallbackScheduler(base::OnceClosure callback)
+ : CacheStorageScheduler(CacheStorageSchedulerClient::kCache,
+ base::ThreadTaskRunnerHandle::Get()),
+ callback_(std::move(callback)) {}
+
+ protected:
+ void DispatchOperationTask(base::OnceClosure task) override {
+ auto wrapped = base::BindOnce(&CallbackScheduler::ExecuteTask,
+ base::Unretained(this), std::move(task));
+ CacheStorageScheduler::DispatchOperationTask(std::move(wrapped));
+ }
+
+ private:
+ void ExecuteTask(base::OnceClosure task) {
+ std::move(task).Run();
+ if (callback_)
+ std::move(callback_).Run();
+ }
+
+ base::OnceClosure callback_;
+};
+
class MockCacheStorageQuotaManagerProxy : public MockQuotaManagerProxy {
public:
MockCacheStorageQuotaManagerProxy(MockQuotaManager* quota_manager,
@@ -276,6 +303,13 @@
run_loop->Quit();
}
+ void CacheMatchAllCallback(base::RunLoop* run_loop,
+ CacheStorageError error,
+ std::vector<blink::mojom::FetchAPIResponsePtr>) {
+ callback_error_ = error;
+ run_loop->Quit();
+ }
+
void CreateStorageManager() {
ChromeBlobStorageContext* blob_storage_context(
ChromeBlobStorageContext::GetFor(&browser_context_));
@@ -1180,6 +1214,40 @@
<< "unreferenced cache not destroyed on critical memory pressure";
}
+TEST_F(CacheStorageManagerTest, DropReferenceDuringQuery) {
+ // Setup the cache and execute an operation to make sure all initialization
+ // is complete.
+ EXPECT_TRUE(Open(origin1_, "foo"));
+ base::WeakPtr<LegacyCacheStorageCache> cache =
+ LegacyCacheStorageCache::From(callback_cache_handle_)->AsWeakPtr();
+ EXPECT_FALSE(CacheMatch(callback_cache_handle_.value(),
+ GURL("http://example.com/foo")));
+
+ // Override the cache scheduler so that we can take an action below
+ // after the query operation begins.
+ base::RunLoop scheduler_loop;
+ auto scheduler =
+ std::make_unique<CallbackScheduler>(scheduler_loop.QuitClosure());
+ cache->SetSchedulerForTesting(std::move(scheduler));
+
+ // Perform a MatchAll() operation to trigger a full query of the cache
+ // that does not hit the fast path optimization.
+ base::RunLoop match_loop;
+ cache->MatchAll(
+ nullptr, nullptr, /* trace_id = */ 0,
+ base::BindOnce(&CacheStorageManagerTest::CacheMatchAllCallback,
+ base::Unretained(this), base::Unretained(&match_loop)));
+
+ // Wait for the MatchAll operation to begin.
+ scheduler_loop.Run();
+
+ // Clear the external cache handle.
+ callback_cache_handle_ = CacheStorageCacheHandle();
+
+ // Wait for the MatchAll operation to complete as expected.
+ match_loop.Run();
+ EXPECT_EQ(CacheStorageError::kSuccess, callback_error_);
+}
// A cache continues to work so long as there is a handle to it. Only after the
// last cache handle is deleted can the cache be freed.
TEST_P(CacheStorageManagerTestP, CacheWorksAfterDelete) {
diff --git a/content/browser/cache_storage/cache_storage_scheduler.cc b/content/browser/cache_storage/cache_storage_scheduler.cc
index 3532bf01..c549160 100644
--- a/content/browser/cache_storage/cache_storage_scheduler.cc
+++ b/content/browser/cache_storage/cache_storage_scheduler.cc
@@ -49,6 +49,10 @@
return running_operation_ || !pending_operations_.empty();
}
+void CacheStorageScheduler::DispatchOperationTask(base::OnceClosure task) {
+ base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(task));
+}
+
void CacheStorageScheduler::RunOperationIfIdle() {
if (!running_operation_ && !pending_operations_.empty()) {
// TODO(jkarlin): Run multiple operations in parallel where allowed.
@@ -60,9 +64,8 @@
running_operation_->op_type(),
base::TimeTicks::Now() - running_operation_->creation_ticks());
- base::ThreadTaskRunnerHandle::Get()->PostTask(
- FROM_HERE, base::BindOnce(&CacheStorageOperation::Run,
- running_operation_->AsWeakPtr()));
+ DispatchOperationTask(base::BindOnce(&CacheStorageOperation::Run,
+ running_operation_->AsWeakPtr()));
}
}
diff --git a/content/browser/cache_storage/cache_storage_scheduler.h b/content/browser/cache_storage/cache_storage_scheduler.h
index eda2a8f..3f0fbf7 100644
--- a/content/browser/cache_storage/cache_storage_scheduler.h
+++ b/content/browser/cache_storage/cache_storage_scheduler.h
@@ -53,6 +53,10 @@
weak_ptr_factory_.GetWeakPtr(), std::move(callback));
}
+ protected:
+ // virtual for testing
+ virtual void DispatchOperationTask(base::OnceClosure task);
+
private:
void RunOperationIfIdle();
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 943d276f..abd1706 100644
--- a/content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
+++ b/content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
@@ -901,6 +901,12 @@
quota_manager_proxy_->NotifyOriginNoLongerInUse(origin_);
}
+void LegacyCacheStorageCache::SetSchedulerForTesting(
+ std::unique_ptr<CacheStorageScheduler> scheduler) {
+ DCHECK(!scheduler_->ScheduledOperations());
+ scheduler_ = std::move(scheduler);
+}
+
LegacyCacheStorageCache::LegacyCacheStorageCache(
const url::Origin& origin,
CacheStorageOwner owner,
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 6610ef46..3764aab 100644
--- a/content/browser/cache_storage/legacy/legacy_cache_storage_cache.h
+++ b/content/browser/cache_storage/legacy/legacy_cache_storage_cache.h
@@ -198,6 +198,10 @@
void DropHandleRef() override;
bool IsUnreferenced() const override;
+ // Override the default scheduler with a customized scheduler for testing.
+ // The current scheduler must be idle.
+ void SetSchedulerForTesting(std::unique_ptr<CacheStorageScheduler> scheduler);
+
static LegacyCacheStorageCache* From(const CacheStorageCacheHandle& handle) {
return static_cast<LegacyCacheStorageCache*>(handle.value());
}
