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

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

Doom used not to be treated as a regular operation in the sense that it didn't
use the pending operations queue. Therefore it was possible to kick off a doom
operation on an entry while another one was happening.

Review URL: https://chromiumcodereview.appspot.com/22859060

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@222014 0039d316-1c4b-4281-b951-d872f2087c98

net/disk_cache/entry_unittest.cc

@@ -2874,9 +2874,7 @@ TEST_F(DiskCacheEntryTest, SimpleCacheOptimistic4) {
   entry2->Close();
 }
 
-// This test is flaky because of the race of Create followed by a Doom.
-// See test SimpleCacheCreateDoomRace.
-TEST_F(DiskCacheEntryTest, DISABLED_SimpleCacheOptimistic5) {
+TEST_F(DiskCacheEntryTest, SimpleCacheOptimistic5) {
   // Test sequence:
   // Create, Doom, Write, Read, Close.
   SetSimpleCacheMode();
@@ -2945,10 +2943,6 @@ TEST_F(DiskCacheEntryTest, SimpleCacheOptimistic6) {
   EXPECT_EQ(0, memcmp(buffer1->data(), buffer1_read->data(), kSize1));
 
   entry->Doom();
-
-  // Check that we are not leaking.
-  EXPECT_TRUE(
-      static_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef());
 }
 
 // Confirm that IO buffers are not referenced by the Simple Cache after a write
@@ -2987,7 +2981,7 @@ TEST_F(DiskCacheEntryTest, SimpleCacheOptimisticWriteReleases) {
   EXPECT_TRUE(buffer1->HasOneRef());
 }
 
-TEST_F(DiskCacheEntryTest, DISABLED_SimpleCacheCreateDoomRace) {
+TEST_F(DiskCacheEntryTest, SimpleCacheCreateDoomRace) {
   // Test sequence:
   // Create, Doom, Write, Close, Check files are not on disk anymore.
   SetSimpleCacheMode();
@@ -3008,17 +3002,10 @@ TEST_F(DiskCacheEntryTest, DISABLED_SimpleCacheCreateDoomRace) {
   cache_->DoomEntry(key, cb.callback());
   EXPECT_EQ(net::OK, cb.GetResult(net::ERR_IO_PENDING));
 
-  // Lets do a Write so we block until all operations are done, so we can check
-  // the HasOneRef() below. This call can't be optimistic and we are checking
-  // that here.
   EXPECT_EQ(
-      net::ERR_IO_PENDING,
+      kSize1,
       entry->WriteData(0, 0, buffer1.get(), kSize1, cb.callback(), false));
-  EXPECT_EQ(kSize1, cb.GetResult(net::ERR_IO_PENDING));
 
-  // Check that we are not leaking.
-  EXPECT_TRUE(
-      static_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef());
   entry->Close();
 
   // Finish running the pending tasks so that we fully complete the close

net/disk_cache/simple/simple_entry_impl.cc

@@ -15,6 +15,7 @@
 #include "base/logging.h"
 #include "base/message_loop/message_loop_proxy.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"
@@ -264,12 +265,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;
 }
 
@@ -587,6 +584,9 @@ void SimpleEntryImpl::RunNextOperationIfNeeded() {
                           operation->callback(),
                           operation->truncate());
         break;
+      case SimpleEntryOperation::TYPE_DOOM:
+        DoomEntryInternal(operation->callback());
+        break;
       default:
         NOTREACHED();
     }
@@ -613,7 +613,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));
@@ -743,7 +744,6 @@ void SimpleEntryImpl::CloseInternal() {
       }
     }
   } else {
-    synchronous_entry_ = NULL;
     CloseOperationComplete();
   }
 }
@@ -901,6 +901,15 @@ void SimpleEntryImpl::WriteDataInternal(int stream_index,
   worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
 }
 
+void SimpleEntryImpl::DoomEntryInternal(const CompletionCallback& callback) {
+  PostTaskAndReplyWithResult(
+      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,
@@ -1073,6 +1082,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,

net/disk_cache/simple/simple_entry_impl.h

@@ -179,6 +179,8 @@ class SimpleEntryImpl : public Entry, public base::RefCounted<SimpleEntryImpl>,
                          const CompletionCallback& callback,
                          bool truncate);
 
+  void DoomEntryInternal(const CompletionCallback& callback);
+
   // Called after a SimpleSynchronousEntry has completed CreateEntry() or
   // OpenEntry(). If |in_sync_entry| is non-NULL, creation is successful and we
   // can return |this| SimpleEntryImpl to |*out_entry|. Runs
@@ -216,6 +218,11 @@ class SimpleEntryImpl : public Entry, public base::RefCounted<SimpleEntryImpl>,
                               scoped_ptr<SimpleEntryStat> entry_stat,
                               scoped_ptr<int> result);
 
+  // Called after an asynchronous doom completes.
+  void DoomOperationComplete(const CompletionCallback& callback,
+                             State state_to_restore,
+                             int result);
+
   // Called after validating the checksums on an entry. Passes through the
   // original result if successful, propogates the error if the checksum does
   // not validate.
@@ -277,8 +284,11 @@ class SimpleEntryImpl : public Entry, public base::RefCounted<SimpleEntryImpl>,
   CheckCrcResult crc_check_state_[kSimpleEntryFileCount];
 
   // The |synchronous_entry_| is the worker thread object that performs IO on
-  // entries. It's owned by this SimpleEntryImpl whenever |operation_running_|
-  // is false (i.e. when an operation is not pending on the worker pool).
+  // entries. It's owned by this SimpleEntryImpl whenever |executing_operation_|
+  // is false (i.e. when an operation is not pending on the worker pool). When
+  // an operation is being executed no one owns the synchronous entry. Therefore
+  // SimpleEntryImpl should not be deleted while an operation is running as that
+  // would leak the SimpleSynchronousEntry.
   SimpleSynchronousEntry* synchronous_entry_;
 
   std::queue<SimpleEntryOperation> pending_operations_;

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,33 @@ SimpleEntryOperation SimpleEntryOperation::WriteOperation(
                               false);
 }
 
+// static
+SimpleEntryOperation SimpleEntryOperation::DoomOperation(
+    SimpleEntryImpl* entry,
+    const CompletionCallback& callback) {
+  net::IOBuffer* const buf = NULL;
+  Entry** const out_entry = NULL;
+  const int offset = 0;
+  const int length = 0;
+  const bool have_index = false;
+  const int index = 0;
+  const bool truncate = false;
+  const bool optimistic = false;
+  const bool alone_in_queue = false;
+  return SimpleEntryOperation(entry,
+                              buf,
+                              callback,
+                              out_entry,
+                              offset,
+                              length,
+                              TYPE_DOOM,
+                              have_index,
+                              index,
+                              truncate,
+                              optimistic,
+                              alone_in_queue);
+}
+
 bool SimpleEntryOperation::ConflictsWith(
     const SimpleEntryOperation& other_op) const {
   if (type_ != TYPE_READ && type_ != TYPE_WRITE)

net/disk_cache/simple/simple_entry_operation.h

@@ -31,6 +31,7 @@ class SimpleEntryOperation {
     TYPE_CLOSE = 2,
     TYPE_READ = 3,
     TYPE_WRITE = 4,
+    TYPE_DOOM = 5,
   };
 
   SimpleEntryOperation(const SimpleEntryOperation& other);
@@ -63,6 +64,10 @@ class SimpleEntryOperation {
       bool optimistic,
       const CompletionCallback& callback);
 
+  static SimpleEntryOperation DoomOperation(
+      SimpleEntryImpl* entry,
+      const CompletionCallback& callback);
+
   bool ConflictsWith(const SimpleEntryOperation& other_op) const;
   // Releases all references. After calling this operation, SimpleEntryOperation
   // will only hold POD members.

net/disk_cache/simple/simple_synchronous_entry.cc

@@ -257,14 +257,13 @@ bool SimpleSynchronousEntry::DeleteFilesForEntryHash(
 }
 
 // static
-void SimpleSynchronousEntry::DoomEntry(
+int SimpleSynchronousEntry::DoomEntry(
     const FilePath& path,
     const std::string& key,
-    uint64 entry_hash,
-    int* out_result) {
+    uint64 entry_hash) {
   DCHECK_EQ(entry_hash, GetEntryHashKey(key));
-  bool deleted_well = DeleteFilesForEntryHash(path, entry_hash);
-  *out_result = deleted_well ? net::OK : net::ERR_FAILED;
+  const bool deleted_well = DeleteFilesForEntryHash(path, entry_hash);
+  return deleted_well ? net::OK : net::ERR_FAILED;
 }
 
 // static

net/disk_cache/simple/simple_synchronous_entry.h

@@ -85,14 +85,12 @@ class SimpleSynchronousEntry {
                           bool had_index,
                           SimpleEntryCreationResults* out_results);
 
-  // Deletes an entry without first Opening it. Does not check if there is
-  // already an Entry object in memory holding the open files. Be careful! This
-  // is meant to be used by the Backend::DoomEntry() call. |callback| will be
-  // run by |callback_runner|.
-  static void DoomEntry(const base::FilePath& path,
+  // Deletes an entry from the file system without affecting the state of the
+  // corresponding instance, if any (allowing operations to continue to be
+  // executed through that instance). Returns a net error code.
+  static int DoomEntry(const base::FilePath& path,
                        const std::string& key,
-                        uint64 entry_hash,
-                        int* out_result);
+                       uint64 entry_hash);
 
   // Like |DoomEntry()| above. Deletes all entries corresponding to the
   // |key_hashes|. Succeeds only when all entries are deleted. Returns a net