Http cache: Make sure that we don't send notifications

from the cache transaction destructor.

BUG=31723
TEST=unittests

Review URL: http://codereview.chromium.org/594018

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@38554 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 44 insertions, 9 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 3be313f..4db23f9 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-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.
 
@@ -138,6 +138,11 @@ HttpCache::Transaction::~Transaction() {
   // TODO(rvargas): remove this after finding the cause for bug 31723.
   char local_obj[sizeof(*this)];
   memcpy(local_obj, this, sizeof(local_obj));
+
+  // We may have to issue another IO, but we should never invoke the callback_
+  // after this point.
+  callback_ = NULL;
+
   if (cache_) {
     if (entry_) {
       bool cancel_request = reading_ && enable_range_support_;
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 15ac1aa..c62052f 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-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.
 
@@ -47,6 +47,18 @@ int GetTestModeForEntry(const std::string& key) {
   return t->test_mode;
 }
 
+// We can override the test mode for a given operation by setting this global
+// variable. Just remember to reset it after the test!.
+int g_test_mode = 0;
+
+// Returns the test mode after considering the global override.
+int GetEffectiveTestMode(int test_mode) {
+  if (!g_test_mode)
+    return test_mode;
+
+  return g_test_mode;
+}
+
 //-----------------------------------------------------------------------------
 // mock disk cache (a very basic memory cache implementation)
 
@@ -108,7 +120,8 @@ class MockDiskEntry : public disk_cache::Entry,
     int num = std::min(buf_len, static_cast<int>(data_[index].size()) - offset);
     memcpy(buf->data(), &data_[index][offset], num);
 
-    if (!callback || (test_mode_ & TEST_MODE_SYNC_CACHE_READ))
+    if (!callback ||
+        (GetEffectiveTestMode(test_mode_) & TEST_MODE_SYNC_CACHE_READ))
       return num;
 
     CallbackLater(callback, num);
@@ -130,7 +143,8 @@ class MockDiskEntry : public disk_cache::Entry,
     if (buf_len)
       memcpy(&data_[index][offset], buf->data(), buf_len);
 
-    if (!callback || (test_mode_ & TEST_MODE_SYNC_CACHE_WRITE))
+    if (!callback ||
+        (GetEffectiveTestMode(test_mode_) & TEST_MODE_SYNC_CACHE_WRITE))
       return buf_len;
 
     CallbackLater(callback, buf_len);
@@ -156,7 +170,8 @@ class MockDiskEntry : public disk_cache::Entry,
                        buf_len);
     memcpy(buf->data(), &data_[1][real_offset], num);
 
-    if (!completion_callback || (test_mode_ & TEST_MODE_SYNC_CACHE_READ))
+    if (!completion_callback ||
+        (GetEffectiveTestMode(test_mode_) & TEST_MODE_SYNC_CACHE_READ))
       return num;
 
     CallbackLater(completion_callback, num);
@@ -189,7 +204,8 @@ class MockDiskEntry : public disk_cache::Entry,
       data_[1].resize(real_offset + buf_len);
 
     memcpy(&data_[1][real_offset], buf->data(), buf_len);
-    if (!completion_callback || (test_mode_ & TEST_MODE_SYNC_CACHE_WRITE))
+    if (!completion_callback ||
+        (GetEffectiveTestMode(test_mode_) & TEST_MODE_SYNC_CACHE_WRITE))
       return buf_len;
 
     CallbackLater(completion_callback, buf_len);
@@ -241,7 +257,7 @@ class MockDiskEntry : public disk_cache::Entry,
 
     cancel_ = false;
     DCHECK(completion_callback);
-    if (test_mode_ & TEST_MODE_SYNC_CACHE_READ)
+    if (GetEffectiveTestMode(test_mode_) & TEST_MODE_SYNC_CACHE_READ)
       return net::OK;
 
     // The pending operation is already in the message loop (and hopefuly
@@ -3565,7 +3581,7 @@ TEST(HttpCache, Set_Truncated_Flag) {
   AddMockTransaction(&transaction);
   MockHttpRequest request(transaction);
 
-  Context* c = new Context();
+  scoped_ptr<Context> c(new Context());
   int rv = cache.http_cache()->CreateTransaction(&c->trans);
   EXPECT_EQ(net::OK, rv);
 
@@ -3584,8 +3600,22 @@ TEST(HttpCache, Set_Truncated_Flag) {
     rv = c->callback.WaitForResult();
   EXPECT_EQ(buf->size(), rv);
 
+  // We want to cancel the request when the transaction is busy.
+  rv = c->trans->Read(buf, buf->size(), &c->callback);
+  EXPECT_EQ(net::ERR_IO_PENDING, rv);
+  EXPECT_FALSE(c->callback.have_result());
+
+  g_test_mode = TEST_MODE_SYNC_ALL;
+
   // Destroy the transaction.
-  delete c;
+  c->trans.reset();
+  g_test_mode = 0;
+
+  // Make sure that we don't invoke the callback. We may have an issue if the
+  // UrlRequestJob is killed directly (without cancelling the UrlRequest) so we
+  // could end up with the transaction being deleted twice if we send any
+  // notification from the transaction destructor (see http://crbug.com/31723).
+  EXPECT_FALSE(c->callback.have_result());
 
   // Verify that the entry is marked as incomplete.
   disk_cache::Entry* entry;