Http Cache: Doom cache entries when there is any error reading from the

cache.

BUG=110795
TEST=net_unittests
Review URL: https://chromiumcodereview.appspot.com/9358008

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

6 files changed, 56 insertions, 15 deletions

diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index cf1cd43..41e5436 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -582,6 +582,17 @@ std::string HttpCache::GenerateCacheKey(const HttpRequestInfo* request) {
   return result;
 }
 
+void HttpCache::DoomActiveEntry(const std::string& key) {
+  ActiveEntriesMap::iterator it = active_entries_.find(key);
+  if (it == active_entries_.end())
+    return;
+
+  // This is not a performance critical operation, this is handling an error
+  // condition so it is OK to look up the entry again.
+  int rv = DoomEntry(key, NULL);
+  DCHECK_EQ(OK, rv);
+}
+
 int HttpCache::DoomEntry(const std::string& key, Transaction* trans) {
   // Need to abandon the ActiveEntry, but any transaction attached to the entry
   // should not be impacted.  Dooming an entry only means that it will no
diff --git a/net/http/http_cache.h b/net/http/http_cache.h
index 27c6951..d1e7ad8 100644
--- a/net/http/http_cache.h
+++ b/net/http/http_cache.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -254,6 +254,10 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
   // Generates the cache key for this request.
   std::string GenerateCacheKey(const HttpRequestInfo*);
 
+  // Dooms the entry selected by |key|, if it is currently in the list of active
+  // entries.
+  void DoomActiveEntry(const std::string& key);
+
   // Dooms the entry selected by |key|. |trans| will be notified via its IO
   // callback if this method returns ERR_IO_PENDING. The entry can be
   // currently in use or not.
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index f5f7d3c..7851678 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -1191,8 +1191,7 @@ int HttpCache::Transaction::DoCacheReadResponseComplete(int result) {
   if (result != io_buf_len_ ||
       !HttpCache::ParseResponseInfo(read_buf_->data(), io_buf_len_,
                                     &response_, &truncated_)) {
-    DLOG(ERROR) << "ReadData failed: " << result;
-    return ERR_CACHE_READ_FAILURE;
+    return OnCacheReadError(result);
   }
 
   // Some resources may have slipped in as truncated when they're not.
@@ -1277,10 +1276,8 @@ int HttpCache::Transaction::DoCacheReadMetadata() {
 
 int HttpCache::Transaction::DoCacheReadMetadataComplete(int result) {
   net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_READ_INFO, result);
-  if (result != response_.metadata->size()) {
-    DLOG(ERROR) << "ReadData failed: " << result;
-    return ERR_CACHE_READ_FAILURE;
-  }
+  if (result != response_.metadata->size())
+    return OnCacheReadError(result);;
 
   return OK;
 }
@@ -1332,6 +1329,8 @@ int HttpCache::Transaction::DoCacheReadDataComplete(int result) {
   } else if (result == 0) {  // End of file.
     cache_->DoneReadingFromEntry(entry_, this);
     entry_ = NULL;
+  } else {
+    return OnCacheReadError(result);
   }
   return result;
 }
@@ -1991,6 +1990,16 @@ void HttpCache::Transaction::DoneWritingToEntry(bool success) {
   mode_ = NONE;  // switch to 'pass through' mode
 }
 
+int HttpCache::Transaction::OnCacheReadError(int result) {
+  DLOG(ERROR) << "ReadData failed: " << result;
+
+  // Avoid using this entry in the future.
+  if (cache_)
+    cache_->DoomActiveEntry(cache_key_);
+
+  return ERR_CACHE_READ_FAILURE;
+}
+
 void HttpCache::Transaction::DoomPartialEntry(bool delete_object) {
   DVLOG(2) << "DoomPartialEntry";
   int rv = cache_->DoomEntry(cache_key_, NULL);
@@ -2019,6 +2028,8 @@ int HttpCache::Transaction::DoPartialCacheReadCompleted(int result) {
   if (result == 0 && mode_ == READ_WRITE) {
     // We need to move on to the next range.
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
+  } else if (result < 0) {
+    return OnCacheReadError(result);
   }
   return result;
 }
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 38cf781..69cf40d 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -307,6 +307,10 @@ class HttpCache::Transaction : public HttpTransaction {
   // Called when we are done writing to the cache entry.
   void DoneWritingToEntry(bool success);
 
+  // Returns an error to signal the caller that the current read failed. The
+  // current operation |result| is also logged.
+  int OnCacheReadError(int result);
+
   // Deletes the current partial cache entry (sparse), and optionally removes
   // the control object (partial_).
   void DoomPartialEntry(bool delete_object);
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 1c8363e..5701db0 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -530,7 +530,7 @@ TEST(HttpCache, SimpleGETWithDiskFailures2) {
   EXPECT_EQ(2, cache.disk_cache()->create_count());
 }
 
-// Tests that we don't crash after failures to read from the cache.
+// Tests that we handle failures to read from the cache.
 TEST(HttpCache, SimpleGETWithDiskFailures3) {
   MockHttpCache cache;
 
@@ -551,6 +551,19 @@ TEST(HttpCache, SimpleGETWithDiskFailures3) {
   MockHttpRequest request(kSimpleGET_Transaction);
   rv = c->trans->Start(&request, c->callback.callback(), net::BoundNetLog());
   EXPECT_EQ(net::ERR_CACHE_READ_FAILURE, c->callback.GetResult(rv));
+
+  // Now verify that the entry was removed from the cache.
+  cache.disk_cache()->set_soft_failures(false);
+
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
+
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
 }
 
 TEST(HttpCache, SimpleGET_LoadOnlyFromCache_Hit) {
diff --git a/net/http/mock_http_cache.cc b/net/http/mock_http_cache.cc
index ed8976f..4237a91 100644
--- a/net/http/mock_http_cache.cc
+++ b/net/http/mock_http_cache.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -70,8 +70,6 @@ void MockDiskEntry::Close() {
 }
 
 std::string MockDiskEntry::GetKey() const {
-  if (fail_requests_)
-    return std::string();
   return key_;
 }