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"
@@ skipping 24 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);
+    *entry = new_entry.get();

[rvargas (doing something else), 2013/04/15 22:07:27]

Although strictly not required by anything, it is better to avoid returning an
invalid object pointer in case of failure.

[gavinp, 2013/04/16 08:41:55]

Done.

     SynchronousCreationCallback sync_creation_callback =
         base::Bind(&SimpleEntryImpl::CreationOperationComplete,
-                   index, callback, key, entry);
+                   new_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,
                                  const FilePath& path,
                                  const std::string& key,
                                  Entry** entry,
                                  const CompletionCallback& callback) {
+  scoped_refptr<SimpleEntryImpl> new_entry =
+      new SimpleEntryImpl(index, path, key);
+  *entry = new_entry.get();
   SynchronousCreationCallback sync_creation_callback =
       base::Bind(&SimpleEntryImpl::CreationOperationComplete,
-                 index, callback, key, entry);
+                 new_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,
                                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;
+  DCHECK_EQ(this, self_.get());
+  bool in_use = synchronous_entry_in_use_by_worker_;
+  DCHECK((in_use && !HasOneRef()) || (!in_use && HasOneRef()));
+  self_ = NULL;
+  // 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.
+  DCHECK(!in_use || (in_use && HasOneRef()));

[rvargas (doing something else), 2013/04/15 22:07:27]

if (in_use)
  DCHECK();

[gavinp, 2013/04/16 08:41:55]

Done.

 }
 
 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,
     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_);
     return;
   }
-  if (index)
-    index->Insert(sync_entry->key());
-  *out_entry = new SimpleEntryImpl(sync_entry, index);
+  Initialize(sync_entry);

[rvargas (doing something else), 2013/04/15 22:07:27]

Why do you need a self_ reference? It is created at this point, and released at
Close time, so it is active throughout the whole lifetime of the object.

Is it just so that you don't addref/release this explicitly? In that case,
addref/release is better because it is explicit :) (and simpler).

[gavinp, 2013/04/16 08:41:55]

At least self_ advertises that it doesn't leak references.

We'll probably need to switch to the fragile system eventually anyway, and now
is as good as anytime.

Done.

+  if (index_)
+    index_->Insert(key_);
   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::Initialize(
+    SimpleSynchronousEntry* sync_entry) {
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  self_ = this;
+  synchronous_entry_ = sync_entry;
+  SetSynchronousData();
+}
+
 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