Allow ServiceWorkerDiskCache operations to be overlapped

There seems a situation where multiple operations are queued at a
time for the same resource id. Since `active_entry_calls_` and
`active_doom_calls_` used resource id as a key, only one callback was
preserved. When DidGetEntryResult() or DidDoomEntry() were called for
the second time, these tried to find active calls from the maps but
callbacks were already consumed.

This CL replaces keys of `active_entry_calls_` and
`active_doom_calls_` with monotonically increasing `call_id_` to make
sure we don't lose the callback of each operation.

Bug: 1142779
Change-Id: Ie4a999c50bcee3417453da8bbe4b2b529462b91f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505338
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822045}

content/browser/service_worker/service_worker_disk_cache.cc

@@ -172,15 +172,16 @@ void ServiceWorkerDiskCache::CreateEntry(int64_t key, EntryCallback callback) {
     return;
   }
 
-  DCHECK(!base::Contains(active_entry_calls_, key));
-  active_entry_calls_.emplace(key, std::move(callback));
+  uint64_t call_id = GetNextCallId();
+  DCHECK(!base::Contains(active_entry_calls_, call_id));
+  active_entry_calls_.emplace(call_id, std::move(callback));
 
   disk_cache::EntryResult result = disk_cache_->CreateEntry(
       base::NumberToString(key), net::HIGHEST,
       base::BindOnce(&ServiceWorkerDiskCache::DidGetEntryResult,
-                     weak_factory_.GetWeakPtr(), key));
+                     weak_factory_.GetWeakPtr(), call_id));
   if (result.net_error() != net::ERR_IO_PENDING) {
-    DidGetEntryResult(key, std::move(result));
+    DidGetEntryResult(call_id, std::move(result));
   }
 }
 
@@ -206,15 +207,16 @@ void ServiceWorkerDiskCache::OpenEntry(int64_t key, EntryCallback callback) {
     return;
   }
 
-  DCHECK(!base::Contains(active_entry_calls_, key));
-  active_entry_calls_.emplace(key, std::move(callback));
+  uint64_t call_id = GetNextCallId();
+  DCHECK(!base::Contains(active_entry_calls_, call_id));
+  active_entry_calls_.emplace(call_id, std::move(callback));
 
   disk_cache::EntryResult result = disk_cache_->OpenEntry(
       base::NumberToString(key), net::HIGHEST,
       base::BindOnce(&ServiceWorkerDiskCache::DidGetEntryResult,
-                     weak_factory_.GetWeakPtr(), key));
+                     weak_factory_.GetWeakPtr(), call_id));
   if (result.net_error() != net::ERR_IO_PENDING) {
-    DidGetEntryResult(key, std::move(result));
+    DidGetEntryResult(call_id, std::move(result));
   }
 }
 
@@ -241,15 +243,16 @@ void ServiceWorkerDiskCache::DoomEntry(int64_t key,
     return;
   }
 
-  DCHECK(!base::Contains(active_doom_calls_, key));
-  active_doom_calls_.emplace(key, std::move(callback));
+  uint64_t call_id = GetNextCallId();
+  DCHECK(!base::Contains(active_doom_calls_, call_id));
+  active_doom_calls_.emplace(call_id, std::move(callback));
 
   net::Error net_error = disk_cache_->DoomEntry(
       base::NumberToString(key), net::HIGHEST,
       base::BindOnce(&ServiceWorkerDiskCache::DidDoomEntry,
-                     weak_factory_.GetWeakPtr(), key));
+                     weak_factory_.GetWeakPtr(), call_id));
   if (net_error != net::ERR_IO_PENDING) {
-    DidDoomEntry(key, net_error);
+    DidDoomEntry(call_id, net_error);
   }
 }
 
@@ -304,9 +307,13 @@ void ServiceWorkerDiskCache::OnCreateBackendComplete(int return_value) {
   pending_calls_.clear();
 }
 
-void ServiceWorkerDiskCache::DidGetEntryResult(int64_t key,
+uint64_t ServiceWorkerDiskCache::GetNextCallId() {
+  return next_call_id_++;
+}
+
+void ServiceWorkerDiskCache::DidGetEntryResult(uint64_t call_id,
                                                disk_cache::EntryResult result) {
-  auto it = active_entry_calls_.find(key);
+  auto it = active_entry_calls_.find(call_id);
   DCHECK(it != active_entry_calls_.end());
   EntryCallback callback = std::move(it->second);
   active_entry_calls_.erase(it);
@@ -321,8 +328,8 @@ void ServiceWorkerDiskCache::DidGetEntryResult(int64_t key,
   std::move(callback).Run(net_error, std::move(entry));
 }
 
-void ServiceWorkerDiskCache::DidDoomEntry(int64_t key, int net_error) {
-  auto it = active_doom_calls_.find(key);
+void ServiceWorkerDiskCache::DidDoomEntry(uint64_t call_id, int net_error) {
+  auto it = active_doom_calls_.find(call_id);
   DCHECK(it != active_doom_calls_.end());
   net::CompletionOnceCallback callback = std::move(it->second);
   active_doom_calls_.erase(it);

content/browser/service_worker/service_worker_disk_cache.h

@@ -87,8 +87,7 @@ class CONTENT_EXPORT ServiceWorkerDiskCache {
       base::OnceCallback<void(int rv,
                               std::unique_ptr<ServiceWorkerDiskCacheEntry>)>;
 
-  // Creates/opens/dooms a disk cache entry associated with `key`. These calls
-  // should not overlap.
+  // Creates/opens/dooms a disk cache entry associated with `key`.
   void CreateEntry(int64_t key, EntryCallback callback);
   void OpenEntry(int64_t key, EntryCallback callback);
   void DoomEntry(int64_t key, net::CompletionOnceCallback callback);
@@ -118,8 +117,10 @@ class CONTENT_EXPORT ServiceWorkerDiskCache {
                   net::CompletionOnceCallback callback);
   void OnCreateBackendComplete(int return_value);
 
-  void DidGetEntryResult(int64_t key, disk_cache::EntryResult result);
-  void DidDoomEntry(int64_t key, int net_error);
+  uint64_t GetNextCallId();
+
+  void DidGetEntryResult(uint64_t call_id, disk_cache::EntryResult result);
+  void DidDoomEntry(uint64_t call_id, int net_error);
 
   // Called by ServiceWorkerDiskCacheEntry constructor.
   void AddOpenEntry(ServiceWorkerDiskCacheEntry* entry);
@@ -131,8 +132,10 @@ class CONTENT_EXPORT ServiceWorkerDiskCache {
   net::CompletionOnceCallback init_callback_;
   scoped_refptr<CreateBackendCallbackShim> create_backend_callback_;
   std::vector<base::OnceClosure> pending_calls_;
-  std::map</*key=*/int64_t, EntryCallback> active_entry_calls_;
-  std::map</*key=*/int64_t, net::CompletionOnceCallback> active_doom_calls_;
+  uint64_t next_call_id_ = 0;
+  std::map</*call_id=*/uint64_t, EntryCallback> active_entry_calls_;
+  std::map</*call_id=*/uint64_t, net::CompletionOnceCallback>
+      active_doom_calls_;
   std::set<ServiceWorkerDiskCacheEntry*> open_entries_;
   std::unique_ptr<disk_cache::Backend> disk_cache_;
 

content/browser/service_worker/service_worker_disk_cache_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/service_worker/service_worker_disk_cache.h"
+
+#include "base/bind_helpers.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/test/bind_test_util.h"
+#include "base/test/task_environment.h"
+#include "net/base/net_errors.h"
+#include "net/disk_cache/disk_cache.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// TODO(bashi): Port tests from AppCacheDiskCacheTest.
+
+namespace content {
+
+class ServiceWorkerDiskCacheTest : public testing::Test {
+ public:
+  ServiceWorkerDiskCacheTest() = default;
+
+  void SetUp() override { ASSERT_TRUE(directory_.CreateUniqueTempDir()); }
+
+  void TearDown() override { FlushCacheTasks(); }
+
+  void FlushCacheTasks() {
+    disk_cache::FlushCacheThreadForTesting();
+    task_environment_.RunUntilIdle();
+  }
+
+  void InitializeDiskCache(ServiceWorkerDiskCache* disk_cache) {
+    base::RunLoop loop;
+    disk_cache->InitWithDiskBackend(
+        directory_.GetPath(), false,
+        /*post_cleanup_callback=*/base::DoNothing::Once(),
+        base::BindLambdaForTesting([&](int rv) {
+          ASSERT_EQ(rv, net::OK);
+          loop.Quit();
+        }));
+    loop.Run();
+  }
+
+ private:
+  base::test::TaskEnvironment task_environment_;
+  base::ScopedTempDir directory_;
+};
+
+// Tests that callbacks of operations are invoked even when these operations are
+// called at the same time for the same key.
+TEST_F(ServiceWorkerDiskCacheTest, MultipleCallsForSameKey) {
+  auto disk_cache = std::make_unique<ServiceWorkerDiskCache>();
+
+  bool create_entry_called = false;
+  bool open_entry_called = false;
+  bool doom_entry_called = false;
+
+  const int64_t kKey = 1;
+  disk_cache->CreateEntry(
+      kKey, base::BindLambdaForTesting(
+                [&](int rv, std::unique_ptr<ServiceWorkerDiskCacheEntry>) {
+                  create_entry_called = true;
+                }));
+  disk_cache->OpenEntry(
+      kKey, base::BindLambdaForTesting(
+                [&](int rv, std::unique_ptr<ServiceWorkerDiskCacheEntry>) {
+                  open_entry_called = true;
+                }));
+  disk_cache->DoomEntry(kKey, base::BindLambdaForTesting(
+                                  [&](int rv) { doom_entry_called = true; }));
+
+  InitializeDiskCache(disk_cache.get());
+  FlushCacheTasks();
+
+  EXPECT_TRUE(create_entry_called);
+  EXPECT_TRUE(open_entry_called);
+  EXPECT_TRUE(doom_entry_called);
+}
+
+}  // namespace content

content/test/BUILD.gn

@@ -1967,6 +1967,7 @@ test("content_unittests") {
     "../browser/service_worker/service_worker_context_wrapper_unittest.cc",
     "../browser/service_worker/service_worker_controllee_request_handler_unittest.cc",
     "../browser/service_worker/service_worker_database_unittest.cc",
+    "../browser/service_worker/service_worker_disk_cache_unittest.cc",
     "../browser/service_worker/service_worker_installed_scripts_sender_unittest.cc",
     "../browser/service_worker/service_worker_job_unittest.cc",
     "../browser/service_worker/service_worker_main_resource_loader_interceptor_unittest.cc",