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));
+  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 254 matching lines @@
   if (!backend_.get())
     return;
   backend_->OnDeactivated(this);
   backend_.reset();
 }
 
 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() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   UMA_HISTOGRAM_CUSTOM_COUNTS("SimpleCache.EntryOperationsPending",
                               pending_operations_.size(), 0, 100, 20);
   if (!pending_operations_.empty() && state_ != STATE_IO_PENDING) {
     scoped_ptr<SimpleEntryOperation> operation(
         new SimpleEntryOperation(pending_operations_.front()));
     pending_operations_.pop();
@@ skipping 21 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.
   }
 }
@@ skipping 291 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/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,
     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(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,
     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