Use RevocableStore to isolate the http cache from its transactions.

This fixes a crash on shutdown when transactions are deleted after the cache is gone.

Bug=6956


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

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

3 files changed, 51 insertions, 6 deletions

diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index f1132fc..bf42584 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -160,10 +160,12 @@ HttpCache::ActiveEntry::~ActiveEntry() {
 
 //-----------------------------------------------------------------------------
 
-class HttpCache::Transaction : public HttpTransaction {
+class HttpCache::Transaction
+    : public HttpTransaction, public RevocableStore::Revocable {
  public:
   explicit Transaction(HttpCache* cache)
-      : request_(NULL),
+      : RevocableStore::Revocable(&cache->transactions_),
+        request_(NULL),
         cache_(cache),
         entry_(NULL),
         network_trans_(NULL),
@@ -318,10 +320,12 @@ class HttpCache::Transaction : public HttpTransaction {
 };
 
 HttpCache::Transaction::~Transaction() {
-  if (entry_) {
-    cache_->DoneWithEntry(entry_, this);
-  } else {
-    cache_->RemovePendingTransaction(this);
+  if (!revoked()) {
+    if (entry_) {
+      cache_->DoneWithEntry(entry_, this);
+    } else {
+      cache_->RemovePendingTransaction(this);
+    }
   }
 
   // If there is an outstanding callback, mark it as cancelled so running it
@@ -341,6 +345,9 @@ int HttpCache::Transaction::Start(const HttpRequestInfo* request,
   // ensure that we only have one asynchronous call at a time.
   DCHECK(!callback_);
 
+  if (revoked())
+    return ERR_UNEXPECTED;
+
   SetRequest(request);
 
   int rv;
@@ -382,6 +389,9 @@ int HttpCache::Transaction::RestartIgnoringLastError(
   // ensure that we only have one asynchronous call at a time.
   DCHECK(!callback_);
 
+  if (revoked())
+    return ERR_UNEXPECTED;
+
   int rv = RestartNetworkRequest();
 
   if (rv == ERR_IO_PENDING)
@@ -400,6 +410,9 @@ int HttpCache::Transaction::RestartWithAuth(
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(!callback_);
 
+  if (revoked())
+    return ERR_UNEXPECTED;
+
   // Clear the intermediate response since we are going to start over.
   auth_response_ = HttpResponseInfo();
 
@@ -419,6 +432,9 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len,
 
   DCHECK(!callback_);
 
+  if (revoked())
+    return ERR_UNEXPECTED;
+
   // If we have an intermediate auth response at this point, then it means the
   // user wishes to read the network response (the error page).  If there is a
   // previous response in the cache then we should leave it intact.
@@ -485,6 +501,9 @@ uint64 HttpCache::Transaction::GetUploadProgress() const {
 int HttpCache::Transaction::AddToEntry() {
   ActiveEntry* entry = NULL;
 
+  if (revoked())
+    return ERR_UNEXPECTED;
+
   if (mode_ == WRITE) {
     cache_->DoomEntry(cache_key_);
   } else {
@@ -842,6 +861,11 @@ void HttpCache::Transaction::DoneWritingToEntry(bool success) {
 void HttpCache::Transaction::OnNetworkInfoAvailable(int result) {
   DCHECK(result != ERR_IO_PENDING);
 
+  if (revoked()) {
+    HandleResult(ERR_UNEXPECTED);
+    return;
+  }
+
   if (result == OK) {
     const HttpResponseInfo* new_response = network_trans_->GetResponseInfo();
     if (new_response->headers->response_code() == 401 ||
@@ -895,6 +919,11 @@ void HttpCache::Transaction::OnNetworkInfoAvailable(int result) {
 void HttpCache::Transaction::OnNetworkReadCompleted(int result) {
   DCHECK(mode_ & WRITE || mode_ == NONE);
 
+  if (revoked()) {
+    HandleResult(ERR_UNEXPECTED);
+    return;
+  }
+
   if (result > 0) {
     AppendResponseDataToEntry(read_buf_, result);
   } else if (result == 0) {  // end of file
@@ -907,6 +936,11 @@ void HttpCache::Transaction::OnCacheReadCompleted(int result) {
   DCHECK(cache_);
   cache_read_callback_->Release();  // Balance the AddRef() from Start().
 
+  if (revoked()) {
+    HandleResult(ERR_UNEXPECTED);
+    return;
+  }
+
   if (result > 0) {
     read_offset_ += result;
   } else if (result == 0) {  // end of file
diff --git a/net/http/http_cache.h b/net/http/http_cache.h
index 9729527..63e5257 100644
--- a/net/http/http_cache.h
+++ b/net/http/http_cache.h
@@ -164,6 +164,8 @@ class HttpCache : public HttpTransactionFactory {
   typedef base::hash_map<std::string, int> PlaybackCacheMap;
   scoped_ptr<PlaybackCacheMap> playback_cache_map_;
 
+  RevocableStore transactions_;
+
   DISALLOW_COPY_AND_ASSIGN(HttpCache);
 };
 
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 91bec6e..e17448d 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -1088,3 +1088,12 @@ TEST(HttpCache, SimpleGET_SSLError) {
     rv = callback.WaitForResult();
   ASSERT_EQ(net::ERR_CACHE_MISS, rv);
 }
+
+// Ensure that we don't crash by if left-behind transactions.
+TEST(HttpCache, OutlivedTransactions) {
+  MockHttpCache* cache = new MockHttpCache;
+
+  net::HttpTransaction* trans = cache->http_cache()->CreateTransaction();
+  delete cache; 
+  delete trans;
+}