Make SimpleEntryImpl::Close asynchronous.

net/disk_cache/simple/simple_entry_impl.h

Index: net/disk_cache/simple/simple_entry_impl.h
diff --git a/net/disk_cache/simple/simple_entry_impl.h b/net/disk_cache/simple/simple_entry_impl.h
index 4dc76ddddd803c45106a0081f1d2c2e79535daaa..82d7bbbdcf8e99588799b9f8f33d4c77f2f8561d 100644
--- a/net/disk_cache/simple/simple_entry_impl.h
+++ b/net/disk_cache/simple/simple_entry_impl.h
@@ -89,9 +89,11 @@ class SimpleEntryImpl : public Entry {
 
   // Called after a SimpleSynchronousEntry has completed an asynchronous IO
   // operation, such as ReadData() or WriteData(). Calls |completion_callback|.
+  // If |entry| no longer exists, then it ensures |sync_entry| is closed.
   static void EntryOperationComplete(
       const CompletionCallback& completion_callback,
       base::WeakPtr<SimpleEntryImpl> entry,
+      SimpleSynchronousEntry* sync_entry,
       int result);
 
   // Called on construction and also after the completion of asynchronous IO to
@@ -114,7 +116,11 @@ class SimpleEntryImpl : public Entry {
   int32 data_size_[kSimpleEntryFileCount];
 
   // The |synchronous_entry_| is the worker thread object that performs IO on
-  // entries.
+  // entries. It's owned by this SimpleEntryImpl, which is responsible for its
+  // destruction by calling SimpleSynchronousEntry::Close(). However, since
+  // |this| can be destroyed while a synchronous operation is completing, the
+  // |synchronous_entry_| must sometimes be destroyed when an operation
+  // completes and finds its owning SimpleEntryImpl has already been destroyed.

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

So the way I read the code / another way to phrase this is that the
|synchonous_entry_| ownership is held by the SimpleEntryImpl, but it sometimes
passed to the worker thread and then passed back through EntryOperationComplete.
 The bool synchronous_entry_in_use_by_worker_ indicates when the ownership is
out of class, and in those situations the class avoids using synchronous_entry_.
 Is that right?  

If it is, would it make sense to signal it with a scoped_ptr<>, and pass that
scoped_ptr<> around explicitly so it's easier for code readers to follow the
bouncing ownership ball?  I'm not dead set on that change, and I'm aware I
sometimes go overboard in my use of scoped_ptr<>, but it seems a simpler way to
represent what you're trying to do than to have a separate bool.

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

I like the way you wrote that ownership, so I've fixed the comment to reflect
that. Thanks.

As for the ownership issues, it's interesting you brought this up. I think
there's going to be a need to move to something more like this in the future,
but I'm not sure. I am not comfortable using scoped_ptr<> here, because we never
call the destructor on |synchronous_entry_|, since it's private. We just call
SimpleSynchronousEntry::Close(), and we do that on a worker (since it does
blocking filesystem operations).

For context, I didn't use scoped_ptr<> earlier, but I did NULL this pointer out
to show a lack of ownership in earlier revisions of the simple backend. As well,
I made the pending operations themselves objects. See
https://codereview.chromium.org/12192005/diff2/21009:26024/net/disk_cache/sim...
for the change in which I removed that, and moved to a more C-style usage. Since
I basically was close to this scoped_ptr<> idea, and my reviewers talked me away
from it, I'm not going to switch to the scoped_ptrs<> right now in response to
your comment.

However, when we add support for multiple overlapping read operations, things
get trickier. I wonder how much data will need to be tracked for that to work
properly.

   SimpleSynchronousEntry* synchronous_entry_;
 
   // Set to true when a worker operation is posted on the |synchronous_entry_|,