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..3bb42b6ee7cabe5c5c51371719f4f4ec2369d838 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_);
-  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 at present only one pending entry operation can be in flight at a

[rvargas (doing something else), 2013/02/15 23:00:23]

Why write this CL at all instead of fixing the issue once and for all? (as in we
still have to support multiple concurrent operation)

[gavinp, 2013/02/25 23:02:28]

It's true, we do absolutely have to support multiple concurrent reads.

I'd like us to be free of memory leaks before we enter that state, so we can
test that this invariant stays true both before and after. This patch fixes a
memory leak; that's an improvement, even if it doesn't finish the simple cache,
and even if it relies on an invariant that will change soon.

There's three of us working on this code (and possibly four soon). Removing
obvious problems feels like progress.

+    // time, it's safe to now call Close() on |sync|entry|.
+    WorkerPool::PostTask(FROM_HERE,
+                         base::Bind(&SimpleSynchronousEntry::Close,
+                                    base::Unretained(sync_entry)),
+                         true);
   }
   completion_callback.Run(result);
 }