Make SimpleEntryImpl::Close asynchronous.

net/disk_cache/simple/simple_entry_impl.cc

Index: net/disk_cache/simple/simple_entry_impl.cc
diff --git a/net/disk_cache/simple/simple_entry_impl.cc b/net/disk_cache/simple/simple_entry_impl.cc
index 68f65dc12f8e1c5e4bf4bac22270844bc943dbb6..6d53d3f7e8db07ba3b9a5f319bd50a06b17d502a 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -86,17 +86,12 @@ void SimpleEntryImpl::Doom() {
 }
 
 void SimpleEntryImpl::Close() {
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    delete this;
-    return;
+  if (!synchronous_entry_in_use_by_worker_) {
+    WorkerPool::PostTask(FROM_HERE,
+                         base::Bind(&SimpleSynchronousEntry::Close,
+                                    base::Unretained(synchronous_entry_)),
+                         true);
   }
-  DCHECK(synchronous_entry_);

[Randy Smith (Not in Mondays), 2013/02/26 18:47:58]

I don't see how this change might result in synchronous_entry_ being null at the
time of a SimpleEntryImpl::Close(); it seems like you are arranging for
synchronous_entry_ to potentially outlive, SimpleEntryImpl, not be destroyed
before it.

[gavinp, 2013/02/26 21:00:34]

You're right. This was cruft that got mismerged when an upstream review removed
this DCHECK. Thank you.

[Randy Smith (Not in Mondays), 2013/02/26 21:13:11]

Still a little confused--the last patch set still doesn't have the DCHECK. 
Should it?

[gavinp, 2013/02/27 21:50:01]

Aha, I was also confused. In response to this comment, I removed the DCHECK in
SimpleEntryImpl::~SimpleEntryImpl. The DCHECK that was here in this CL in
earlier revisions isn't needed any more: rebases from upstream changes made it
redundant.

See my reply to your comment about scoped_ptr<> for more on this topic.

-  WorkerPool::PostTask(FROM_HERE,
-                       base::Bind(&SimpleSynchronousEntry::Close,
-                                  base::Unretained(synchronous_entry_)),
-                       true);
-  synchronous_entry_ = NULL;
   // Entry::Close() is expected to release this entry. See disk_cache.h for
   // details.
   delete this;
@@ -125,7 +120,8 @@ int SimpleEntryImpl::ReadData(int index,
                               const CompletionCallback& callback) {
   // 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.
+  // 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);
@@ -133,7 +129,7 @@ int SimpleEntryImpl::ReadData(int index,
   synchronous_entry_in_use_by_worker_ = true;
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
-                 callback, weak_ptr_factory_.GetWeakPtr());
+                 callback, weak_ptr_factory_.GetWeakPtr(), synchronous_entry_);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::ReadData,
                                   base::Unretained(synchronous_entry_),
@@ -156,7 +152,7 @@ int SimpleEntryImpl::WriteData(int index,
   synchronous_entry_in_use_by_worker_ = true;
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
-                 callback, weak_ptr_factory_.GetWeakPtr());
+                 callback, weak_ptr_factory_.GetWeakPtr(), synchronous_entry_);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::WriteData,
                                   base::Unretained(synchronous_entry_),
@@ -241,11 +237,20 @@ void SimpleEntryImpl::CreationOperationComplete(
 void SimpleEntryImpl::EntryOperationComplete(
     const CompletionCallback& completion_callback,
     base::WeakPtr<SimpleEntryImpl> entry,
+    SimpleSynchronousEntry* sync_entry,
     int result) {
   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)),

[Randy Smith (Not in Mondays), 2013/02/26 18:47:58]

I would feel better about this change if the ownership model for sync_entry was
documented either directly or by references somewhere in this class (probably
the right place is simple_entry_impl.h:synchronous-entry).  Despite the fact of
it having perfectly valid uses, base::Unretained() makes me twitch, and so I
want to make it as easily as possible to verify that the uses are reasonable
ones. 

[gavinp, 2013/02/26 21:00:34]

Done.

+                         true);
   }
   completion_callback.Run(result);
 }