Http cache: Make sure that issuing a byte range request

does not prevent caching of responses that are not 200,
and does not delete the cached response if not really needed.

For example, we should be able to cache redirects when the
request is made using a byte range.

BUG=134267
TEST=net_unittests
Review URL: https://chromiumcodereview.appspot.com/10829069

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

4 files changed, 80 insertions, 28 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index c6a7c9f..7b84844 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -1611,15 +1611,24 @@ int HttpCache::Transaction::BeginCacheValidation() {
   if (truncated_)
     skip_validation = !partial_->initial_validation();
 
-  if ((partial_.get() && !partial_->IsCurrentRangeCached()) || invalid_range_)
+  if (partial_.get() && (is_sparse_ || truncated_) &&
+      (!partial_->IsCurrentRangeCached() || invalid_range_)) {
+    // Force revalidation for sparse or truncated entries. Note that we don't
+    // want to ignore the regular validation logic just because a byte range was
+    // part of the request.
     skip_validation = false;
+  }
 
   if (skip_validation) {
     if (partial_.get()) {
-      // We are going to return the saved response headers to the caller, so
-      // we may need to adjust them first.
-      next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
-      return OK;
+      if (truncated_ || is_sparse_ || !invalid_range_) {
+        // We are going to return the saved response headers to the caller, so
+        // we may need to adjust them first.
+        next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
+        return OK;
+      } else {
+        partial_.reset();
+      }
     }
     cache_->ConvertWriterToReader(entry_);
     mode_ = READ;
@@ -1632,7 +1641,9 @@ int HttpCache::Transaction::BeginCacheValidation() {
     // 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()) {
-      DCHECK(!partial_.get());
+      if (partial_.get())
+        return DoRestartPartialRequest();
+
       DCHECK_NE(206, response_.headers->response_code());
       mode_ = WRITE;
     }
@@ -1669,13 +1680,7 @@ int HttpCache::Transaction::ValidateEntryHeadersAndContinue() {
 
   if (!partial_->UpdateFromStoredHeaders(response_.headers, entry_->disk_entry,
                                          truncated_)) {
-    // The stored data cannot be used. Get rid of it and restart this request.
-    // We need to also reset the |truncated_| flag as a new entry is created.
-    DoomPartialEntry(!range_requested_);
-    mode_ = WRITE;
-    truncated_ = false;
-    next_state_ = STATE_INIT_ENTRY;
-    return OK;
+    return DoRestartPartialRequest();
   }
 
   if (response_.headers->response_code() == 206)
@@ -1915,12 +1920,19 @@ bool HttpCache::Transaction::ValidatePartialResponse() {
       return true;
     }
 
-    if (response_code == 200 && !reading_ && !is_sparse_) {
-      // The server is sending the whole resource, and we can save it.
-      DCHECK((truncated_ && !partial_->IsLastRange()) || range_requested_);
-      partial_.reset();
-      truncated_ = false;
-      return true;
+    if (!reading_ && !is_sparse_ && !partial_response) {
+      // See if we can ignore the fact that we issued a byte range request.
+      // If the server sends 200, just store it. If it sends an error, redirect
+      // or something else, we may store the response as long as we didn't have
+      // anything already stored.
+      if (response_code == 200 ||
+          (!truncated_ && response_code != 304 && response_code != 416)) {
+        // The server is sending something else, and we can save it.
+        DCHECK((truncated_ && !partial_->IsLastRange()) || range_requested_);
+        partial_.reset();
+        truncated_ = false;
+        return true;
+      }
     }
 
     // 304 is not expected here, but we'll spare the entry (unless it was
@@ -2117,6 +2129,16 @@ int HttpCache::Transaction::DoPartialCacheReadCompleted(int result) {
   return result;
 }
 
+int HttpCache::Transaction::DoRestartPartialRequest() {
+  // The stored data cannot be used. Get rid of it and restart this request.
+  // We need to also reset the |truncated_| flag as a new entry is created.
+  DoomPartialEntry(!range_requested_);
+  mode_ = WRITE;
+  truncated_ = false;
+  next_state_ = STATE_INIT_ENTRY;
+  return OK;
+}
+
 // Histogram data from the end of 2010 show the following distribution of
 // response headers:
 //
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 3f8a5e5..991c5ad 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -327,6 +327,11 @@ class HttpCache::Transaction : public HttpTransaction {
   // working with range requests.
   int DoPartialCacheReadCompleted(int result);
 
+  // Restarts this transaction after deleting the cached data. It is meant to
+  // be used when the current request cannot be fulfilled due to conflicts
+  // between the byte range request and the cached entry.
+  int DoRestartPartialRequest();
+
   // 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.
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index c43d49d..589b565 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -2805,6 +2805,31 @@ TEST(HttpCache, RangeGET_ModifiedResult) {
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }
 
+// Tests that we cache 301s for range requests.
+TEST(HttpCache, RangeGET_301) {
+  MockHttpCache cache;
+  ScopedMockTransaction transaction(kRangeGET_TransactionOK);
+  transaction.status = "HTTP/1.1 301 Moved Permanently";
+  transaction.response_headers = "Location: http://www.bar.com/\n";
+  transaction.data = "";
+  transaction.handler = NULL;
+  AddMockTransaction(&transaction);
+
+  // Write to the cache.
+  RunTransactionTest(cache.http_cache(), transaction);
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Read from the cache.
+  RunTransactionTest(cache.http_cache(), transaction);
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  RemoveMockTransaction(&transaction);
+}
+
 // Tests that we can cache range requests when the start or end is unknown.
 // We start with one suffix request, followed by a request from a given point.
 TEST(HttpCache, UnknownRangeGET_1) {
@@ -3184,13 +3209,13 @@ TEST(HttpCache, RangeGET_Previous200) {
   // Make a request for an invalid range.
   MockTransaction transaction3(kRangeGET_TransactionOK);
   transaction3.request_headers = "Range: bytes = 80-90\r\n" EXTRA_HEADER;
-  transaction3.data = "";
+  transaction3.data = transaction.data;
   transaction3.load_flags = net::LOAD_PREFERRING_CACHE;
   RunTransactionTestWithResponse(cache.http_cache(), transaction3, &headers);
   EXPECT_EQ(2, cache.disk_cache()->open_count());
-  EXPECT_EQ(0U, headers.find("HTTP/1.1 416 "));
-  EXPECT_NE(std::string::npos, headers.find("Content-Range: bytes 0-0/80"));
-  EXPECT_NE(std::string::npos, headers.find("Content-Length: 0"));
+  EXPECT_EQ(0U, headers.find("HTTP/1.1 200 "));
+  EXPECT_EQ(std::string::npos, headers.find("Content-Range:"));
+  EXPECT_EQ(std::string::npos, headers.find("Content-Length: 80"));
 
   // Make sure the entry is deactivated.
   MessageLoop::current()->RunAllPending();
@@ -3207,7 +3232,7 @@ TEST(HttpCache, RangeGET_Previous200) {
   transaction2.request_headers = kRangeGET_TransactionOK.request_headers;
   RunTransactionTestWithResponse(cache.http_cache(), transaction2, &headers);
   Verify206Response(headers, 40, 49);
-  EXPECT_EQ(5, cache.network_layer()->transaction_count());
+  EXPECT_EQ(4, cache.network_layer()->transaction_count());
   EXPECT_EQ(4, cache.disk_cache()->open_count());
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index eb0239d..0e095ad 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -242,9 +242,6 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
                                           disk_cache::Entry* entry,
                                           bool truncated) {
   resource_size_ = 0;
-  if (!headers->HasStrongValidators())
-    return false;
-
   if (truncated) {
     DCHECK_EQ(headers->response_code(), 200);
     // We don't have the real length and the user may be trying to create a
@@ -252,6 +249,9 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
     if (byte_range_.IsValid())
       return false;
 
+    if (!headers->HasStrongValidators())
+      return false;
+
     // Now we avoid resume if there is no content length, but that was not
     // always the case so double check here.
     int64 total_length = headers->GetContentLength();
@@ -270,7 +270,7 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
     return true;
   }
 
-  if (headers->response_code() == 200) {
+  if (headers->response_code() != 206) {
     DCHECK(byte_range_.IsValid());
     sparse_entry_ = false;
     resource_size_ = entry->GetDataSize(kDataStream);