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..32a22b44984766480bb5d432646b6aad47500116 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));

[gavinp, 2013/08/29 19:31:40]

There's two things about this that might be unnecessarily conservative.

1. Perhaps we should push it to the front of the operations queue, rather than
the back? Dooms should happen quickly.

2. It's actually safe to just doom completely asynchronously as long as the
state is not STATE_UNINITIALIZED. However, ttuttle's upcoming third-stream CL
will add STATE_IO_PENDING to that list when the IO is a write.

My thinking is we should do (1), and not (2). WDYT?

[Philippe, 2013/08/30 13:43:02]

IMO, it would be a little subtle/risky to re-order the operations in the queue,
wdyt? I would favor safety/simplicity here unless this proves to be really
problematic. To be honest I wouldn't even have thought about doing this :)

[gavinp, 2013/08/30 14:58:18]

You're right. This kind of subtle perf change doesn't belong here.

+  RunNextOperationIfNeeded();
   return net::ERR_IO_PENDING;
 }
 
@@ -579,6 +576,9 @@ void SimpleEntryImpl::RunNextOperationIfNeeded() {
                           operation->callback(),
                           operation->truncate());
         break;
+      case SimpleEntryOperation::TYPE_DOOM:
+        DoomEntryInternal(operation->callback());
+        break;
       default:
         NOTREACHED();
     }
@@ -605,7 +605,8 @@ void SimpleEntryImpl::OpenEntryInternal(bool have_index,
         net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_OPEN_END,
         CreateNetLogSimpleEntryCreationCallback(this, net::OK));
     return;
-  } else if (state_ == STATE_FAILURE) {
+  }
+  if (state_ == STATE_FAILURE) {
     if (!callback.is_null()) {
       MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
           callback, net::ERR_FAILED));
@@ -732,7 +733,6 @@ void SimpleEntryImpl::CloseInternal() {
       }
     }
   } else {
-    synchronous_entry_ = NULL;
     CloseOperationComplete();
   }
 }
@@ -890,6 +890,15 @@ void SimpleEntryImpl::WriteDataInternal(int stream_index,
   worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
 }
 
+void SimpleEntryImpl::DoomEntryInternal(const CompletionCallback& callback) {
+  PostTaskAndReplyWithResult(

[gavinp, 2013/08/29 19:31:40]

I think there's a bug here, although it's pre-existing. If this entry is already
doomed, we don't want to do this, right?

I guess that needs a test, too. Nice.

[Philippe, 2013/08/30 13:43:02]

That's your last CL, right?

[gavinp, 2013/08/30 14:58:18]

Yes.

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