Fix race condition for non-open/create operations happening after a doom.

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 72d6d4c7e104a03db5499460443afac22d7d20f9..c6b9ef5cf7edb22e8552adb3e152b713196675cd 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -16,6 +16,7 @@
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/metrics/histogram.h"
 #include "base/task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -257,12 +258,8 @@ int SimpleEntryImpl::DoomEntry(const CompletionCallback& callback) {
   net_log_.AddEvent(net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_DOOM_BEGIN);
 
   MarkAsDoomed();
-  scoped_ptr<int> result(new int());
-  Closure task = base::Bind(&SimpleSynchronousEntry::DoomEntry, path_, key_,
-                            entry_hash_, result.get());
-  Closure reply = base::Bind(&CallCompletionCallback,
-                             callback, base::Passed(&result));
-  worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
+  pending_operations_.push(SimpleEntryOperation::DoomOperation(this, callback));
+  RunNextOperationIfNeeded();
   return net::ERR_IO_PENDING;
 }
 
@@ -537,7 +534,6 @@ void SimpleEntryImpl::MarkAsDoomed() {
   if (!backend_.get())
     return;
   backend_->index()->Remove(entry_hash_);
-  RemoveSelfFromBackend();

[gavinp, 2013/08/26 20:36:50]

There's more to it than this.

This was the only callsite other than the destructor for this method; so that
means that backend_.get() can never be false; we always have one.

But backend_.get() was being used as a proxy for "we're doomed", so things like
the use time weren't being updated in the index (look in ReadDataInternal).

So we need some other solution for that.

Did this pass unit tests on doom? If so, we need a unit test that doomed entries
don't update the index!

[Philippe, 2013/08/27 11:33:30]

To be honest I'm a bit confused with this MarkAsDoomed() method and the fact
that it's re-used in some error code paths in contexts other than dooming. It
makes it hard to reason about it and to predict its effects.

I'm not sure what you meant by doomed entries should not update the index. Are
we not removing them from the index on the line above? I will need you to
elaborate :)

[gavinp, 2013/08/27 15:48:02]

There's a member backend_ on SimpleEntryImpl, it's a weakptr. It's zeroed in
RemoveSelfFromBackend(). That previously was taken as a signal that the entry
was doomed, so that ReadData() for instance wouldn't update the index (really,
it couldn't, since the index is accessed through backend_->index()).

But now, we're not using backend_ as a weakptr with this change; the only place
the weakptr is cleared is in SimpleEntryImpl::~SimpleEntryImpl, so it's really
just a naked ptr (except that a entry can outlive its backend object, so it
probably still needs to be a weakptr).

So both ReadData and writedata have:

if (backend.get())
  backend_->index()->UseIfExists(entry_hash_);

So now, with your change, if we:

Create "foo" (entry1)
entry1->Doom();
Create "foo" (entry2)
entry1->WriteData();  // This should not update the use time in the index.

[Philippe, 2013/08/27 16:11:21]

I see now, thanks :) I hadn't understood that the validity of the backend
pointer was used to tell whether the entry was doomed (although it's pretty
clear now when I read you back).

So I intend to add a new DOOM entry state. I would have really appreciated
avoiding that but I don't see any other approach, do you?

 }
 
 void SimpleEntryImpl::RunNextOperationIfNeeded() {
@@ -579,6 +575,9 @@ void SimpleEntryImpl::RunNextOperationIfNeeded() {
                           operation->callback(),
                           operation->truncate());
         break;
+      case SimpleEntryOperation::TYPE_DOOM:
+        DoomEntryInternal(operation->callback());
+        break;
       default:
         NOTREACHED();
     }
@@ -890,6 +889,15 @@ void SimpleEntryImpl::WriteDataInternal(int stream_index,
   worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
 }
 
+void SimpleEntryImpl::DoomEntryInternal(const CompletionCallback& callback) {
+  PostTaskAndReplyWithResult(

[gavinp, 2013/08/26 20:36:50]

Every one of the other calls has a kind of manual expansion of
PostTaskAndReplyWithResult, in part I guess because we're handling two separate
return values, though not always.

I wonder if we should switch every caller to use PostTaskAndReplyWithResult,
switch this to be really similar to the others, or leave them a bit
inconsistent? I don't have a strong opinion, but I'd like to know what drew you
to using this util, rather than cutting and pasting the code from DoomEntry()?

[Philippe, 2013/08/27 11:33:30]

In my opinion it would be nice to switch to PostTaskAndReplyWithResult() for
single-valued return types. It's not high priority though that's why I think we
can do the transition incrementally when touching code around. In my opinion,
this helper function is more expressive, self-documenting and less hackish. It
also makes the bound functions' signature much nicer (by using a return type
rather than an output parameter) and also saves a heap allocation. It's also
less code to write. What do you think?

+      worker_pool_, FROM_HERE,
+      base::Bind(&SimpleSynchronousEntry::DoomEntry, path_, key_, entry_hash_),
+      base::Bind(&SimpleEntryImpl::DoomOperationComplete, this, state_,
+                 callback));
+  state_ = STATE_IO_PENDING;
+}
+
 void SimpleEntryImpl::CreationOperationComplete(
     const CompletionCallback& completion_callback,
     const base::TimeTicks& start_time,
@@ -1060,6 +1068,15 @@ void SimpleEntryImpl::WriteOperationComplete(
       stream_index, completion_callback, *entry_stat, result.Pass());
 }
 
+void SimpleEntryImpl::DoomOperationComplete(State state_to_restore,
+                                            const CompletionCallback& callback,
+                                            int result) {
+  state_ = state_to_restore;
+  if (!callback.is_null())
+    callback.Run(result);
+  RunNextOperationIfNeeded();
+}
+
 void SimpleEntryImpl::ChecksumOperationComplete(
     int orig_result,
     int stream_index,