StoragePartitionHttpCacheDataRemover: Properly handle async result from both wasm and js caches

Both calls can't be invoked on the same step with the state machine as the
callback, as two asynchronous (net::ERR_IO_PENDING) returns mean that the callback
would be invoked twice, and it's not meant for that (what with one invocation queuing
self-deletion...)

Also improve testing somewhat to actually make sure the wasm stuff is deleted
as well.

Longer-term, this should probably be elsewhere, this method isn't generally used with
NetworkService (see: crbug.com/892652)

Bug: 892583
Change-Id: I61be72c19f4ec713a2c27e5c4be963f603f3df20
Reviewed-on: https://chromium-review.googlesource.com/c/1264917Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597572}

content/browser/browsing_data/storage_partition_http_cache_data_remover.cc

@@ -99,8 +99,8 @@ void StoragePartitionHttpCacheDataRemover::ClearedHttpCache() {
 
 // The expected state sequence is CacheState::NONE --> CacheState::CREATE_MAIN
 // --> CacheState::DELETE_MAIN --> CacheState::CREATE_MEDIA -->
-// CacheState::DELETE_MEDIA --> CacheState::DELETE_CODE --> CacheState::DONE,
-// and any errors are ignored.
+// CacheState::DELETE_MEDIA --> CacheState::DELETE_CODE_JS --> DELETE_CODE_WASM
+// -> CacheState::DONE, and any errors are ignored.
 void StoragePartitionHttpCacheDataRemover::DoClearCache(int rv) {
   DCHECK_NE(CacheState::NONE, next_cache_state_);
 
@@ -118,7 +118,7 @@ void StoragePartitionHttpCacheDataRemover::DoClearCache(int rv) {
         if (!getter) {
           next_cache_state_ = (next_cache_state_ == CacheState::CREATE_MAIN)
                                   ? CacheState::CREATE_MEDIA
-                                  : CacheState::DELETE_CODE;
+                                  : CacheState::DELETE_CODE_JS;
           break;
         }
 
@@ -145,7 +145,7 @@ void StoragePartitionHttpCacheDataRemover::DoClearCache(int rv) {
       case CacheState::DELETE_MEDIA: {
         next_cache_state_ = (next_cache_state_ == CacheState::DELETE_MAIN)
                                 ? CacheState::CREATE_MEDIA
-                                : CacheState::DELETE_CODE;
+                                : CacheState::DELETE_CODE_JS;
 
         // |cache_| can be null if it cannot be initialized.
         if (cache_) {
@@ -172,22 +172,28 @@ void StoragePartitionHttpCacheDataRemover::DoClearCache(int rv) {
         }
         break;
       }
-      case CacheState::DELETE_CODE: {
-        next_cache_state_ = CacheState::DONE;
-        if (generated_code_cache_context_) {
+      case CacheState::DELETE_CODE_JS: {
+        next_cache_state_ = CacheState::DELETE_CODE_WASM;
         // TODO(crbug.com/866419): Currently we just clear entire caches.
         // Change it to conditionally clear entries based on the filters.
-          auto callback = base::BindRepeating(
-              &StoragePartitionHttpCacheDataRemover::DoClearCache,
-              base::Unretained(this));
-          if (generated_code_cache_context_->generated_js_code_cache()) {
+        // Likewise for DELETE_CODE_WASM.
+        if (generated_code_cache_context_ &&
+            generated_code_cache_context_->generated_js_code_cache()) {
           rv = generated_code_cache_context_->generated_js_code_cache()
-                     ->ClearCache(callback);
+                   ->ClearCache(base::BindRepeating(
+                       &StoragePartitionHttpCacheDataRemover::DoClearCache,
+                       base::Unretained(this)));
         }
-          if (generated_code_cache_context_->generated_wasm_code_cache()) {
-            rv = generated_code_cache_context_->generated_wasm_code_cache()
-                     ->ClearCache(callback);
+        break;
       }
+      case CacheState::DELETE_CODE_WASM: {
+        next_cache_state_ = CacheState::DONE;
+        if (generated_code_cache_context_ &&
+            generated_code_cache_context_->generated_wasm_code_cache()) {
+          rv = generated_code_cache_context_->generated_wasm_code_cache()
+                   ->ClearCache(base::BindRepeating(
+                       &StoragePartitionHttpCacheDataRemover::DoClearCache,
+                       base::Unretained(this)));
         }
         break;
       }

content/browser/browsing_data/storage_partition_http_cache_data_remover.h

@@ -57,7 +57,8 @@ class StoragePartitionHttpCacheDataRemover {
     CREATE_MEDIA,
     DELETE_MAIN,
     DELETE_MEDIA,
-    DELETE_CODE,
+    DELETE_CODE_JS,
+    DELETE_CODE_WASM,
     DONE
   };
 

content/browser/storage_partition_impl_unittest.cc

@@ -287,26 +287,36 @@ class RemoveCodeCacheTester {
   explicit RemoveCodeCacheTester(GeneratedCodeCacheContext* code_cache_context)
       : code_cache_context_(code_cache_context) {}
 
-  bool ContainsEntry(GURL url, url::Origin origin) {
+  enum Cache { kJs, kWebAssembly };
+
+  bool ContainsEntry(Cache cache, GURL url, url::Origin origin) {
     entry_exists_ = false;
     GeneratedCodeCache::ReadDataCallback callback = base::BindRepeating(
         &RemoveCodeCacheTester::FetchEntryCallback, base::Unretained(this));
-    code_cache_context_->generated_js_code_cache()->FetchEntry(url, origin,
-                                                               callback);
+    GetCache(cache)->FetchEntry(url, origin, callback);
     await_completion_.BlockUntilNotified();
     return entry_exists_;
   }
 
-  void AddEntry(GURL url, url::Origin origin, const std::string& data) {
+  void AddEntry(Cache cache,
+                GURL url,
+                url::Origin origin,
+                const std::string& data) {
     std::vector<uint8_t> data_vector(data.begin(), data.end());
-    code_cache_context_->generated_js_code_cache()->WriteData(
-        url, origin, base::Time::Now(), data_vector);
+    GetCache(cache)->WriteData(url, origin, base::Time::Now(), data_vector);
     base::RunLoop().RunUntilIdle();
   }
 
   std::string received_data() { return received_data_; }
 
  private:
+  GeneratedCodeCache* GetCache(Cache cache) {
+    if (cache == kJs)
+      return code_cache_context_->generated_js_code_cache();
+    else
+      return code_cache_context_->generated_wasm_code_cache();
+  }
+
   void FetchEntryCallback(const base::Time& response_time,
                           const std::vector<uint8_t>& data) {
     if (!response_time.is_null()) {
@@ -1337,16 +1347,30 @@ TEST_F(StoragePartitionImplTest, ClearCodeCache) {
 
   url::Origin origin = kOrigin1;
   std::string data("SomeData");
-  tester.AddEntry(kResourceURL, origin, data);
-  EXPECT_TRUE(tester.ContainsEntry(kResourceURL, origin));
+  std::string data2("SomeData.wasm");
+  tester.AddEntry(RemoveCodeCacheTester::kJs, kResourceURL, origin, data);
+  tester.AddEntry(RemoveCodeCacheTester::kWebAssembly, kResourceURL, origin,
+                  data2);
+  EXPECT_TRUE(
+      tester.ContainsEntry(RemoveCodeCacheTester::kJs, kResourceURL, origin));
   EXPECT_EQ(tester.received_data(), data);
+  EXPECT_TRUE(tester.ContainsEntry(RemoveCodeCacheTester::kWebAssembly,
+                                   kResourceURL, origin));
+  EXPECT_EQ(tester.received_data(), data2);
 
   base::RunLoop run_loop;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&ClearCodeCache, partition, &run_loop));
   run_loop.Run();
 
-  EXPECT_FALSE(tester.ContainsEntry(kResourceURL, origin));
+  EXPECT_FALSE(
+      tester.ContainsEntry(RemoveCodeCacheTester::kJs, kResourceURL, origin));
+  EXPECT_FALSE(tester.ContainsEntry(RemoveCodeCacheTester::kWebAssembly,
+                                    kResourceURL, origin));
+
+  // Make sure there isn't a second invalid callback sitting in the queue.
+  // (this used to be a bug).
+  base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(StoragePartitionImplTest, ClearCodeCacheNoIsolatedCodeCache) {