Disk cache: Remove all non essential synchronization from the cache destructor.

net/disk_cache/backend_impl.cc

 // Copyright (c) 2012 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/backend_impl.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
@@ skipping 334 matching lines @@
       disabled_(false),
       new_eviction_(false),
       first_timer_(true),
       user_load_(false),
       net_log_(net_log),
       done_(true, false),
       ALLOW_THIS_IN_INITIALIZER_LIST(ptr_factory_(this)) {
 }
 
 BackendImpl::~BackendImpl() {
-  background_queue_.WaitForPendingIO();
+  if (user_flags_ & kNoRandom) {

[gavinp, 2012/03/18 00:35:26]

Do these long comments call the name kNoRandom into question?  Perhaps we should
add a flag or rename this one.

[rvargas (doing something else), 2012/03/20 02:54:23]

I hear you, but I guess I'd have a comment here regardless of the name of the
flag.

Seriously though, using kNoRandom is weird (and we've been using it this way for
other things), but having a flag specifically for unit tests would not be
correct either: that would be set for the _Fast tests anyway. The other option
would be have separate flags for everything (as in a flag for fast shutdown),
but that's an explosion of flags (some set by default) so it may not be the best
thing either.

+    // This is a unit test, so we want to be strict about not leaking entries
+    // and completing all the work.
+    background_queue_.WaitForPendingIO();
+  } else {
+    // This is most likely not a test, so we want to do as little work as
+    // possible at this time, at the price of leaving dirty entries behind.
+    background_queue_.DropPendingIO();

[gavinp, 2012/03/18 00:35:26]

ignore_result(trace_object_.release());  // Leak the trace_object as it's used
by pending IO.

The problem is this:

1. An IO operation completes, calling Trace(), getting the IP past the check for
    if (!s_trace_buffer || !s_trace_enabled) , meanwhile...

2. BackendImpl::~BackendImpl calls
trace_object_.scoped_refptr<TraceObject>::~scoped_refptr<TraceObject>(),
particular this bit:
    s_trace_buffer = NULL;

3. Our old friend from (1) continues executing Trace(), and evaluates
s_trace_buffer->buffer[blah].  **BOOM**

[gavinp, 2012/03/18 00:35:26]

ignore_result(index_.release());  // Do we need this too?

I haven't found a crash on this one.  I suspect one of the fast exit
create-entry tests could, but it hasn't happened to me yet.

[gavinp, 2012/03/18 00:35:26]

What what about block_files_ ?  I hacked up a patch to make it a releasable
scoped_ptr (a nice fast-exit idiom), and crashes seemed down.  However, even
with that, I'm still seeing crashes in the main thread in OnOperationComplete() 

+  }
 
   if (background_queue_.BackgroundIsCurrentThread()) {
     // Unit tests may use the same thread for everything.
     CleanupCache();
   } else {
     background_queue_.background_thread()->PostTask(
         FROM_HERE, base::Bind(&FinalCleanupCallback, base::Unretained(this)));
     done_.Wait();
   }
 }
@@ skipping 123 matching lines @@
 void BackendImpl::CleanupCache() {
   Trace("Backend Cleanup");
   eviction_.Stop();
   timer_.Stop();
 
   if (init_) {
     stats_.Store();
     if (data_)
       data_->header.crash = 0;
 
-    File::WaitForPendingIO(&num_pending_io_);
     if (user_flags_ & kNoRandom) {
       // This is a net_unittest, verify that we are not 'leaking' entries.
+      File::WaitForPendingIO(&num_pending_io_);
       DCHECK(!num_refs_);
     }
   }
   block_files_.CloseFiles();
   index_ = NULL;
   ptr_factory_.InvalidateWeakPtrs();
   done_.Signal();
 }
 
 // ------------------------------------------------------------------------
@@ skipping 307 matching lines @@
   std::string tmp = base::StringPrintf("f_%06x", address.FileNumber());
   return path_.AppendASCII(tmp);
 }
 
 MappedFile* BackendImpl::File(Addr address) {
   if (disabled_)
     return NULL;
   return block_files_.GetFile(address);
 }
 
+base::WeakPtr<InFlightBackendIO> BackendImpl::GetBackgroundQueue() {
+  return background_queue_.GetWeakPtr();
+}
+
 bool BackendImpl::CreateExternalFile(Addr* address) {
   int file_number = data_->header.last_file + 1;
   Addr file_address(0);
   bool success = false;
   for (int i = 0; i < 0x0fffffff; i++, file_number++) {
     if (!file_address.SetFileNumber(file_number)) {
       file_number = 1;
       continue;
     }
     FilePath name = GetFileName(file_address);
@@ skipping 1327 matching lines @@
     if (total_memory > kMaxBuffersSize || total_memory <= 0)
       total_memory = kMaxBuffersSize;
 
     done = true;
   }
 
   return static_cast<int>(total_memory);
 }
 
 }  // namespace disk_cache