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 101 matching lines @@
   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) {
+        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);
   }
 
   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)));
+        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) {
     if (rv == net::ERR_IO_PENDING) {
       // OnAsyncCompletion will be called later.
       callback_ = callback;
       entry_ = entry;
       owner_->AddActiveCall(this);
       return net::ERR_IO_PENDING;
     }
     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_);
     callback_.Run(rv);
-    callback_.Reset();
     delete this;
   }
 
   Entry** entry_;
   net::CompletionCallback callback_;
   AppCacheDiskCache* owner_;
   disk_cache::Entry* entry_ptr_;
+
+  base::WeakPtrFactory<AppCacheDiskCache::ActiveCall> weak_factory_;
 };
 
 AppCacheDiskCache::AppCacheDiskCache()
     : is_disabled_(false) {
 }
 
 AppCacheDiskCache::~AppCacheDiskCache() {
   Disable();
 }
 
@@ skipping 31 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_);
+
+  for (AppCacheDiskCache::ActiveCall* active_call : active_calls_)
+    active_call->Abort();
+  active_calls_.clear();
 }
 
 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()) {
@@ skipping 124 matching lines @@
         NOTREACHED();
         break;
     }
     if (rv != net::ERR_IO_PENDING)
       iter->callback.Run(rv);
   }
   pending_calls_.clear();
 }
 
 }  // namespace content