Disk Cache: Replace the backend pointer of the ChildrenDeleter

with a weak pointer to avoid crashing at shutdown.

BUG=50082
TEST=net_unittests

Review URL: http://codereview.chromium.org/3054012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53838 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 68 insertions, 5 deletions

diff --git a/net/disk_cache/backend_impl.cc b/net/disk_cache/backend_impl.cc
index 5ae789e..619b5ed 100644
--- a/net/disk_cache/backend_impl.cc
+++ b/net/disk_cache/backend_impl.cc
@@ -328,6 +328,7 @@ int BackendImpl::CreateBackend(const FilePath& full_path, bool force,
                                uint32 flags, base::MessageLoopProxy* thread,
                                Backend** backend,
                                CompletionCallback* callback) {
+  DCHECK(callback);
   CacheCreator* creator = new CacheCreator(full_path, force, max_bytes, type,
                                            flags, thread, backend, callback);
   // This object will self-destroy when finished.
@@ -538,6 +539,7 @@ void BackendImpl::CleanupCache() {
     DCHECK(!num_refs_);
   }
   factory_.RevokeAll();
+  ptr_factory_.InvalidateWeakPtrs();
   done_.Signal();
 }
 
diff --git a/net/disk_cache/backend_impl.h b/net/disk_cache/backend_impl.h
index d67dad9..f64f3c8 100644
--- a/net/disk_cache/backend_impl.h
+++ b/net/disk_cache/backend_impl.h
@@ -43,7 +43,8 @@ class BackendImpl : public Backend {
         cache_type_(net::DISK_CACHE), uma_report_(0), user_flags_(0),
         init_(false), restarted_(false), unit_test_(false), read_only_(false),
         new_eviction_(false), first_timer_(true), done_(true, false),
-        ALLOW_THIS_IN_INITIALIZER_LIST(factory_(this)) {}
+        ALLOW_THIS_IN_INITIALIZER_LIST(factory_(this)),
+        ALLOW_THIS_IN_INITIALIZER_LIST(ptr_factory_(this)) {}
   // mask can be used to limit the usable size of the hash table, for testing.
   BackendImpl(const FilePath& path, uint32 mask,
               base::MessageLoopProxy* cache_thread)
@@ -52,7 +53,8 @@ class BackendImpl : public Backend {
         cache_type_(net::DISK_CACHE), uma_report_(0), user_flags_(kMask),
         init_(false), restarted_(false), unit_test_(false), read_only_(false),
         new_eviction_(false), first_timer_(true), done_(true, false),
-        ALLOW_THIS_IN_INITIALIZER_LIST(factory_(this)) {}
+        ALLOW_THIS_IN_INITIALIZER_LIST(factory_(this)),
+        ALLOW_THIS_IN_INITIALIZER_LIST(ptr_factory_(this)) {}
   ~BackendImpl();
 
   // Returns a new backend with the desired flags. See the declaration of
@@ -185,6 +187,11 @@ class BackendImpl : public Backend {
     return cache_type_;
   }
 
+  // Returns a weak pointer to this object.
+  base::WeakPtr<BackendImpl> GetWeakPtr() {
+    return ptr_factory_.GetWeakPtr();
+  }
+
   // Returns the group for this client, based on the current cache size.
   int GetSizeGroup() const;
 
@@ -338,6 +345,7 @@ class BackendImpl : public Backend {
   base::WaitableEvent done_;  // Signals the end of background work.
   scoped_refptr<TraceObject> trace_object_;  // Inits internal tracing.
   ScopedRunnableMethodFactory<BackendImpl> factory_;
+  base::WeakPtrFactory<BackendImpl> ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BackendImpl);
 };
diff --git a/net/disk_cache/entry_unittest.cc b/net/disk_cache/entry_unittest.cc
index 067f260..859cabb 100644
--- a/net/disk_cache/entry_unittest.cc
+++ b/net/disk_cache/entry_unittest.cc
@@ -1287,6 +1287,56 @@ TEST_F(DiskCacheEntryTest, MemoryOnlyDoomSparseEntry) {
   DoomSparseEntry();
 }
 
+// A CompletionCallback that deletes the cache from within the callback. The way
+// a TestCompletionCallback works means that all tasks (even new ones) are
+// executed by the message loop before returning to the caller so the only way
+// to simulate a race is to execute what we want on the callback.
+class SparseTestCompletionCallback : public TestCompletionCallback {
+ public:
+  explicit SparseTestCompletionCallback(disk_cache::Backend* cache)
+      : cache_(cache) {}
+
+  virtual void RunWithParams(const Tuple1<int>& params) {
+    delete cache_;
+    TestCompletionCallback::RunWithParams(params);
+  }
+ private:
+  disk_cache::Backend* cache_;
+  DISALLOW_COPY_AND_ASSIGN(SparseTestCompletionCallback);
+};
+
+// Tests that we don't crash when the backend is deleted while we are working
+// deleting the sub-entries of a sparse entry.
+TEST_F(DiskCacheEntryTest, DoomSparseEntry2) {
+  SetDirectMode();
+  UseCurrentThread();
+  InitCache();
+  std::string key("the key");
+  disk_cache::Entry* entry;
+  ASSERT_EQ(net::OK, CreateEntry(key, &entry));
+
+  const int kSize = 4 * 1024;
+  scoped_refptr<net::IOBuffer> buf = new net::IOBuffer(kSize);
+  CacheTestFillBuffer(buf->data(), kSize, false);
+
+  int64 offset = 1024;
+  // Write to a bunch of ranges.
+  for (int i = 0; i < 12; i++) {
+    EXPECT_EQ(kSize, entry->WriteSparseData(offset, buf, kSize, NULL));
+    offset *= 4;
+  }
+  EXPECT_EQ(9, cache_->GetEntryCount());
+
+  entry->Close();
+  SparseTestCompletionCallback cb(cache_);
+  int rv = cache_->DoomEntry(key, &cb);
+  EXPECT_EQ(net::ERR_IO_PENDING, rv);
+  EXPECT_EQ(net::OK, cb.WaitForResult());
+
+  // TearDown will attempt to delete the cache_.
+  cache_ = NULL;
+}
+
 void DiskCacheEntryTest::PartialSparseEntry() {
   std::string key("the first key");
   disk_cache::Entry* entry;
diff --git a/net/disk_cache/sparse_control.cc b/net/disk_cache/sparse_control.cc
index 884a1b7..afd3317 100644
--- a/net/disk_cache/sparse_control.cc
+++ b/net/disk_cache/sparse_control.cc
@@ -51,7 +51,7 @@ class ChildrenDeleter
       public disk_cache::FileIOCallback {
  public:
   ChildrenDeleter(disk_cache::BackendImpl* backend, const std::string& name)
-      : backend_(backend), name_(name) {}
+      : backend_(backend->GetWeakPtr()), name_(name) {}
 
   virtual void OnFileIOComplete(int bytes_copied);
 
@@ -66,7 +66,7 @@ class ChildrenDeleter
 
   void DeleteChildren();
 
-  disk_cache::BackendImpl* backend_;
+  base::WeakPtr<disk_cache::BackendImpl> backend_;
   std::string name_;
   disk_cache::Bitmap children_map_;
   int64 signature_;
@@ -101,6 +101,9 @@ void ChildrenDeleter::Start(char* buffer, int len) {
 
 void ChildrenDeleter::ReadData(disk_cache::Addr address, int len) {
   DCHECK(address.is_block_file());
+  if (!backend_)
+    return Release();
+
   disk_cache::File* file(backend_->File(address));
   if (!file)
     return Release();
@@ -121,7 +124,7 @@ void ChildrenDeleter::ReadData(disk_cache::Addr address, int len) {
 
 void ChildrenDeleter::DeleteChildren() {
   int child_id = 0;
-  if (!children_map_.FindNextSetBit(&child_id)) {
+  if (!children_map_.FindNextSetBit(&child_id) || !backend_) {
     // We are done. Just delete this object.
     return Release();
   }