Http cache: It turns out that the cache destructor...

net/http/http_cache.cc

 // Copyright (c) 2010 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/http/http_cache.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 
@@ skipping 87 matching lines @@
   // Calls back the transaction with the result of the operation.
   void NotifyTransaction(int result, ActiveEntry* entry) {
     // TODO(rvargas): convert to DCHECK after fixing bug 47895.
     CHECK(!entry || entry->disk_entry);
     if (entry_)
       *entry_ = entry;
     if (trans_)
       trans_->io_callback()->Run(result);
   }
 
-  // Notifies the caller about the operation completion.
-  void DoCallback(int result, disk_cache::Backend* backend) {
+  // Notifies the caller about the operation completion. Returns true if the
+  // callback was invoked.
+  bool DoCallback(int result, disk_cache::Backend* backend) {
     if (backend_)
       *backend_ = backend;
-    if (callback_)
+    if (callback_) {
       callback_->Run(result);
+      return true;
+    }
+    return false;
   }
 
   WorkItemOperation operation() { return operation_; }
   void ClearTransaction() { trans_ = NULL; }
   void ClearEntry() { entry_ = NULL; }
   void ClearCallback() { callback_ = NULL; }
   bool Matches(Transaction* trans) const { return trans == trans_; }
   bool IsValid() const { return trans_ || entry_ || callback_; }
 
  private:
@@ skipping 167 matching lines @@
   for (; pending_it != pending_ops_.end(); ++pending_it) {
     // We are not notifying the transactions about the cache going away, even
     // though they are waiting for a callback that will never fire.
     PendingOp* pending_op = pending_it->second;
     delete pending_op->writer;
     if (building_backend_) {
       // If we don't have a backend, when its construction finishes it will
       // deliver the callbacks.
       BackendCallback* callback =
           static_cast<BackendCallback*>(pending_op->callback);
-      callback->Cancel();
+      if (callback)
+        callback->Cancel();
     } else {
       delete pending_op->callback;
     }
 
     STLDeleteElements(&pending_op->pending_queue);
     delete pending_op;
   }
 }
 
 int HttpCache::GetBackend(disk_cache::Backend** backend,
@@ skipping 690 matching lines @@
       }
     }
   }
 }
 
 void HttpCache::OnBackendCreated(int result, PendingOp* pending_op) {
   scoped_ptr<WorkItem> item(pending_op->writer);
   WorkItemOperation op = item->operation();
   DCHECK_EQ(WI_CREATE_BACKEND, op);
 
-  backend_factory_.reset();  // Reclaim memory.
+  // We don't need the callback anymore.
+  pending_op->callback = NULL;
 
-  if (result == OK)
-    disk_cache_.reset(temp_backend_);
+  if (backend_factory_.get()) {

[eroman, 2010/08/20 18:15:23]

Can you add a comment explaining this check is because we re-call
OnBackendCreated() for each pending operation?

[rvargas (doing something else), 2010/08/20 18:47:38]

Added:
// We may end up calling OnBackendCreated multiple times if we have pending
// work items. The first call saves the backend and releases the factory,
// and the last call clears building_backend_.

+    backend_factory_.reset();  // Reclaim memory.
+    if (result == OK)
+      disk_cache_.reset(temp_backend_);
+  }
 
-  item->DoCallback(result, temp_backend_);
-
-  // Notify all callers and delete all pending work items.
-  while (!pending_op->pending_queue.empty()) {
-    scoped_ptr<WorkItem> pending_item(pending_op->pending_queue.front());
+  if (!pending_op->pending_queue.empty()) {
+    WorkItem* pending_item = pending_op->pending_queue.front();
     pending_op->pending_queue.pop_front();
     DCHECK_EQ(WI_CREATE_BACKEND, pending_item->operation());
 
-    // This could be an external caller or a transaction waiting on Start().
-    pending_item->DoCallback(result, temp_backend_);
-    pending_item->NotifyTransaction(result, NULL);
+    // We want to process a single callback at a time, because the cache may
+    // go away from the callback.
+    pending_op->writer = pending_item;
+
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        task_factory_.NewRunnableMethod(&HttpCache::OnBackendCreated,
+                                        result, pending_op));

[eroman, 2010/08/20 18:15:23]

Won't this leak pending_op if HttpCache gets deleted before the callback is run?

[rvargas (doing something else), 2010/08/20 18:47:38]

No, it doesn't. pending_op is part of a list that is deleted by the destructor
(DeletePendingOp removes the operation from that list).

[eroman, 2010/08/20 18:56:38]

Ah my bad. LGTM

+  } else {
+    building_backend_ = false;
+    DeletePendingOp(pending_op);
   }
 
-  DeletePendingOp(pending_op);
-  building_backend_ = false;
+  // The cache may be gone when we return from the callback.
+  if (!item->DoCallback(result, temp_backend_))
+    item->NotifyTransaction(result, NULL);
 }
 
 }  // namespace net