Merge 33133 - Http cache: Add code to restart a network request when the

server doesn't revalidate a partially stored entry, in other
words, after we issued a conditional byte range request.

BUG=27276
TEST=unittests

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

TBR=rvargas@google.com
Review URL: http://codereview.chromium.org/437074

git-svn-id: svn://svn.chromium.org/chrome/branches/249/src@33153 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 94 insertions, 28 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index cdc4ecc..11e2e3d 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -597,7 +597,7 @@ void HttpCache::Transaction::SetRequest(LoadLog* load_log,
       partial_->SetHeaders(new_extra_headers);
     } else {
       // The range is invalid or we cannot handle it properly.
-      LOG(WARNING) << "Invalid byte range found.";
+      LOG(INFO) << "Invalid byte range found.";
       effective_load_flags_ |= LOAD_DISABLE_CACHE;
       partial_.reset(NULL);
     }
@@ -912,7 +912,7 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
     }
     custom_request_->extra_headers.append(etag_value);
     custom_request_->extra_headers.append("\r\n");
-    if (partial_.get() && !partial_->IsCurrentRangeCached())
+    if (partial_.get() && partial_->IsCurrentRangeCached())
       return true;
   }
 
@@ -939,40 +939,42 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
 // expect it after all), or maybe a range that was not exactly what it was asked
 // for.
 //
-// For example, if the original request is for 30KB and we have the last 20KB,
-// we ask the server for the first 10KB. If the resourse has changed, we'll
-// end up forwarding the 200 back to the user (so far so good). However, if
-// we have instead the first 10KB, we end up sending back a byte range response
-// for the first 10KB, because we never asked the server for the last part. It's
-// just too complicated to restart the whole request from this point; and of
-// course, maybe we already returned the headers.
+// If the server is simply telling us that the resource has changed, we delete
+// the cached entry and restart the request as the caller intended (by returning
+// false from this method). However, we may not be able to do that at any point,
+// for instance if we already returned the headers to the user.
+//
+// WARNING: Whenever this code returns false, it has to make sure that the next
+// time it is called it will return true so that we don't keep retrying the
+// request.
 bool HttpCache::Transaction::ValidatePartialResponse(
-    const HttpResponseHeaders* headers) {
+    const HttpResponseHeaders* headers, bool* partial_content) {
   int response_code = headers->response_code();
-  bool partial_content = enable_range_support_ ? response_code == 206 : false;
+  bool partial_response = enable_range_support_ ? response_code == 206 : false;
+  *partial_content = false;
 
   if (!entry_)
-    return false;
+    return true;
 
   if (invalid_range_) {
     // We gave up trying to match this request with the stored data. If the
     // server is ok with the request, delete the entry, otherwise just ignore
     // this request
-    if (partial_content || response_code == 200 || response_code == 304) {
+    if (partial_response || response_code == 200 || response_code == 304) {
       DoomPartialEntry(true);
       mode_ = NONE;
     } else {
       IgnoreRangeRequest();
     }
-    return false;
+    return true;
   }
 
   if (!partial_.get()) {
     // We are not expecting 206 but we may have one.
-    if (partial_content)
+    if (partial_response)
       IgnoreRangeRequest();
 
-    return false;
+    return true;
   }
 
   // TODO(rvargas): Do we need to consider other results here?.
@@ -980,28 +982,39 @@ bool HttpCache::Transaction::ValidatePartialResponse(
 
   if (partial_->IsCurrentRangeCached()) {
     // We asked for "If-None-Match: " so a 206 means a new object.
-    if (partial_content)
+    if (partial_response)
       failure = true;
 
     if (response_code == 304 && partial_->ResponseHeadersOK(headers))
-      return false;
+      return true;
   } else {
     // We asked for "If-Range: " so a 206 means just another range.
-    if (partial_content && partial_->ResponseHeadersOK(headers))
+    if (partial_response && partial_->ResponseHeadersOK(headers)) {
+      *partial_content = true;
       return true;
+    }
 
     // 304 is not expected here, but we'll spare the entry.
   }
 
   if (failure) {
     // We cannot truncate this entry, it has to be deleted.
-    DoomPartialEntry(true);
+    DoomPartialEntry(false);
     mode_ = NONE;
-    return false;
+    if (!reading_ && !partial_->IsLastRange()) {
+      // We'll attempt to issue another network request, this time without us
+      // messing up the headers.
+      partial_->RestoreHeaders(&custom_request_->extra_headers);
+      partial_.reset();
+      return false;
+    }
+    LOG(WARNING) << "Failed to revalidate partial entry";
+    partial_.reset();
+    return true;
   }
 
   IgnoreRangeRequest();
-  return false;
+  return true;
 }
 
 void HttpCache::Transaction::IgnoreRangeRequest() {
@@ -1263,7 +1276,13 @@ void HttpCache::Transaction::OnNetworkInfoAvailable(int result) {
         new_response->headers->response_code() == 407) {
       auth_response_ = *new_response;
     } else {
-      bool partial_content = ValidatePartialResponse(new_response->headers);
+      bool partial_content;
+      if (!ValidatePartialResponse(new_response->headers, &partial_content)) {
+        // Something went wrong with this request and we have to restart it.
+        network_trans_.reset();
+        BeginNetworkRequest();
+        return;
+      }
       if (partial_content && mode_ == READ_WRITE && !truncated_ &&
           response_.headers->response_code() == 200) {
         // We have stored the full entry, but it changed and the server is
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index d58105f..69b785c 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -156,8 +156,11 @@ class HttpCache::Transaction : public HttpTransaction {
   // copy is valid).  Returns true if able to make the request conditional.
   bool ConditionalizeRequest();
 
-  // Makes sure that a 206 response is expected.  Returns a network error code.
-  bool ValidatePartialResponse(const HttpResponseHeaders* headers);
+  // Makes sure that a 206 response is expected.  Returns true on success.
+  // On success, |partial_content| will be set to true if we are processing a
+  // partial entry.
+  bool ValidatePartialResponse(const HttpResponseHeaders* headers,
+                               bool* partial_content);
 
   // Handles a response validation error by bypassing the cache.
   void IgnoreRangeRequest();
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index dbcd3b1..0e693cf 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -2503,6 +2503,48 @@ TEST(HttpCache, GET_Previous206_NotModified) {
   RemoveMockTransaction(&transaction);
 }
 
+// Tests that we can handle a regular request to a sparse entry, that results in
+// new content provided by the server (206).
+TEST(HttpCache, GET_Previous206_NewContent) {
+  MockHttpCache cache;
+  cache.http_cache()->set_enable_range_support(true);
+  AddMockTransaction(&kRangeGET_TransactionOK);
+  std::string headers;
+
+  // Write to the cache (0-9).
+  MockTransaction transaction(kRangeGET_TransactionOK);
+  transaction.request_headers = "Range: bytes = 0-9\r\n" EXTRA_HEADER;
+  transaction.data = "rg: 00-09 ";
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  EXPECT_TRUE(Verify206Response(headers, 0, 9));
+  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 we'll issue a request without any range that should result first in a
+  // 206 (when revalidating), and then in a weird standard answer: the test
+  // server will not modify the response so we'll get the default range... a
+  // real server will answer with 200.
+  MockTransaction transaction2(kRangeGET_TransactionOK);
+  transaction2.request_headers = EXTRA_HEADER;
+  transaction2.data = "rg: 40-49 ";
+  RangeTransactionServer handler;
+  handler.set_modified(true);
+  RunTransactionTestWithResponse(cache.http_cache(), transaction2, &headers);
+
+  EXPECT_EQ(0U, headers.find("HTTP/1.1 206 Partial Content\n"));
+  EXPECT_EQ(3, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Verify that the previous request deleted the entry.
+  RunTransactionTest(cache.http_cache(), transaction);
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+
+  RemoveMockTransaction(&transaction);
+}
+
 // Tests that we can handle cached 206 responses that are not sparse.
 TEST(HttpCache, GET_Previous206_NotSparse) {
   MockHttpCache cache;
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index 162948c..73d07b3 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -47,7 +47,9 @@ void PartialData::RestoreHeaders(std::string* headers) const {
   int64 end = byte_range_.IsSuffixByteRange() ?
               byte_range_.suffix_length() : byte_range_.last_byte_position();
 
-  AddRangeHeader(current_range_start_, end, headers);
+  headers->assign(extra_headers_);
+  if (byte_range_.IsValid())
+    AddRangeHeader(current_range_start_, end, headers);
 }
 
 int PartialData::PrepareCacheValidation(disk_cache::Entry* entry,
diff --git a/net/http/partial_data.h b/net/http/partial_data.h
index 588e3e5..3a30e0a 100644
--- a/net/http/partial_data.h
+++ b/net/http/partial_data.h
@@ -46,8 +46,8 @@ class PartialData {
   // removed.
   void SetHeaders(const std::string& headers);
 
-  // Restores the byte-range header that was removed during Init(), by appending
-  // the data to the provided |headers|.
+  // Restores the byte-range headers, by appending the byte range to the headers
+  // provided to SetHeaders().
   void RestoreHeaders(std::string* headers) const;
 
   // Builds the required |headers| to perform the proper cache validation for