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"

[Philippe, 2013/04/17 10:19:22]

Nit: "base/logging.h" is missing.

[gavinp, 2013/04/17 10:56:28]

Done.

 #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;
@@ skipping 14 matching lines @@
 
 // static
 int SimpleEntryImpl::OpenEntry(WeakPtr<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,

[digit, 2013/04/16 10:21:32]

This callback will be destroyed in the WorkerPool thread after its Run() method
is called (see WorkerThread::ThreadMain() in
base/threading/worker_pool_posix.cc)

This will end up destroying the callback created on line 47, which will end up
derefencing 'new_entry' in the worker pool thread.

As such, it looks like this class needs to be RefCountedThreadSafe.

[gavinp, 2013/04/17 09:52:46]

Great catch.

For moving forward, I went with RefCountedThread safe, as that's the fastest
path to correctness. In practice, we should probably be passing opaque handles
to the entry into the worker pool, as it would be faster (and lock free).

[felipeg, 2013/04/19 18:12:08]

I think this is not true
sync_creation_callback on line 47 will run on io_thread and  the pointer to the
SimpleEntryImpl object must be alive when it calls the
CreationOperationComplete.
So the SimpleEntryImpl object will be destroyed in the io_thread, AFAICT

                                     key, MessageLoopProxy::current(),
                                     sync_creation_callback),
                          true);
     return net::ERR_IO_PENDING;
   }
   return net::ERR_FAILED;
 }
 
 // static
 int SimpleEntryImpl::CreateEntry(WeakPtr<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,

[digit, 2013/04/16 10:21:32]

exact same issue here.

                                   key, MessageLoopProxy::current(),
                                   sync_creation_callback),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 // static
 int SimpleEntryImpl::DoomEntry(WeakPtr<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,
                                   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_;
+  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 20 matching lines @@
   // 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;
   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;
   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(WeakPtr<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,
+                         base::Bind(&SimpleSynchronousEntry::Close,
+                                    base::Unretained(synchronous_entry_)),
+                         true);
+  }
 }
 
-// static
 void SimpleEntryImpl::CreationOperationComplete(
-    WeakPtr<SimpleIndex> index,
+    Entry** 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);
+    if (index_)
+      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();
+  if (index_)
+    index_->Insert(key_);
+  *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();
-  } 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);
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
+  DCHECK(synchronous_entry_in_use_by_worker_);
+  synchronous_entry_in_use_by_worker_ = false;
+  SetSynchronousData();
+  if (index_) {
+    if (result >= 0) {
+      index_->UpdateEntrySize(synchronous_entry_->key(),
+                              synchronous_entry_->GetFileSize());
+    } else {
+      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