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

net/disk_cache/simple/simple_entry_impl.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/disk_cache/simple/simple_entry_impl.h"
 
 #include <algorithm>
 #include <cstring>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/logging.h"
 #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"
 #include "net/disk_cache/net_log_parameters.h"
 #include "net/disk_cache/simple/simple_backend_impl.h"
 #include "net/disk_cache/simple/simple_index.h"
 #include "net/disk_cache/simple/simple_net_log_parameters.h"
 #include "net/disk_cache/simple/simple_synchronous_entry.h"
 #include "net/disk_cache/simple/simple_util.h"
 #include "third_party/zlib/zlib.h"
@@ skipping 221 matching lines @@
 
   RunNextOperationIfNeeded();
   return ret_value;
 }
 
 int SimpleEntryImpl::DoomEntry(const CompletionCallback& callback) {
   net_log_.AddEvent(net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_DOOM_CALL);
   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;
 }
 
 void SimpleEntryImpl::SetKey(const std::string& key) {
   key_ = key;
   net_log_.AddEvent(net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_SET_KEY,
       net::NetLog::StringCallback("key", &key));
 }
 
 void SimpleEntryImpl::Doom() {
@@ skipping 296 matching lines @@
         break;
       case SimpleEntryOperation::TYPE_WRITE:
         RecordWriteDependencyType(*operation);
         WriteDataInternal(operation->index(),
                           operation->offset(),
                           operation->buf(),
                           operation->length(),
                           operation->callback(),
                           operation->truncate());
         break;
+      case SimpleEntryOperation::TYPE_DOOM:
+        DoomEntryInternal(operation->callback());
+        break;
       default:
         NOTREACHED();
     }
     // The operation is kept for histograms. Makes sure it does not leak
     // resources.
     executing_operation_.swap(operation);
     executing_operation_->ReleaseReferences();
     // |this| may have been deleted.
   }
 }
 
 void SimpleEntryImpl::OpenEntryInternal(bool have_index,
                                         const CompletionCallback& callback,
                                         Entry** out_entry) {
   ScopedOperationRunner operation_runner(this);
 
   net_log_.AddEvent(net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_OPEN_BEGIN);
 
   if (state_ == STATE_READY) {
     ReturnEntryToCaller(out_entry);
     MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(callback,
                                                                 net::OK));
     net_log_.AddEvent(
         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));
     }
     net_log_.AddEvent(
         net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_OPEN_END,
         CreateNetLogSimpleEntryCreationCallback(this, net::ERR_FAILED));
     return;
   }
 
@@ skipping 106 matching lines @@
     synchronous_entry_ = NULL;
     worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
 
     for (int i = 0; i < kSimpleEntryFileCount; ++i) {
       if (!have_written_[i]) {
         UMA_HISTOGRAM_ENUMERATION("SimpleCache.CheckCRCResult",
                                   crc_check_state_[i], CRC_CHECK_MAX);
       }
     }
   } else {
-    synchronous_entry_ = NULL;
     CloseOperationComplete();
   }
 }
 
 void SimpleEntryImpl::ReadDataInternal(int stream_index,
                                        int offset,
                                        net::IOBuffer* buf,
                                        int buf_len,
                                        const CompletionCallback& callback) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
@@ skipping 137 matching lines @@
                             result.get());
   Closure reply = base::Bind(&SimpleEntryImpl::WriteOperationComplete,
                              this,
                              stream_index,
                              callback,
                              base::Passed(&entry_stat),
                              base::Passed(&result));
   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,
     scoped_ptr<SimpleEntryCreationResults> in_results,
     Entry** out_entry,
     net::NetLog::EventType end_event_type) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK_EQ(state_, STATE_IO_PENDING);
   DCHECK(in_results);
   ScopedOperationRunner operation_runner(this);
@@ skipping 150 matching lines @@
     RecordWriteResult(WRITE_RESULT_SYNC_WRITE_FAILURE);
   if (net_log_.IsLoggingAllEvents()) {
     net_log_.AddEvent(net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_WRITE_END,
         CreateNetLogReadWriteCompleteCallback(*result));
   }
 
   EntryOperationComplete(
       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,
     const CompletionCallback& completion_callback,
     scoped_ptr<int> result) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK(synchronous_entry_);
   DCHECK_EQ(STATE_IO_PENDING, state_);
   DCHECK(result);
 
@@ skipping 122 matching lines @@
     } else {
       type = conflicting ? WRITE_FOLLOWS_CONFLICTING_WRITE
                          : WRITE_FOLLOWS_NON_CONFLICTING_WRITE;
     }
   }
   UMA_HISTOGRAM_ENUMERATION(
       "SimpleCache.WriteDependencyType", type, WRITE_DEPENDENCY_TYPE_MAX);
 }
 
 }  // namespace disk_cache