AppCache: Ensure inflight ActiveCalls are not destroyed

content/browser/appcache/appcache_disk_cache.cc

Index: content/browser/appcache/appcache_disk_cache.cc
diff --git a/content/browser/appcache/appcache_disk_cache.cc b/content/browser/appcache/appcache_disk_cache.cc
index 382ed45fed2197ae8be5b861e1f310d18a118694..7bb77d54daba048c4febaff225a44598cae8a994 100644
--- a/content/browser/appcache/appcache_disk_cache.cc
+++ b/content/browser/appcache/appcache_disk_cache.cc
@@ -116,68 +116,86 @@ class AppCacheDiskCache::EntryImpl : public Entry {
 // 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_) {
+      DCHECK(entry_ptr_);
+      *entry_ = new EntryImpl(entry_ptr_, owner_.get());
     }
-    if (rv == net::OK && entry)
-      *entry = new EntryImpl(entry_ptr_, owner_);
     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() {
@@ -225,7 +243,6 @@ void AppCacheDiskCache::Disable() {
   }
   open_entries_.clear();
   disk_cache_.reset();
-  STLDeleteElements(&active_calls_);
 }
 
 int AppCacheDiskCache::CreateEntry(int64 key, Entry** entry,
@@ -243,7 +260,8 @@ int AppCacheDiskCache::CreateEntry(int64 key, Entry** entry,
   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,
@@ -261,7 +279,8 @@ int AppCacheDiskCache::OpenEntry(int64 key, Entry** entry,
   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,
@@ -278,7 +297,7 @@ int AppCacheDiskCache::DoomEntry(int64 key,
   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()