Improve memory management in ServiceWorkerDiskCache

This is a refactoring CL to avoid raw pointer allocations in
ServiceWorkerDiskCache. To remove raw pointers, this CL changes
CreateEntry(), OpenEntry(), DoomEntry() from //net-style maybe-async
to always-call-callback style. This CL also removes
ServiceWorkerDiskCacheEntry::Close() to make its lifetime less
surprising.

Bug: 1117369, 586174
Change-Id: Ie310446b8b55361c47b778237af1a5f6f5582bf0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2484022
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820856}

content/browser/service_worker/service_worker_disk_cache.h

@@ -33,6 +33,7 @@ class CONTENT_EXPORT ServiceWorkerDiskCacheEntry {
   // on destruction. |cache| must outlive the newly created entry.
   ServiceWorkerDiskCacheEntry(disk_cache::Entry* disk_cache_entry,
                               ServiceWorkerDiskCache* cache);
+  ~ServiceWorkerDiskCacheEntry();
 
   // See `disk_cache::Entry::ReadData()`.
   int Read(int index,
@@ -48,21 +49,16 @@ class CONTENT_EXPORT ServiceWorkerDiskCacheEntry {
             int buf_len,
             net::CompletionOnceCallback callback);
   int64_t GetSize(int index);
-  // Closes the disk cache and destroyes the instance.
-  void Close();
 
   // Should only be called by ServiceWorkerDiskCache.
   void Abandon();
 
  private:
-  // Call Close() instead of calling this directly.
-  ~ServiceWorkerDiskCacheEntry();
-
   // The disk_cache::Entry is owned by this entry and closed on destruction.
   disk_cache::Entry* disk_cache_entry_;
 
   // The cache that this entry belongs to.
-  ServiceWorkerDiskCache* cache_;
+  ServiceWorkerDiskCache* const cache_;
 };
 
 // net::DiskCache wrapper for the cache used by service worker resources.
@@ -87,13 +83,15 @@ class CONTENT_EXPORT ServiceWorkerDiskCache {
   void Disable();
   bool is_disabled() const { return is_disabled_; }
 
-  net::Error CreateEntry(int64_t key,
-                         ServiceWorkerDiskCacheEntry** entry,
-                         net::CompletionOnceCallback callback);
-  net::Error OpenEntry(int64_t key,
-                       ServiceWorkerDiskCacheEntry** entry,
-                       net::CompletionOnceCallback callback);
-  net::Error DoomEntry(int64_t key, net::CompletionOnceCallback callback);
+  using EntryCallback =
+      base::OnceCallback<void(int rv,
+                              std::unique_ptr<ServiceWorkerDiskCacheEntry>)>;
+
+  // Creates/opens/dooms a disk cache entry associated with `key`. These calls
+  // should not overlap.
+  void CreateEntry(int64_t key, EntryCallback callback);
+  void OpenEntry(int64_t key, EntryCallback callback);
+  void DoomEntry(int64_t key, net::CompletionOnceCallback callback);
 
   base::WeakPtr<ServiceWorkerDiskCache> GetWeakPtr();
 
@@ -107,30 +105,6 @@ class CONTENT_EXPORT ServiceWorkerDiskCache {
   class CreateBackendCallbackShim;
   friend class ServiceWorkerDiskCacheEntry;
 
-  // PendingCalls allow CreateEntry, OpenEntry, and DoomEntry to be called
-  // immediately after construction, without waiting for the
-  // underlying disk_cache::Backend to be fully constructed. Early
-  // calls are queued up and serviced once the disk_cache::Backend is
-  // really ready to go.
-  enum class PendingCallType { kCreate, kOpen, kDoom };
-  struct PendingCall {
-    PendingCall(PendingCallType call_type,
-                int64_t key,
-                ServiceWorkerDiskCacheEntry** entry,
-                net::CompletionOnceCallback callback);
-    PendingCall(PendingCall&& other);
-    PendingCall(const PendingCall&) = delete;
-    PendingCall& operator=(const PendingCall&) = delete;
-    PendingCall& operator=(PendingCall&&) = delete;
-
-    ~PendingCall();
-
-    const PendingCallType call_type;
-    const int64_t key;
-    ServiceWorkerDiskCacheEntry** const entry;
-    net::CompletionOnceCallback callback;
-  };
-
   bool is_initializing_or_waiting_to_initialize() const {
     return create_backend_callback_.get() != nullptr ||
            is_waiting_to_initialize_;
@@ -144,6 +118,9 @@ 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);
+
   // Called by ServiceWorkerDiskCacheEntry constructor.
   void AddOpenEntry(ServiceWorkerDiskCacheEntry* entry);
   // Called by ServiceWorkerDiskCacheEntry destructor.
@@ -153,7 +130,9 @@ class CONTENT_EXPORT ServiceWorkerDiskCache {
   bool is_waiting_to_initialize_ = false;
   net::CompletionOnceCallback init_callback_;
   scoped_refptr<CreateBackendCallbackShim> create_backend_callback_;
-  std::vector<PendingCall> pending_calls_;
+  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_;
   std::set<ServiceWorkerDiskCacheEntry*> open_entries_;
   std::unique_ptr<disk_cache::Backend> disk_cache_;
 

content/browser/service_worker/service_worker_resource_ops.cc

@@ -132,11 +132,7 @@ DiskEntryCreator::DiskEntryCreator(
   DCHECK(disk_cache_);
 }
 
-DiskEntryCreator::~DiskEntryCreator() {
-  if (entry_) {
-    entry_->Close();
-  }
-}
+DiskEntryCreator::~DiskEntryCreator() = default;
 
 void DiskEntryCreator::EnsureEntryIsCreated(base::OnceClosure callback) {
   DCHECK(creation_phase_ == CreationPhase::kNoAttempt ||
@@ -150,122 +146,84 @@ void DiskEntryCreator::EnsureEntryIsCreated(base::OnceClosure callback) {
   }
 
   if (!disk_cache_) {
-    entry_ = nullptr;
+    entry_.reset();
     RunEnsureEntryIsCreatedCallback();
     return;
   }
 
-  ServiceWorkerDiskCacheEntry** entry_ptr = new ServiceWorkerDiskCacheEntry*;
   creation_phase_ = CreationPhase::kInitialAttempt;
-  int rv = disk_cache_->CreateEntry(
-      resource_id_, entry_ptr,
-      base::BindOnce(&DidCreateEntryForFirstAttempt, weak_factory_.GetWeakPtr(),
-                     entry_ptr));
-  if (rv != net::ERR_IO_PENDING) {
-    DidCreateEntryForFirstAttempt(weak_factory_.GetWeakPtr(), entry_ptr, rv);
-  }
+  disk_cache_->CreateEntry(
+      resource_id_,
+      base::BindOnce(&DiskEntryCreator::DidCreateEntryForFirstAttempt,
+                     weak_factory_.GetWeakPtr()));
 }
 
-// static
 void DiskEntryCreator::DidCreateEntryForFirstAttempt(
-    base::WeakPtr<DiskEntryCreator> entry_creator,
-    ServiceWorkerDiskCacheEntry** entry,
-    int rv) {
-  if (!entry_creator) {
-    delete entry;
-    return;
-  }
+    int rv,
+    std::unique_ptr<ServiceWorkerDiskCacheEntry> entry) {
+  DCHECK_EQ(creation_phase_, CreationPhase::kInitialAttempt);
+  DCHECK(!entry_);
 
-  DCHECK_EQ(entry_creator->creation_phase_, CreationPhase::kInitialAttempt);
-  DCHECK(!entry_creator->entry_);
-
-  if (!entry_creator->disk_cache_) {
-    delete entry;
-    entry_creator->entry_ = nullptr;
-    entry_creator->RunEnsureEntryIsCreatedCallback();
+  if (!disk_cache_) {
+    entry_.reset();
+    RunEnsureEntryIsCreatedCallback();
     return;
   }
 
   if (rv != net::OK) {
     // The first attempt to create an entry is failed. Try to overwrite the
     // existing entry.
-    delete entry;
-    entry_creator->creation_phase_ = CreationPhase::kDoomExisting;
-    rv = entry_creator->disk_cache_->DoomEntry(
-        entry_creator->resource_id_,
-        base::BindOnce(&DiskEntryCreator::DidDoomExistingEntry, entry_creator));
-    if (rv != net::ERR_IO_PENDING) {
-      DidDoomExistingEntry(entry_creator, rv);
-    }
+    creation_phase_ = CreationPhase::kDoomExisting;
+    disk_cache_->DoomEntry(
+        resource_id_, base::BindOnce(&DiskEntryCreator::DidDoomExistingEntry,
+                                     weak_factory_.GetWeakPtr()));
     return;
   }
 
   DCHECK(entry);
-  entry_creator->entry_ = *entry;
-  delete entry;
-  entry_creator->RunEnsureEntryIsCreatedCallback();
+  entry_ = std::move(entry);
+  RunEnsureEntryIsCreatedCallback();
 }
 
-// static
-void DiskEntryCreator::DidDoomExistingEntry(
-    base::WeakPtr<DiskEntryCreator> entry_creator,
-    int rv) {
-  if (!entry_creator) {
-    return;
-  }
-
-  DCHECK_EQ(entry_creator->creation_phase_, CreationPhase::kDoomExisting);
-  DCHECK(!entry_creator->entry_);
+void DiskEntryCreator::DidDoomExistingEntry(int rv) {
+  DCHECK_EQ(creation_phase_, CreationPhase::kDoomExisting);
+  DCHECK(!entry_);
 
-  if (!entry_creator->disk_cache_) {
-    entry_creator->entry_ = nullptr;
-    entry_creator->RunEnsureEntryIsCreatedCallback();
+  if (!disk_cache_) {
+    entry_.reset();
+    RunEnsureEntryIsCreatedCallback();
     return;
   }
 
-  entry_creator->creation_phase_ = CreationPhase::kSecondAttempt;
-  auto** entry_ptr = new ServiceWorkerDiskCacheEntry*;
-  rv = entry_creator->disk_cache_->CreateEntry(
-      entry_creator->resource_id_, entry_ptr,
+  creation_phase_ = CreationPhase::kSecondAttempt;
+  disk_cache_->CreateEntry(
+      resource_id_,
       base::BindOnce(&DiskEntryCreator::DidCreateEntryForSecondAttempt,
-                     entry_creator, entry_ptr));
-  if (rv != net::ERR_IO_PENDING) {
-    DidCreateEntryForSecondAttempt(entry_creator, entry_ptr, rv);
-  }
+                     weak_factory_.GetWeakPtr()));
 }
 
-// static
 void DiskEntryCreator::DidCreateEntryForSecondAttempt(
-    base::WeakPtr<DiskEntryCreator> entry_creator,
-    ServiceWorkerDiskCacheEntry** entry,
-    int rv) {
-  if (!entry_creator) {
-    delete entry;
-    return;
-  }
-
-  DCHECK_EQ(entry_creator->creation_phase_, CreationPhase::kSecondAttempt);
+    int rv,
+    std::unique_ptr<ServiceWorkerDiskCacheEntry> entry) {
+  DCHECK_EQ(creation_phase_, CreationPhase::kSecondAttempt);
 
-  if (!entry_creator->disk_cache_) {
-    delete entry;
-    entry_creator->entry_ = nullptr;
-    entry_creator->RunEnsureEntryIsCreatedCallback();
+  if (!disk_cache_) {
+    entry_.reset();
+    RunEnsureEntryIsCreatedCallback();
     return;
   }
 
   if (rv != net::OK) {
     // The second attempt is also failed. Give up creating an entry.
-    delete entry;
-    entry_creator->entry_ = nullptr;
-    entry_creator->RunEnsureEntryIsCreatedCallback();
+    entry_.reset();
+    RunEnsureEntryIsCreatedCallback();
     return;
   }
 
-  DCHECK(!entry_creator->entry_);
+  DCHECK(!entry_);
   DCHECK(entry);
-  entry_creator->entry_ = *entry;
-  entry_creator->RunEnsureEntryIsCreatedCallback();
-  delete entry;
+  entry_ = std::move(entry);
+  RunEnsureEntryIsCreatedCallback();
 }
 
 void DiskEntryCreator::RunEnsureEntryIsCreatedCallback() {
@@ -281,52 +239,35 @@ DiskEntryOpener::DiskEntryOpener(
   DCHECK(disk_cache_);
 }
 
-DiskEntryOpener::~DiskEntryOpener() {
-  if (entry_) {
-    entry_->Close();
-  }
-}
+DiskEntryOpener::~DiskEntryOpener() = default;
 
 void DiskEntryOpener::EnsureEntryIsOpen(base::OnceClosure callback) {
-  DCHECK(!ensure_entry_is_opened_callback_);
-  ensure_entry_is_opened_callback_ = std::move(callback);
-
-  int rv;
-  ServiceWorkerDiskCacheEntry** entry_ptr = nullptr;
   if (entry_) {
-    rv = net::OK;
-  } else if (!disk_cache_) {
-    rv = net::ERR_FAILED;
-  } else {
-    entry_ptr = new ServiceWorkerDiskCacheEntry*;
-    rv = disk_cache_->OpenEntry(
-        resource_id_, entry_ptr,
-        base::BindOnce(&DiskEntryOpener::DidOpenEntry,
-                       weak_factory_.GetWeakPtr(), entry_ptr));
-  }
-
-  if (rv != net::ERR_IO_PENDING) {
-    DidOpenEntry(weak_factory_.GetWeakPtr(), entry_ptr, rv);
+    std::move(callback).Run();
+    return;
   }
-}
 
-// static
-void DiskEntryOpener::DidOpenEntry(base::WeakPtr<DiskEntryOpener> entry_opener,
-                                   ServiceWorkerDiskCacheEntry** entry,
-                                   int rv) {
-  if (!entry_opener) {
-    delete entry;
+  if (!disk_cache_) {
+    std::move(callback).Run();
     return;
   }
 
-  if (!entry_opener->entry_ && rv == net::OK) {
+  disk_cache_->OpenEntry(
+      resource_id_,
+      base::BindOnce(&DiskEntryOpener::DidOpenEntry, weak_factory_.GetWeakPtr(),
+                     std::move(callback)));
+}
+
+void DiskEntryOpener::DidOpenEntry(
+    base::OnceClosure callback,
+    int rv,
+    std::unique_ptr<ServiceWorkerDiskCacheEntry> entry) {
+  if (!entry_ && rv == net::OK) {
     DCHECK(entry);
-    entry_opener->entry_ = *entry;
+    entry_ = std::move(entry);
   }
-  delete entry;
 
-  DCHECK(entry_opener->ensure_entry_is_opened_callback_);
-  std::move(entry_opener->ensure_entry_is_opened_callback_).Run();
+  std::move(callback).Run();
 }
 
 class ServiceWorkerResourceReaderImpl::DataReader {

content/browser/service_worker/service_worker_resource_ops.h

@@ -26,7 +26,7 @@ class DiskEntryCreator {
   // Can be nullptr when a disk cache error occurs.
   ServiceWorkerDiskCacheEntry* entry() {
     DCHECK_EQ(creation_phase_, CreationPhase::kDone);
-    return entry_;
+    return entry_.get();
   }
 
   // Calls the callback when entry() is created and can be used.
@@ -52,27 +52,20 @@ class DiskEntryCreator {
     kDone,
   };
 
-  // Callbacks of EnsureEntryIsCreated(). These are static to manage the
-  // ownership of ServiceWorkerDiskCacheEntry correctly.
-  // TODO(crbug.com/586174): Refactor service worker's disk cache to use
-  // disk_cache::EntryResult to make these callbacks non-static.
-  static void DidCreateEntryForFirstAttempt(
-      base::WeakPtr<DiskEntryCreator> entry_creator,
-      ServiceWorkerDiskCacheEntry** entry,
-      int rv);
-  static void DidDoomExistingEntry(
-      base::WeakPtr<DiskEntryCreator> entry_creator,
-      int rv);
-  static void DidCreateEntryForSecondAttempt(
-      base::WeakPtr<DiskEntryCreator> entry_creator,
-      ServiceWorkerDiskCacheEntry** entry,
-      int rv);
+  // Callbacks of EnsureEntryIsCreated().
+  void DidCreateEntryForFirstAttempt(
+      int rv,
+      std::unique_ptr<ServiceWorkerDiskCacheEntry> entry);
+  void DidDoomExistingEntry(int rv);
+  void DidCreateEntryForSecondAttempt(
+      int rv,
+      std::unique_ptr<ServiceWorkerDiskCacheEntry> entry);
 
   void RunEnsureEntryIsCreatedCallback();
 
   const int64_t resource_id_;
   base::WeakPtr<ServiceWorkerDiskCache> disk_cache_;
-  ServiceWorkerDiskCacheEntry* entry_ = nullptr;
+  std::unique_ptr<ServiceWorkerDiskCacheEntry> entry_;
 
   CreationPhase creation_phase_ = CreationPhase::kNoAttempt;
 
@@ -93,7 +86,7 @@ class DiskEntryOpener {
   DiskEntryOpener& operator=(const DiskEntryOpener&) = delete;
 
   // Can be nullptr when a disk cache error occurs.
-  ServiceWorkerDiskCacheEntry* entry() { return entry_; }
+  ServiceWorkerDiskCacheEntry* entry() { return entry_.get(); }
 
   // Calls the callback when entry() is opened and can be used.
   //
@@ -103,18 +96,13 @@ class DiskEntryOpener {
   void EnsureEntryIsOpen(base::OnceClosure callback);
 
  private:
-  // TODO(crbug.com/586174): Refactor service worker's disk cache to use
-  // disk_cache::EntryResult to make this callback non-static.
-  static void DidOpenEntry(base::WeakPtr<DiskEntryOpener> entry_creator,
-                           ServiceWorkerDiskCacheEntry** entry,
-                           int rv);
+  void DidOpenEntry(base::OnceClosure callback,
+                    int rv,
+                    std::unique_ptr<ServiceWorkerDiskCacheEntry> entry);
 
   const int64_t resource_id_;
   base::WeakPtr<ServiceWorkerDiskCache> disk_cache_;
-  ServiceWorkerDiskCacheEntry* entry_ = nullptr;
-
-  // Stored as a data member to handle //net-style maybe-async methods.
-  base::OnceClosure ensure_entry_is_opened_callback_;
+  std::unique_ptr<ServiceWorkerDiskCacheEntry> entry_;
 
   base::WeakPtrFactory<DiskEntryOpener> weak_factory_{this};
 };

content/browser/service_worker/service_worker_storage.cc

@@ -1449,11 +1449,9 @@ void ServiceWorkerStorage::ContinuePurgingResources() {
 
 void ServiceWorkerStorage::PurgeResource(int64_t id) {
   DCHECK(is_purge_pending_);
-  int rv = disk_cache()->DoomEntry(
+  disk_cache()->DoomEntry(
       id, base::BindOnce(&ServiceWorkerStorage::OnResourcePurged,
                          weak_factory_.GetWeakPtr(), id));
-  if (rv != net::ERR_IO_PENDING)
-    OnResourcePurged(id, rv);
 }
 
 void ServiceWorkerStorage::OnResourcePurged(int64_t id, int rv) {