Make SimpleEntryImpl ref counted.

net/disk_cache/simple/simple_entry_impl.cc

 // Copyright (c) 2013 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 "net/disk_cache/simple/simple_entry_impl.h"
 
 #include "base/bind.h"
 #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"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/simple/simple_index.h"
 #include "net/disk_cache/simple/simple_synchronous_entry.h"
 
 namespace {
 
 typedef disk_cache::Entry::CompletionCallback CompletionCallback;
 typedef disk_cache::SimpleSynchronousEntry::SynchronousCreationCallback
     SynchronousCreationCallback;
 typedef disk_cache::SimpleSynchronousEntry::SynchronousOperationCallback
     SynchronousOperationCallback;
 
 }  // namespace
 
 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,
                                const CompletionCallback& callback) {
   // 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(),
                                     sync_creation_callback),
                          true);
     return net::ERR_IO_PENDING;
   }
   return net::ERR_FAILED;
 }
 
 // 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(),
                                   sync_creation_callback),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 // 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);
+  index->Remove(key);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::DoomEntry, path, key,
                                   MessageLoopProxy::current(), callback),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 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
   // SimpleSynchronousEntry, and operations can even happen on them. The files
   // will be removed from the filesystem when they are closed.
   DoomEntry(index_, path_, key_, CompletionCallback());
 #else
   NOTIMPLEMENTED();
 #endif
 }
 
 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;
+  bool in_use = synchronous_entry_in_use_by_worker_;

[felipeg, 2013/04/17 11:48:25]

can you add a comment explaining why you need to save this member variable in a
local variable before calling Release() ?

+  // These HasOneRef DCHECKs may cause flakiness since extra refs do exist
+  // ephemerally during copies etc..., if they are on balance causing pain
+  // we should consider removing them.
+  DCHECK((in_use && !HasOneRef()) || (!in_use && HasOneRef()));
+  Release();  // Balanced in CreationOperationCompleted().
+  // At most one worker can be operating on our |synchronous_entry_| at a time,
+  // and so if we were in use when we cleared |self_|, we should expect a single
+  // reference from that operation now.
+  if (in_use)
+    DCHECK(HasOneRef());
 }
 
 std::string SimpleEntryImpl::GetKey() const {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   return key_;
 }
 
 Time SimpleEntryImpl::GetLastUsed() const {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   return last_used_;
@@ skipping 17 matching lines @@
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // TODO(gavinp): Add support for overlapping reads. The net::HttpCache does
   // make overlapping read requests when multiple transactions access the same
   // entry as read only. This might make calling SimpleSynchronousEntry::Close()
   // correctly more tricky (see SimpleEntryImpl::EntryOperationComplete).
   if (synchronous_entry_in_use_by_worker_) {
     NOTIMPLEMENTED();
     CHECK(false);
   }
   synchronous_entry_in_use_by_worker_ = true;
-  if (index_)
-    index_->UseIfExists(key_);
+  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_),
                                   index, offset, make_scoped_refptr(buf),
                                   buf_len, sync_operation_callback),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 int SimpleEntryImpl::WriteData(int index,
                                int offset,
                                net::IOBuffer* buf,
                                int buf_len,
                                const CompletionCallback& callback,
                                bool truncate) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   if (synchronous_entry_in_use_by_worker_) {
     NOTIMPLEMENTED();
     CHECK(false);
   }
   synchronous_entry_in_use_by_worker_ = true;
-  if (index_)
-    index_->UseIfExists(key_);
+  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_),
                                   index, offset, make_scoped_refptr(buf),
                                   buf_len, sync_operation_callback, truncate),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 int SimpleEntryImpl::ReadSparseData(int64 offset,
@@ skipping 38 matching lines @@
   NOTIMPLEMENTED();
 }
 
 int SimpleEntryImpl::ReadyForSparseIO(const CompletionCallback& callback) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // TODO(gavinp): Determine if the simple backend should support sparse data.
   NOTIMPLEMENTED();
   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)
+    : 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_) {
+    WorkerPool::PostTask(FROM_HERE,

[gavinp, 2013/04/17 11:02:21]

Wait, worker pool can't post to worker pool.

[Philippe, 2013/04/17 12:04:46]

I don't think you will be able to use a RefCountedThreadSafeTrait actually to
specify which thread the ref-counted object needs to be deleted on. The trait is
used by RefCountedThreadSafe through its static methods which doesn't allow you
to inject the IO thread message loop pointer.

You might be able to do something as in:

SimpleEntryImpl::~SimpleEntryImpl() {
  if (io_thread_->BelongsToCurrentThread()) {
    CloseSynchronousEntry(synchronous_entry_);  // Posts the task to the worker
pool.
  } else {
    io_thread_->PostTask(
        FROM_HERE,
        base::Bind(&SimpleEntryImpl::CloseSynchronousEntry,
                   base::Unretained(synchronous_entry_)));
  }
}

Note that CloseSynchronousEntry() would be a static method.

+                         base::Bind(&SimpleSynchronousEntry::Close,
+                                    base::Unretained(synchronous_entry_)),
+                         true);
+  }
 }
 
-// static
 void SimpleEntryImpl::CreationOperationComplete(
-    WeakPtr<SimpleIndex> index,
+    Entry** out_entry,
     const CompletionCallback& completion_callback,
-    const std::string& key,
-    Entry** out_entry,
     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);
 }
 
 void SimpleEntryImpl::SetSynchronousData() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK(!synchronous_entry_in_use_by_worker_);
   // TODO(felipeg): These copies to avoid data races are not optimal. While
   // adding an IO thread index (for fast misses etc...), we can store this data
   // in that structure. This also solves problems with last_used() on ext4
   // filesystems not being accurate.
   last_used_ = synchronous_entry_->last_used();
   last_modified_ = synchronous_entry_->last_modified();
   for (int i = 0; i < kSimpleEntryFileCount; ++i)
     data_size_[i] = synchronous_entry_->data_size(i);
 }
 
 }  // namespace disk_cache