Make SimpleEntryImpl ref counted.

Upcoming work for multiple operations, CRC calculation, etc... have made the lifetime of the SimpleEntryImpl more tricky. By adding ref counting, asynchronous operations are guaranteed access to the entry after execution.

R=pasko@chromium.org,felipeg@chromium.org,rvargas@chromium.org
BUG=None

Review URL: https://codereview.chromium.org/13880016

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

net/disk_cache/simple/simple_backend_impl.cc

@@ -78,20 +78,18 @@ int32 SimpleBackendImpl::GetEntryCount() const {
 int SimpleBackendImpl::OpenEntry(const std::string& key,
                                  Entry** entry,
                                  const CompletionCallback& callback) {
-  return SimpleEntryImpl::OpenEntry(
-      index_->AsWeakPtr(), path_, key, entry, callback);
+  return SimpleEntryImpl::OpenEntry(index_, path_, key, entry, callback);
 }
 
 int SimpleBackendImpl::CreateEntry(const std::string& key,
                                    Entry** entry,
                                    const CompletionCallback& callback) {
-  return SimpleEntryImpl::CreateEntry(
-      index_->AsWeakPtr(), path_, key, entry, callback);
+  return SimpleEntryImpl::CreateEntry(index_, path_, key, entry, callback);
 }
 
 int SimpleBackendImpl::DoomEntry(const std::string& key,
                                  const net::CompletionCallback& callback) {
-  return SimpleEntryImpl::DoomEntry(index_->AsWeakPtr(), path_, key, callback);
+  return SimpleEntryImpl::DoomEntry(index_, path_, key, callback);
 }
 
 int SimpleBackendImpl::DoomAllEntries(const CompletionCallback& callback) {

net/disk_cache/simple/simple_backend_impl.h

@@ -11,9 +11,11 @@
 
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
 #include "base/task_runner.h"
 #include "net/base/cache_type.h"
 #include "net/disk_cache/disk_cache.h"
+#include "net/disk_cache/simple/simple_index.h"
 
 namespace base {
 class MessageLoopProxy;
@@ -29,8 +31,6 @@ namespace disk_cache {
 
 // See http://www.chromium.org/developers/design-documents/network-stack/disk-cache/very-simple-backend
 
-class SimpleIndex;
-
 class NET_EXPORT_PRIVATE SimpleBackendImpl : public Backend {
  public:
   SimpleBackendImpl(const base::FilePath& path, int max_bytes,
@@ -79,7 +79,7 @@ class NET_EXPORT_PRIVATE SimpleBackendImpl : public Backend {
       const InitializeIndexCallback& initialize_index_callback);
 
   const base::FilePath path_;
-  scoped_ptr<SimpleIndex> index_;
+  scoped_refptr<SimpleIndex> index_;
   const scoped_refptr<base::TaskRunner> cache_thread_;
 };
 

net/disk_cache/simple/simple_entry_impl.cc

@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/message_loop_proxy.h"
 #include "base/threading/worker_pool.h"
 #include "net/base/io_buffer.h"
@@ -30,11 +31,10 @@ namespace disk_cache {
 using base::FilePath;
 using base::MessageLoopProxy;
 using base::Time;
-using base::WeakPtr;
 using base::WorkerPool;
 
 // static
-int SimpleEntryImpl::OpenEntry(WeakPtr<SimpleIndex> index,
+int SimpleEntryImpl::OpenEntry(const scoped_refptr<SimpleIndex>& index,
                                const FilePath& path,
                                const std::string& key,
                                Entry** entry,
@@ -42,9 +42,11 @@ int SimpleEntryImpl::OpenEntry(WeakPtr<SimpleIndex> index,
   // TODO(gavinp): More closely unify the last_used_ in the
   // SimpleSynchronousEntry  and the SimpleIndex.
   if (!index || index->UseIfExists(key)) {
+    scoped_refptr<SimpleEntryImpl> new_entry =
+        new SimpleEntryImpl(index, path, key);
     SynchronousCreationCallback sync_creation_callback =
         base::Bind(&SimpleEntryImpl::CreationOperationComplete,
-                   index, callback, key, entry);
+                   new_entry, entry, callback);
     WorkerPool::PostTask(FROM_HERE,
                          base::Bind(&SimpleSynchronousEntry::OpenEntry, path,
                                     key, MessageLoopProxy::current(),
@@ -56,14 +58,16 @@ int SimpleEntryImpl::OpenEntry(WeakPtr<SimpleIndex> index,
 }
 
 // static
-int SimpleEntryImpl::CreateEntry(WeakPtr<SimpleIndex> index,
+int SimpleEntryImpl::CreateEntry(const scoped_refptr<SimpleIndex>& index,
                                  const FilePath& path,
                                  const std::string& key,
                                  Entry** entry,
                                  const CompletionCallback& callback) {
+  scoped_refptr<SimpleEntryImpl> new_entry =
+      new SimpleEntryImpl(index, path, key);
   SynchronousCreationCallback sync_creation_callback =
       base::Bind(&SimpleEntryImpl::CreationOperationComplete,
-                 index, callback, key, entry);
+                 new_entry, entry, callback);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::CreateEntry, path,
                                   key, MessageLoopProxy::current(),
@@ -73,11 +77,10 @@ int SimpleEntryImpl::CreateEntry(WeakPtr<SimpleIndex> index,
 }
 
 // static
-int SimpleEntryImpl::DoomEntry(WeakPtr<SimpleIndex> index,
+int SimpleEntryImpl::DoomEntry(const scoped_refptr<SimpleIndex>& index,
                                const FilePath& path,
                                const std::string& key,
                                const CompletionCallback& callback) {
-  if (index)
   index->Remove(key);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::DoomEntry, path, key,
@@ -88,6 +91,7 @@ int SimpleEntryImpl::DoomEntry(WeakPtr<SimpleIndex> index,
 
 void SimpleEntryImpl::Doom() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
 #if defined(OS_POSIX)
   // This call to static SimpleEntryImpl::DoomEntry() will just erase the
   // underlying files. On POSIX, this is fine; the files are still open on the
@@ -101,15 +105,7 @@ void SimpleEntryImpl::Doom() {
 
 void SimpleEntryImpl::Close() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
-  if (!synchronous_entry_in_use_by_worker_) {
-    WorkerPool::PostTask(FROM_HERE,
-                         base::Bind(&SimpleSynchronousEntry::Close,
-                                    base::Unretained(synchronous_entry_)),
-                         true);
-  }
-  // Entry::Close() is expected to release this entry. See disk_cache.h for
-  // details.
-  delete this;
+  Release();  // Balanced in CreationOperationCompleted().
 }
 
 std::string SimpleEntryImpl::GetKey() const {
@@ -147,12 +143,10 @@ int SimpleEntryImpl::ReadData(int index,
     CHECK(false);
   }
   synchronous_entry_in_use_by_worker_ = true;
-  if (index_)
   index_->UseIfExists(key_);
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
-                 index_, callback, weak_ptr_factory_.GetWeakPtr(),
-                 synchronous_entry_);
+                 this, callback);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::ReadData,
                                   base::Unretained(synchronous_entry_),
@@ -174,12 +168,10 @@ int SimpleEntryImpl::WriteData(int index,
     CHECK(false);
   }
   synchronous_entry_in_use_by_worker_ = true;
-  if (index_)
   index_->UseIfExists(key_);
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
-                 index_, callback, weak_ptr_factory_.GetWeakPtr(),
-                 synchronous_entry_);
+                 this, callback);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::WriteData,
                                   base::Unretained(synchronous_entry_),
@@ -238,70 +230,68 @@ int SimpleEntryImpl::ReadyForSparseIO(const CompletionCallback& callback) {
   return net::ERR_FAILED;
 }
 
-SimpleEntryImpl::SimpleEntryImpl(
-    SimpleSynchronousEntry* synchronous_entry,
-    WeakPtr<SimpleIndex> index)
-    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
-      path_(synchronous_entry->path()),
-      key_(synchronous_entry->key()),
-      synchronous_entry_(synchronous_entry),
-      synchronous_entry_in_use_by_worker_(false),
-      index_(index) {
-  DCHECK(synchronous_entry);
-  SetSynchronousData();
+SimpleEntryImpl::SimpleEntryImpl(const scoped_refptr<SimpleIndex>& index,
+                                 const base::FilePath& path,
+                                 const std::string& key)
+    : constructor_thread_(base::MessageLoopProxy::current()),
+      index_(index),
+      path_(path),
+      key_(key),
+      synchronous_entry_(NULL),
+      synchronous_entry_in_use_by_worker_(false) {
 }
 
 SimpleEntryImpl::~SimpleEntryImpl() {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
+  if (synchronous_entry_) {
+    base::Closure close_sync_entry =
+        base::Bind(&SimpleSynchronousEntry::Close,
+                   base::Unretained(synchronous_entry_));
+    // We aren't guaranteed to be able to run IO on our constructor thread, but
+    // we are also not guaranteed to be allowed to run WorkerPool::PostTask on
+    // our other threads.
+    if (constructor_thread_->BelongsToCurrentThread())
+      WorkerPool::PostTask(FROM_HERE, close_sync_entry, true);
+    else
+      close_sync_entry.Run();
+  }
 }
 
-// static
 void SimpleEntryImpl::CreationOperationComplete(
-    WeakPtr<SimpleIndex> index,
-    const CompletionCallback& completion_callback,
-    const std::string& key,
     Entry** out_entry,
+    const CompletionCallback& completion_callback,
     SimpleSynchronousEntry* sync_entry) {
+  DCHECK(io_thread_checker_.CalledOnValidThread());
   if (!sync_entry) {
     completion_callback.Run(net::ERR_FAILED);
     // If OpenEntry failed, we must remove it from our index.
-    if (index)
-      index->Remove(key);
+    index_->Remove(key_);
+    // The reference held by the Callback calling us will go out of scope and
+    // delete |this| on leaving this scope.
     return;
   }
-  if (index)
-    index->Insert(sync_entry->key());
-  *out_entry = new SimpleEntryImpl(sync_entry, index);
+  // Adding a reference to self will keep |this| alive after the scope of our
+  // Callback calling us is destroyed.
+  AddRef();  // Balanced in Close().
+  synchronous_entry_ = sync_entry;
+  SetSynchronousData();
+  index_->Insert(key_);
+  *out_entry = this;
   completion_callback.Run(net::OK);
 }
 
-// static
 void SimpleEntryImpl::EntryOperationComplete(
-    base::WeakPtr<SimpleIndex> index,
     const CompletionCallback& completion_callback,
-    base::WeakPtr<SimpleEntryImpl> entry,
-    SimpleSynchronousEntry* sync_entry,
     int result) {
-  DCHECK(sync_entry);
-  if (index) {
-    if (result >= 0)
-      index->UpdateEntrySize(sync_entry->key(), sync_entry->GetFileSize());
-    else
-      index->Remove(sync_entry->key());
-  }
-
-  if (entry) {
-    DCHECK(entry->synchronous_entry_in_use_by_worker_);
-    entry->synchronous_entry_in_use_by_worker_ = false;
-    entry->SetSynchronousData();
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
+  DCHECK(synchronous_entry_in_use_by_worker_);
+  synchronous_entry_in_use_by_worker_ = false;
+  SetSynchronousData();
+  if (result >= 0) {
+    index_->UpdateEntrySize(synchronous_entry_->key(),
+                            synchronous_entry_->GetFileSize());
   } else {
-    // |entry| must have had Close() called while this operation was in flight.
-    // Since the simple cache now only supports one pending entry operation in
-    // flight at a time, it's safe to now call Close() on |sync_entry|.
-    WorkerPool::PostTask(FROM_HERE,
-                         base::Bind(&SimpleSynchronousEntry::Close,
-                                    base::Unretained(sync_entry)),
-                         true);
+    index_->Remove(synchronous_entry_->key());
   }
   completion_callback.Run(result);
 }

net/disk_cache/simple/simple_entry_impl.h

@@ -8,12 +8,15 @@
 #include <string>
 
 #include "base/files/file_path.h"
-#include "base/memory/weak_ptr.h"
+#include "base/memory/ref_counted.h"
 #include "base/threading/thread_checker.h"
 #include "net/disk_cache/disk_cache.h"
 #include "net/disk_cache/simple/simple_entry_format.h"
 #include "net/disk_cache/simple/simple_index.h"
 
+namespace base {
+class MessageLoopProxy;
+}
 
 namespace net {
 class IOBuffer;
@@ -26,21 +29,23 @@ class SimpleSynchronousEntry;
 // SimpleEntryImpl is the IO thread interface to an entry in the very simple
 // disk cache. It proxies for the SimpleSynchronousEntry, which performs IO
 // on the worker thread.
-class SimpleEntryImpl : public Entry {
+class SimpleEntryImpl : public Entry,
+                        public base::RefCountedThreadSafe<SimpleEntryImpl> {
+  friend class base::RefCountedThreadSafe<SimpleEntryImpl>;
  public:
-  static int OpenEntry(base::WeakPtr<SimpleIndex> index,
+  static int OpenEntry(const scoped_refptr<SimpleIndex>& index,
                        const base::FilePath& path,
                        const std::string& key,
                        Entry** entry,
                        const CompletionCallback& callback);
 
-  static int CreateEntry(base::WeakPtr<SimpleIndex> index,
+  static int CreateEntry(const scoped_refptr<SimpleIndex>& index,
                          const base::FilePath& path,
                          const std::string& key,
                          Entry** entry,
                          const CompletionCallback& callback);
 
-  static int DoomEntry(base::WeakPtr<SimpleIndex> index,
+  static int DoomEntry(const scoped_refptr<SimpleIndex>& index,
                        const base::FilePath& path,
                        const std::string& key,
                        const CompletionCallback& callback);
@@ -80,34 +85,29 @@ class SimpleEntryImpl : public Entry {
   virtual int ReadyForSparseIO(const CompletionCallback& callback) OVERRIDE;
 
  private:
-  SimpleEntryImpl(SimpleSynchronousEntry* synchronous_entry,
-                  base::WeakPtr<SimpleIndex> index);
+  SimpleEntryImpl(const scoped_refptr<SimpleIndex>& index,
+                  const base::FilePath& path,
+                  const std::string& key);
 
   virtual ~SimpleEntryImpl();
 
   // Called after a SimpleSynchronousEntry has completed CreateEntry() or
-  // OpenEntry(). Constructs the new SimpleEntryImpl (if |result| is net::OK)
-  // and passes it back to the caller via |out_entry|. Also runs
+  // OpenEntry(). If |sync_entry| is non-NULL, creation is successful and we
+  // can return |this| SimpleEntryImpl to |*out_entry|. Runs
   // |completion_callback|.
-  static void CreationOperationComplete(
-      base::WeakPtr<SimpleIndex> index,
-      const CompletionCallback& completion_callback,
-      const std::string& key,
+  void CreationOperationComplete(
       Entry** out_entry,
+      const CompletionCallback& completion_callback,
       SimpleSynchronousEntry* sync_entry);
 
   // Called after a SimpleSynchronousEntry has completed an asynchronous IO
   // operation, such as ReadData() or WriteData(). Calls |completion_callback|.
-  // If |entry| no longer exists, then it ensures |sync_entry| is closed.
-  static void EntryOperationComplete(
-      base::WeakPtr<SimpleIndex> index,
+  void EntryOperationComplete(
       const CompletionCallback& completion_callback,
-      base::WeakPtr<SimpleEntryImpl> entry,
-      SimpleSynchronousEntry* sync_entry,
       int result);
 
-  // Called on construction and also after the completion of asynchronous IO to
-  // initialize the IO thread copies of data returned by synchronous accessor
+  // Called on initialization and also after the completion of asynchronous IO
+  // to initialize the IO thread copies of data returned by synchronous accessor
   // functions. Copies data from |synchronous_entry_| into |this|, so that
   // values can be returned during our next IO operation.
   void SetSynchronousData();
@@ -116,10 +116,8 @@ class SimpleEntryImpl : public Entry {
   // thread, in all cases. |io_thread_checker_| documents and enforces this.
   base::ThreadChecker io_thread_checker_;
 
-  base::WeakPtrFactory<SimpleEntryImpl> weak_ptr_factory_;
-
-  // |path_| and |key_| are copied from the synchronous entry on construction,
-  // and never updated as they are const.
+  const scoped_refptr<base::MessageLoopProxy> constructor_thread_;
+  const scoped_refptr<SimpleIndex> index_;
   const base::FilePath path_;
   const std::string key_;
 
@@ -140,8 +138,6 @@ class SimpleEntryImpl : public Entry {
   // and false after. Used to ensure thread safety by not allowing multiple
   // threads to access the |synchronous_entry_| simultaneously.
   bool synchronous_entry_in_use_by_worker_;
-
-  base::WeakPtr<SimpleIndex> index_;
 };
 
 }  // namespace disk_cache

net/disk_cache/simple/simple_index.cc

@@ -81,13 +81,12 @@ SimpleIndex::SimpleIndex(
 
 SimpleIndex::~SimpleIndex() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
-
 }
 
 void SimpleIndex::Initialize() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   IndexCompletionCallback merge_callback =
-      base::Bind(&SimpleIndex::MergeInitializingSet, AsWeakPtr());
+      base::Bind(&SimpleIndex::MergeInitializingSet, this);
   base::WorkerPool::PostTask(FROM_HERE,
                              base::Bind(&SimpleIndex::LoadFromDisk,
                                         index_filename_,

net/disk_cache/simple/simple_index.h

@@ -14,7 +14,7 @@
 #include "base/hash_tables.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "base/time.h"
 #include "net/base/net_export.h"
 
@@ -67,15 +67,14 @@ class NET_EXPORT_PRIVATE EntryMetadata {
 
 // This class is not Thread-safe.
 class NET_EXPORT_PRIVATE SimpleIndex
-    : public base::SupportsWeakPtr<SimpleIndex> {
+    : public base::RefCountedThreadSafe<SimpleIndex> {
+  friend class base::RefCountedThreadSafe<SimpleIndex>;
  public:
   SimpleIndex(
       const scoped_refptr<base::TaskRunner>& cache_thread,
       const scoped_refptr<base::TaskRunner>& io_thread,
       const base::FilePath& path);
 
-  virtual ~SimpleIndex();
-
   void Initialize();
 
   void Insert(const std::string& key);
@@ -105,6 +104,8 @@ class NET_EXPORT_PRIVATE SimpleIndex
  private:
   typedef base::Callback<void(scoped_ptr<EntrySet>)> IndexCompletionCallback;
 
+  virtual ~SimpleIndex();
+
   static void LoadFromDisk(
       const base::FilePath& index_filename,
       const scoped_refptr<base::TaskRunner>& io_thread,

net/disk_cache/simple/simple_synchronous_entry.cc

@@ -85,7 +85,7 @@ void SimpleSynchronousEntry::CreateEntry(
 void SimpleSynchronousEntry::DoomEntry(
     const FilePath& path,
     const std::string& key,
-    scoped_refptr<TaskRunner> callback_runner,
+    const scoped_refptr<TaskRunner>& callback_runner,
     const net::CompletionCallback& callback) {
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     FilePath to_delete = path.AppendASCII(GetFilenameFromKeyAndIndex(key, i));

net/disk_cache/simple/simple_synchronous_entry.h

@@ -60,7 +60,7 @@ class SimpleSynchronousEntry {
   // run by |callback_runner|.
   static void DoomEntry(const base::FilePath& path,
                         const std::string& key,
-                        scoped_refptr<base::TaskRunner> callback_runner,
+                        const scoped_refptr<base::TaskRunner>& callback_runner,
                         const net::CompletionCallback& callback);
 
   // N.B. Close(), ReadData() and WriteData() may block on IO.