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

net/disk_cache/backend_impl.cc

Index: net/disk_cache/backend_impl.cc
===================================================================
--- net/disk_cache/backend_impl.cc	(revision 126788)
+++ net/disk_cache/backend_impl.cc	(working copy)
@@ -352,7 +352,15 @@
 }
 
 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.
@@ -496,9 +504,9 @@
     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_);
     }
   }
@@ -826,6 +834,10 @@
   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);