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..79efd83887941a68d4eb5d00ce1b621e1f5874bb 100644
--- a/content/browser/appcache/appcache_disk_cache.cc
+++ b/content/browser/appcache/appcache_disk_cache.cc
@@ -119,14 +119,15 @@ class AppCacheDiskCache::ActiveCall {
   explicit ActiveCall(AppCacheDiskCache* owner)
       : entry_(NULL),
         owner_(owner),
-        entry_ptr_(NULL) {
+        entry_ptr_(NULL),
+        weak_factory_(this) {
   }
 
   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)));
+        base::Bind(&ActiveCall::OnAsyncCompletion, weak_factory_.GetWeakPtr()));

[michaeln, 2015/05/14 02:25:01]

I remember there being a difference in behavior between the backends when it
came to shutdown, but don't remember exactly what that difference is. Given the
weakptr usage, looks like the difference is that pending calls continue and
their completion callbacks can get invoked sometime after the disk_cache_ has
been deleted? Is that right?

If that's the case, we may need to bind ownership of the ActiveCall into the
closure itself. The DiskCache.Open/CreateEntry methods could write their results
to &entry_ptr_, a use-after-free bug.

[nhiroki, 2015/05/14 04:27:05]

I haven't had an absolute proof yet, but it seems most likely. I'm now
confirming that the situation actually happens:
https://codereview.chromium.org/1139573003/#ps20001
Good point. I'll remake this patch in that way.

     return HandleImmediateReturnValue(rv, entry, callback);
   }
 
@@ -134,17 +135,22 @@ class AppCacheDiskCache::ActiveCall {
                 const net::CompletionCallback& callback) {
     int rv = owner_->disk_cache()->OpenEntry(
         base::Int64ToString(key), &entry_ptr_,
-        base::Bind(&ActiveCall::OnAsyncCompletion, base::Unretained(this)));
+        base::Bind(&ActiveCall::OnAsyncCompletion, weak_factory_.GetWeakPtr()));
     return HandleImmediateReturnValue(rv, entry, callback);
   }
 
   int DoomEntry(int64 key, const net::CompletionCallback& callback) {
     int rv = owner_->disk_cache()->DoomEntry(
         base::Int64ToString(key),
-        base::Bind(&ActiveCall::OnAsyncCompletion, base::Unretained(this)));
+        base::Bind(&ActiveCall::OnAsyncCompletion, weak_factory_.GetWeakPtr()));
     return HandleImmediateReturnValue(rv, NULL, callback);
   }
 
+  void Abort() {
+    callback_.Run(net::ERR_ABORTED);
+    delete this;
+  }
+
  private:
   int HandleImmediateReturnValue(int rv, Entry** entry,
                                  const net::CompletionCallback& callback) {
@@ -166,7 +172,6 @@ class AppCacheDiskCache::ActiveCall {
     if (rv == net::OK && entry_)
       *entry_ = new EntryImpl(entry_ptr_, owner_);
     callback_.Run(rv);
-    callback_.Reset();
     delete this;
   }
 
@@ -174,6 +179,8 @@ class AppCacheDiskCache::ActiveCall {
   net::CompletionCallback callback_;
   AppCacheDiskCache* owner_;
   disk_cache::Entry* entry_ptr_;
+
+  base::WeakPtrFactory<AppCacheDiskCache::ActiveCall> weak_factory_;
 };
 
 AppCacheDiskCache::AppCacheDiskCache()
@@ -225,7 +232,10 @@ void AppCacheDiskCache::Disable() {
   }
   open_entries_.clear();
   disk_cache_.reset();
-  STLDeleteElements(&active_calls_);
+
+  for (AppCacheDiskCache::ActiveCall* active_call : active_calls_)
+    active_call->Abort();
+  active_calls_.clear();
 }
 
 int AppCacheDiskCache::CreateEntry(int64 key, Entry** entry,