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

net/disk_cache/simple/simple_backend_impl.cc

Index: net/disk_cache/simple/simple_backend_impl.cc
diff --git a/net/disk_cache/simple/simple_backend_impl.cc b/net/disk_cache/simple/simple_backend_impl.cc
index 4aed36c78fc6c7db72720e3dcd84a380e72dca77..1f3c15a60a0cad8eb736ff2b6cb6d080ac062f4c 100644
--- a/net/disk_cache/simple/simple_backend_impl.cc
+++ b/net/disk_cache/simple/simple_backend_impl.cc
@@ -59,6 +59,19 @@ 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;
@@ -205,6 +218,23 @@ void RecordIndexLoad(base::TimeTicks constructed_since, int result) {
     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 {
@@ -317,7 +347,10 @@ void SimpleBackendImpl::IndexReadyForDoom(Time initial_time,
   }
   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.
@@ -326,18 +359,22 @@ void SimpleBackendImpl::IndexReadyForDoom(Time initial_time,
     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(