Http Cache: Only cache range requests when the server provides

strong validators.

BUG=77085
TEST=net_unittests
Review URL: http://codereview.chromium.org/6824048

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

4 files changed, 131 insertions, 23 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index e217893..c4293d9 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -181,19 +181,6 @@ int HttpCache::Transaction::WriteMetadata(IOBuffer* buf, int buf_len,
                                        callback, true);
 }
 
-// Histogram data from the end of 2010 show the following distribution of
-// response headers:
-//
-//   Content-Length............... 87%
-//   Date......................... 98%
-//   Last-Modified................ 49%
-//   Etag......................... 19%
-//   Accept-Ranges: bytes......... 25%
-//   Accept-Ranges: none.......... 0.4%
-//   Strong Validator............. 50%
-//   Strong Validator + ranges.... 24%
-//   Strong Validator + CL........ 49%
-//
 bool HttpCache::Transaction::AddTruncatedFlag() {
   DCHECK(mode_ & WRITE);
 
@@ -201,13 +188,7 @@ bool HttpCache::Transaction::AddTruncatedFlag() {
   if (partial_.get() && !truncated_)
     return true;
 
-  // Double check that there is something worth keeping.
-  if (!entry_->disk_entry->GetDataSize(kResponseContentIndex))
-    return false;
-
-  if (response_.headers->GetContentLength() <= 0 ||
-      response_.headers->HasHeaderValue("Accept-Ranges", "none") ||
-      !response_.headers->HasStrongValidators())
+  if (!CanResume(true))
     return false;
 
   truncated_ = true;
@@ -1073,6 +1054,16 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
     partial_->FixContentLength(new_response_->headers);
 
   response_ = *new_response_;
+
+  if (server_responded_206_ && !CanResume(false)) {
+    // There is no point in storing this resource because it will never be used.
+    DoneWritingToEntry(false);
+    if (partial_.get())
+      partial_->FixResponseHeaders(response_.headers, true);
+    next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
+    return OK;
+  }
+
   target_state_ = STATE_TRUNCATE_CACHED_DATA;
   next_state_ = truncated_ ? STATE_CACHE_WRITE_TRUNCATED_RESPONSE :
                              STATE_CACHE_WRITE_RESPONSE;
@@ -1514,8 +1505,11 @@ int HttpCache::Transaction::BeginCacheValidation() {
     // response.  If we cannot do so, then we just resort to a normal fetch.
     // Our mode remains READ_WRITE for a conditional request.  We'll switch to
     // either READ or WRITE mode once we hear back from the server.
-    if (!ConditionalizeRequest())
+    if (!ConditionalizeRequest()) {
+      DCHECK(!partial_.get());
+      DCHECK_NE(206, response_.headers->response_code());
       mode_ = WRITE;
+    }
     next_state_ = STATE_SEND_REQUEST;
   }
   return OK;
@@ -1668,6 +1662,10 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
       response_.headers->response_code() != 206)
     return false;
 
+  // We should have handled this case before.
+  DCHECK(response_.headers->response_code() != 206 ||
+         response_.headers->HasStrongValidators());
+
   // Just use the first available ETag and/or Last-Modified header value.
   // TODO(darin): Or should we use the last?
 
@@ -1970,6 +1968,35 @@ int HttpCache::Transaction::DoPartialCacheReadCompleted(int result) {
   return result;
 }
 
+// Histogram data from the end of 2010 show the following distribution of
+// response headers:
+//
+//   Content-Length............... 87%
+//   Date......................... 98%
+//   Last-Modified................ 49%
+//   Etag......................... 19%
+//   Accept-Ranges: bytes......... 25%
+//   Accept-Ranges: none.......... 0.4%
+//   Strong Validator............. 50%
+//   Strong Validator + ranges.... 24%
+//   Strong Validator + CL........ 49%
+//
+bool HttpCache::Transaction::CanResume(bool has_data) {
+  // Double check that there is something worth keeping.
+  if (has_data && !entry_->disk_entry->GetDataSize(kResponseContentIndex))
+    return false;
+
+  if (request_->method != "GET")
+    return false;
+
+  if (response_.headers->GetContentLength() <= 0 ||
+      response_.headers->HasHeaderValue("Accept-Ranges", "none") ||
+      !response_.headers->HasStrongValidators())
+    return false;
+
+  return true;
+}
+
 void HttpCache::Transaction::OnIOComplete(int result) {
   DoLoop(result);
 }
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 17d7ead..945a70e 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -318,6 +318,11 @@ class HttpCache::Transaction : public HttpTransaction {
   // working with range requests.
   int DoPartialCacheReadCompleted(int result);
 
+  // Returns true if we should bother attempting to resume this request if it
+  // is aborted while in progress. If |has_data| is true, the size of the stored
+  // data is considered for the result.
+  bool CanResume(bool has_data);
+
   // Called to signal completion of asynchronous IO.
   void OnIOComplete(int result);
 
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 682ed51..5e52c31 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -2900,6 +2900,35 @@ TEST(HttpCache, GET_Crazy206) {
   RemoveMockTransaction(&transaction);
 }
 
+// Tests that we don't cache partial responses that can't be validated.
+TEST(HttpCache, RangeGET_NoStrongValidators) {
+  MockHttpCache cache;
+  std::string headers;
+
+  // Attempt to write to the cache (40-49).
+  MockTransaction transaction(kRangeGET_TransactionOK);
+  AddMockTransaction(&transaction);
+  transaction.response_headers = "Content-Length: 10\n"
+                                 "ETag: w/\"foo\"\n";
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  Verify206Response(headers, 40, 49);
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Now verify that there's no cached data.
+  RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_TransactionOK,
+                                 &headers);
+
+  Verify206Response(headers, 40, 49);
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+
+  RemoveMockTransaction(&transaction);
+}
+
 // Tests that we can cache range requests and fetch random blocks from the
 // cache and the network.
 TEST(HttpCache, RangeGET_OK) {
@@ -3448,6 +3477,49 @@ TEST(HttpCache, RangeGET_Previous206_NotSparse_2) {
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }
 
+// Tests that we can handle cached 206 responses that can't be validated.
+TEST(HttpCache, GET_Previous206_NotValidation) {
+  MockHttpCache cache;
+
+  // Create a disk cache entry that stores 206 headers.
+  disk_cache::Entry* entry;
+  ASSERT_TRUE(cache.CreateBackendEntry(kSimpleGET_Transaction.url, &entry,
+                                       NULL));
+
+  // Make sure that the headers cannot be validated with the server.
+  std::string raw_headers(kRangeGET_TransactionOK.status);
+  raw_headers.append("\n");
+  raw_headers.append("Content-Length: 80\n");
+  raw_headers = net::HttpUtil::AssembleRawHeaders(raw_headers.data(),
+                                                  raw_headers.size());
+
+  net::HttpResponseInfo response;
+  response.headers = new net::HttpResponseHeaders(raw_headers);
+  EXPECT_TRUE(MockHttpCache::WriteResponseInfo(entry, &response, true, false));
+
+  scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(500));
+  int len = static_cast<int>(base::strlcpy(buf->data(),
+                                           kRangeGET_TransactionOK.data, 500));
+  TestCompletionCallback cb;
+  int rv = entry->WriteData(1, 0, buf, len, &cb, true);
+  EXPECT_EQ(len, cb.GetResult(rv));
+  entry->Close();
+
+  // Now see that we don't use the stored entry.
+  std::string headers;
+  RunTransactionTestWithResponse(cache.http_cache(), kSimpleGET_Transaction,
+                                 &headers);
+
+  // We are expecting a 200.
+  std::string expected_headers(kSimpleGET_Transaction.status);
+  expected_headers.append("\n");
+  expected_headers.append(kSimpleGET_Transaction.response_headers);
+  EXPECT_EQ(expected_headers, headers);
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+}
+
 // Tests that we can handle range requests with cached 200 responses.
 TEST(HttpCache, RangeGET_Previous200) {
   MockHttpCache cache;
@@ -3846,6 +3918,7 @@ TEST(HttpCache, RangeGET_LargeValues) {
   transaction.request_headers = "Range: bytes = 4294967288-4294967297\r\n"
                                 EXTRA_HEADER;
   transaction.response_headers =
+      "ETag: \"foo\"\n"
       "Content-Range: bytes 4294967288-4294967297/4294967299\n"
       "Content-Length: 10\n";
   AddMockTransaction(&transaction);
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index c1f448e..d76d689 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -273,6 +273,9 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
     return true;
   }
 
+  if (!headers->HasStrongValidators())
+    return false;
+
   int64 length_value = headers->GetContentLength();
   if (length_value <= 0)
     return false;  // We must have stored the resource length.