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

net/disk_cache/simple/simple_backend_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_backend_impl.h"
 
 #include <algorithm>
 #include <cstdlib>
 
 #if defined(OS_POSIX)
@@ skipping 41 matching lines @@
 
 // Cache size when all other size heuristics failed.
 const uint64 kDefaultCacheSize = 80 * 1024 * 1024;
 
 // Maximum fraction of the cache that one entry can consume.
 const int kMaxFileRatio = 8;
 
 // A global sequenced worker pool to use for launching all tasks.
 SequencedWorkerPool* g_sequenced_worker_pool = NULL;
 
+class DoomEntrySetContext : public base::RefCounted<DoomEntrySetContext> {
+ public:
+  DoomEntrySetContext() : entries_left(0), error_happened(false) {}
+
+  int entries_left;  // Number of entries that remain to be doomed.
+  bool error_happened;
+
+ private:
+  friend class base::RefCounted<DoomEntrySetContext>;
+
+  ~DoomEntrySetContext() {}
+};
+
 void MaybeCreateSequencedWorkerPool() {
   if (!g_sequenced_worker_pool) {
     int max_worker_threads = kDefaultMaxWorkerThreads;
 
     const std::string thread_count_field_trial =
         base::FieldTrialList::FindFullName("SimpleCacheMaxThreads");
     if (!thread_count_field_trial.empty()) {
       max_worker_threads =
           std::max(1, std::atoi(thread_count_field_trial.c_str()));
     }
@@ skipping 126 matching lines @@
 
 void RecordIndexLoad(base::TimeTicks constructed_since, int result) {
   const base::TimeDelta creation_to_index = base::TimeTicks::Now() -
                                             constructed_since;
   if (result == net::OK)
     UMA_HISTOGRAM_TIMES("SimpleCache.CreationToIndex", creation_to_index);
   else
     UMA_HISTOGRAM_TIMES("SimpleCache.CreationToIndexFail", creation_to_index);
 }
 
+// Used only by mass dooming to execute the client-provided callback once all
+// the entries are doomed. Note that this is called multiple times during a mass
+// doom operation (until the number of left entries in the context reaches 0).
+void OnDoomEntriesCompleted(
+    DoomEntrySetContext* context,
+    const net::CompletionCallback& all_entries_doomed_callback,
+    int doomed_entries_count,
+    int net_error) {
+  context->entries_left -= doomed_entries_count;
+  if (net_error == net::ERR_FAILED)
+    context->error_happened = true;
+  if (context->entries_left == 0 && !all_entries_doomed_callback.is_null()) {
+    all_entries_doomed_callback.Run(
+        context->error_happened ? net::ERR_FAILED : net::OK);
+  }
+}
+
 }  // namespace
 
 namespace disk_cache {
 
 SimpleBackendImpl::SimpleBackendImpl(const FilePath& path,
                                      int max_bytes,
                                      net::CacheType type,
                                      base::SingleThreadTaskRunner* cache_thread,
                                      net::NetLog* net_log)
     : path_(path),
@@ skipping 92 matching lines @@
 void SimpleBackendImpl::IndexReadyForDoom(Time initial_time,
                                           Time end_time,
                                           const CompletionCallback& callback,
                                           int result) {
   if (result != net::OK) {
     callback.Run(result);
     return;
   }
   scoped_ptr<std::vector<uint64> > removed_key_hashes(
       index_->RemoveEntriesBetween(initial_time, end_time).release());
-
+  const scoped_refptr<DoomEntrySetContext> context(new DoomEntrySetContext());
+  const CompletionCallback on_entry_doomed_callback = base::Bind(
+      &OnDoomEntriesCompleted, context, callback, 1  /* entry count */);
+  int doomed_active_entries_count = 0;
   // If any of the entries we are dooming are currently open, we need to remove
   // them from |active_entries_|, so that attempts to create new entries will
   // succeed and attempts to open them will fail.
   for (int i = removed_key_hashes->size() - 1; i >= 0; --i) {
     const uint64 entry_hash = (*removed_key_hashes)[i];
     EntryMap::iterator it = active_entries_.find(entry_hash);
     if (it == active_entries_.end())
       continue;
+    ++doomed_active_entries_count;
     SimpleEntryImpl* entry = it->second.get();
-    entry->Doom();
+    entry->DoomEntry(on_entry_doomed_callback);
 
     (*removed_key_hashes)[i] = removed_key_hashes->back();
     removed_key_hashes->resize(removed_key_hashes->size() - 1);
   }
+  context->entries_left += doomed_active_entries_count;
+  context->entries_left += removed_key_hashes->size();
 
   PostTaskAndReplyWithResult(
       worker_pool_, FROM_HERE,
       base::Bind(&SimpleSynchronousEntry::DoomEntrySet,
                  base::Passed(&removed_key_hashes), path_),
-      base::Bind(&CallCompletionCallback, callback));
+      base::Bind(&OnDoomEntriesCompleted, context, callback,
+                 removed_key_hashes->size()));

[gavinp, 2013/08/27 15:48:02]

What was the undefined behaviour?

[Deprecated (see juliatuttle), 2013/08/27 16:44:37]

+1; this looked fine to me.

[Philippe, 2013/08/27 16:58:56]

Ah thanks Thomas for reminding me this :) So base::Passed(&removed_key_hashes)
invalidates |removed_key_hashes| (nothing unexpected so far). But look at this
whole PostTaskAndReplyWithResult() expression. |removed_key_hashes| appear twice
in two different function call parameter expressions. One of them invalidates
|removed_key_hashes| so this was actually relying on the parameters evaluation
order :) If the parameters were evaluated from left to right the following code
would have produced a nice SIGSEGV since the second use of |removed_key_hashes|
is actually dereferencing the pointer (which was nulled by the first use doing a
Passed()).

Does that make sense?

 }
 
 int SimpleBackendImpl::DoomEntriesBetween(
     const Time initial_time,
     const Time end_time,
     const CompletionCallback& callback) {
   return index_->ExecuteWhenReady(
       base::Bind(&SimpleBackendImpl::IndexReadyForDoom, AsWeakPtr(),
                  initial_time, end_time, callback));
 }
@@ skipping 210 matching lines @@
     const CompletionCallback& callback,
     int error_code) {
   if (error_code == net::ERR_FAILED) {
     OpenNextEntry(iter, entry, callback);
     return;
   }
   CallCompletionCallback(callback, error_code);
 }
 
 }  // namespace disk_cache