AppCache: Ensure inflight ActiveCalls are not destroyed

content/browser/appcache/appcache_disk_cache.cc

 // Copyright (c) 2012 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/appcache/appcache_disk_cache.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
@@ skipping 98 matching lines @@
   }
 
   disk_cache::Entry* disk_cache_entry_;
   AppCacheDiskCache* owner_;
 };
 
 // Separate object to hold state for each Create, Delete, or Doom call
 // while the call is in-flight and to produce an EntryImpl upon completion.
 class AppCacheDiskCache::ActiveCall {
  public:
-  explicit ActiveCall(AppCacheDiskCache* owner)
-      : entry_(NULL),
-        owner_(owner),
-        entry_ptr_(NULL) {
+  static int CreateEntry(const base::WeakPtr<AppCacheDiskCache>& owner,
+                         int64 key, Entry** entry,
+                         const net::CompletionCallback& callback) {
+    ActiveCall* active_call = new ActiveCall(owner, entry, callback);
+    int rv = owner->disk_cache()->CreateEntry(
+        base::Int64ToString(key), &active_call->entry_ptr_,
+        base::Bind(&ActiveCall::OnAsyncCompletion,
+                   base::Unretained(active_call)));

[michaeln, 2015/05/14 23:24:00]

Hmmm... I think this will leak ActiveCall instances when used with the DEFAULT
backend?

The default backend (backend_impl.cc) drops pending io in its dtor without
invoking the completion callback.

The simple backend will get around to invoking completion callback sometime
after backend destruction.

Using base::Owned(active_call) might work provided the CompletionCallback was on
the stack to accomodate the immediate return case where the backend doesn't use
the callback to return the result.

[nhiroki, 2015/05/15 00:42:08]

Oh...
In this way, how can we manage the second arg (ie. entry_ptr) of
Create/Open/DoomEntry call? If |entry_ptr| is owned by |active_call|, the
callback on the stack for the immediate return case cannot access the ptr
because |active_call| is destroyed immediately after
CreateEntry/OpenEntry/DoomEntry call.

(RefCounted<ActiveCall> could work?)

[nhiroki, 2015/05/15 01:02:25]

Uploaded a refcounted version to PS4

+    return active_call->HandleImmediateReturnValue(rv);
   }
 
-  int CreateEntry(int64 key, Entry** entry,
-                  const net::CompletionCallback& callback) {
-    int rv = owner_->disk_cache()->CreateEntry(
-        base::Int64ToString(key), &entry_ptr_,
-        base::Bind(&ActiveCall::OnAsyncCompletion, base::Unretained(this)));
-    return HandleImmediateReturnValue(rv, entry, callback);
+  static int OpenEntry(const base::WeakPtr<AppCacheDiskCache>& owner,
+                       int64 key, Entry** entry,
+                       const net::CompletionCallback& callback) {
+    ActiveCall* active_call = new ActiveCall(owner, entry, callback);
+    int rv = owner->disk_cache()->OpenEntry(
+        base::Int64ToString(key), &active_call->entry_ptr_,
+        base::Bind(&ActiveCall::OnAsyncCompletion,
+                   base::Unretained(active_call)));
+    return active_call->HandleImmediateReturnValue(rv);
   }
 
-  int OpenEntry(int64 key, Entry** entry,
-                const net::CompletionCallback& callback) {
-    int rv = owner_->disk_cache()->OpenEntry(
-        base::Int64ToString(key), &entry_ptr_,
-        base::Bind(&ActiveCall::OnAsyncCompletion, base::Unretained(this)));
-    return HandleImmediateReturnValue(rv, entry, callback);
-  }
-
-  int DoomEntry(int64 key, const net::CompletionCallback& callback) {
-    int rv = owner_->disk_cache()->DoomEntry(
+  static int DoomEntry(const base::WeakPtr<AppCacheDiskCache>& owner,
+                       int64 key, const net::CompletionCallback& callback) {
+    ActiveCall* active_call = new ActiveCall(owner, nullptr, callback);
+    int rv = owner->disk_cache()->DoomEntry(
         base::Int64ToString(key),
-        base::Bind(&ActiveCall::OnAsyncCompletion, base::Unretained(this)));
-    return HandleImmediateReturnValue(rv, NULL, callback);
+        base::Bind(&ActiveCall::OnAsyncCompletion,
+                   base::Unretained(active_call)));
+    return active_call->HandleImmediateReturnValue(rv);
   }
 
  private:
-  int HandleImmediateReturnValue(int rv, Entry** entry,
-                                 const net::CompletionCallback& callback) {
+  ActiveCall(const base::WeakPtr<AppCacheDiskCache>& owner,
+             Entry** entry,
+             const net::CompletionCallback& callback)
+      : owner_(owner),
+        entry_(entry),
+        callback_(callback),
+        entry_ptr_(nullptr) {
+    DCHECK(owner_);
+  }
+
+  int HandleImmediateReturnValue(int rv) {
     if (rv == net::ERR_IO_PENDING) {
       // OnAsyncCompletion will be called later.
-      callback_ = callback;
-      entry_ = entry;
-      owner_->AddActiveCall(this);
-      return net::ERR_IO_PENDING;
+      return rv;
     }
-    if (rv == net::OK && entry)
-      *entry = new EntryImpl(entry_ptr_, owner_);
+
+    if (rv == net::OK && entry_) {
+      DCHECK(entry_ptr_);
+      *entry_ = new EntryImpl(entry_ptr_, owner_.get());
+    }
     delete this;
     return rv;
   }
 
   void OnAsyncCompletion(int rv) {
-    owner_->RemoveActiveCall(this);
-    if (rv == net::OK && entry_)
-      *entry_ = new EntryImpl(entry_ptr_, owner_);
+    if (rv == net::OK && entry_) {
+      DCHECK(entry_ptr_);
+      if (owner_) {
+        *entry_ = new EntryImpl(entry_ptr_, owner_.get());
+      } else {
+        entry_ptr_->Close();
+        rv = net::ERR_ABORTED;
+      }
+    }
     callback_.Run(rv);
-    callback_.Reset();
     delete this;
   }
 
+  base::WeakPtr<AppCacheDiskCache> owner_;
   Entry** entry_;
   net::CompletionCallback callback_;
-  AppCacheDiskCache* owner_;
   disk_cache::Entry* entry_ptr_;
 };
 
 AppCacheDiskCache::AppCacheDiskCache()
-    : is_disabled_(false) {
+    : is_disabled_(false),
+      weak_factory_(this) {
 }
 
 AppCacheDiskCache::~AppCacheDiskCache() {
   Disable();
 }
 
 int AppCacheDiskCache::InitWithDiskBackend(
     const base::FilePath& disk_cache_directory,
     int disk_cache_size,
     bool force,
@@ skipping 27 matching lines @@
 
   // We need to close open file handles in order to reinitalize the
   // appcache system on the fly. File handles held in both entries and in
   // the main disk_cache::Backend class need to be released.
   for (OpenEntries::const_iterator iter = open_entries_.begin();
        iter != open_entries_.end(); ++iter) {
     (*iter)->Abandon();
   }
   open_entries_.clear();
   disk_cache_.reset();
-  STLDeleteElements(&active_calls_);
 }
 
 int AppCacheDiskCache::CreateEntry(int64 key, Entry** entry,
                                    const net::CompletionCallback& callback) {
   DCHECK(entry);
   DCHECK(!callback.is_null());
   if (is_disabled_)
     return net::ERR_ABORTED;
 
   if (is_initializing()) {
     pending_calls_.push_back(PendingCall(CREATE, key, entry, callback));
     return net::ERR_IO_PENDING;
   }
 
   if (!disk_cache_)
     return net::ERR_FAILED;
 
-  return (new ActiveCall(this))->CreateEntry(key, entry, callback);
+  return ActiveCall::CreateEntry(
+      weak_factory_.GetWeakPtr(), key, entry, callback);
 }
 
 int AppCacheDiskCache::OpenEntry(int64 key, Entry** entry,
                                  const net::CompletionCallback& callback) {
   DCHECK(entry);
   DCHECK(!callback.is_null());
   if (is_disabled_)
     return net::ERR_ABORTED;
 
   if (is_initializing()) {
     pending_calls_.push_back(PendingCall(OPEN, key, entry, callback));
     return net::ERR_IO_PENDING;
   }
 
   if (!disk_cache_)
     return net::ERR_FAILED;
 
-  return (new ActiveCall(this))->OpenEntry(key, entry, callback);
+  return ActiveCall::OpenEntry(
+      weak_factory_.GetWeakPtr(), key, entry, callback);
 }
 
 int AppCacheDiskCache::DoomEntry(int64 key,
                                  const net::CompletionCallback& callback) {
   DCHECK(!callback.is_null());
   if (is_disabled_)
     return net::ERR_ABORTED;
 
   if (is_initializing()) {
     pending_calls_.push_back(PendingCall(DOOM, key, NULL, callback));
     return net::ERR_IO_PENDING;
   }
 
   if (!disk_cache_)
     return net::ERR_FAILED;
 
-  return (new ActiveCall(this))->DoomEntry(key, callback);
+  return ActiveCall::DoomEntry(weak_factory_.GetWeakPtr(), key, callback);
 }
 
 AppCacheDiskCache::PendingCall::PendingCall()
     : call_type(CREATE),
       key(0),
       entry(NULL) {
 }
 
 AppCacheDiskCache::PendingCall::PendingCall(PendingCallType call_type,
     int64 key,
@@ skipping 71 matching lines @@
         NOTREACHED();
         break;
     }
     if (rv != net::ERR_IO_PENDING)
       iter->callback.Run(rv);
   }
   pending_calls_.clear();
 }
 
 }  // namespace content