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

net/disk_cache/simple/simple_entry_operation.cc

Index: net/disk_cache/simple/simple_entry_operation.cc
diff --git a/net/disk_cache/simple/simple_entry_operation.cc b/net/disk_cache/simple/simple_entry_operation.cc
index 81d5f7c888b1d020bfa23d601e89dae0e4aa8f56..97d1fa1ad89cc60ee4ad53f33b8ef198bb38a68f 100644
--- a/net/disk_cache/simple/simple_entry_operation.cc
+++ b/net/disk_cache/simple/simple_entry_operation.cc
@@ -28,7 +28,7 @@ SimpleEntryOperation::SimpleEntryOperation(const SimpleEntryOperation& other)
 
 SimpleEntryOperation::~SimpleEntryOperation() {}
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::OpenOperation(
     SimpleEntryImpl* entry,
     bool have_index,
@@ -48,7 +48,7 @@ SimpleEntryOperation SimpleEntryOperation::OpenOperation(
                               false);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::CreateOperation(
     SimpleEntryImpl* entry,
     bool have_index,
@@ -68,7 +68,7 @@ SimpleEntryOperation SimpleEntryOperation::CreateOperation(
                               false);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::CloseOperation(
     SimpleEntryImpl* entry) {
   return SimpleEntryOperation(entry,
@@ -85,7 +85,7 @@ SimpleEntryOperation SimpleEntryOperation::CloseOperation(
                               false);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::ReadOperation(
     SimpleEntryImpl* entry,
     int index,
@@ -108,7 +108,7 @@ SimpleEntryOperation SimpleEntryOperation::ReadOperation(
                               alone_in_queue);
 }
 
-// Static.
+// static
 SimpleEntryOperation SimpleEntryOperation::WriteOperation(
     SimpleEntryImpl* entry,
     int index,
@@ -132,6 +132,23 @@ SimpleEntryOperation SimpleEntryOperation::WriteOperation(
                               false);
 }
 

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

// static

And you were so good everywhere else!

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

Ahaha :)

+SimpleEntryOperation SimpleEntryOperation::DoomOperation(
+    SimpleEntryImpl* entry,
+    const CompletionCallback& callback) {
+  return SimpleEntryOperation(entry,
+                              NULL,
+                              callback,
+                              NULL,
+                              0,
+                              0,
+                              TYPE_DOOM,
+                              false,

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

All these booleans make me dizzy. Can you make a const bool with the right name
for each of them, and use it in the call? I prefer that slightly to /*
nameofbool */. I wonder what the right thing to do about all these bools is
though.

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

Yeah I was also pretty sad to have to add this code especially for some state
that we don't use (and I don't like to pay for what I don't use :)). And I'm
even more sad now with this const variables boilerplate code.

This made me think that an alternative design for SimpleEntryOperation (maybe
less efficient though due to the extra vptr and heap allocation) would be to
have a SimpleEntryOperation abstract base class with the state (e.g. |entry_|,
|callback_|) needed by all the subclasses (the subclasses being CreateOperation,
OpenOperation...). The fact that we would have to heap-allocate the object would
not be a big regression though since we are already doing that to keep track of
the current operation in |executing_operation_|. Then we would have a queue of
pointers rather than a queue of values. There would be an extra indirection
(completely negligible in my opinion) to access the object but there would be
less copy on the other hand when the underlying vector is resized.
Then there is also the question of how we operate on the concrete type when we
are provided with an abstract entry operation. I see two options, my favorite
being the second one:
- keep the type enumeration and provide a cast template method.
- add a visitor interface that simple entry or possibly a private nested
delegate class would implement. This would also let us get rid of the operation
dispatching code that we have in simple_entry_impl.cc.

I would be glad to experiment with this interesting refactoring once we have
less high priority tasks (if this ever happens :))

+                              0,
+                              false,
+                              false,
+                              false);
+}
+
 bool SimpleEntryOperation::ConflictsWith(
     const SimpleEntryOperation& other_op) const {
   if (type_ != TYPE_READ && type_ != TYPE_WRITE)