Revert 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, 28850
TEST=unittests

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

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

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

5 files changed, 28 insertions, 94 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 11e2e3d..cdc4ecc 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(INFO) << "Invalid byte range found.";
+      LOG(WARNING) << "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,42 +939,40 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
 // expect it after all), or maybe a range that was not exactly what it was asked
 // for.
 //
-// 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.
+// 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.
 bool HttpCache::Transaction::ValidatePartialResponse(
-    const HttpResponseHeaders* headers, bool* partial_content) {
+    const HttpResponseHeaders* headers) {
   int response_code = headers->response_code();
-  bool partial_response = enable_range_support_ ? response_code == 206 : false;
-  *partial_content = false;
+  bool partial_content = enable_range_support_ ? response_code == 206 : false;
 
   if (!entry_)
-    return true;
+    return false;
 
   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_response || response_code == 200 || response_code == 304) {
+    if (partial_content || response_code == 200 || response_code == 304) {
       DoomPartialEntry(true);
       mode_ = NONE;
     } else {
       IgnoreRangeRequest();
     }
-    return true;
+    return false;
   }
 
   if (!partial_.get()) {
     // We are not expecting 206 but we may have one.
-    if (partial_response)
+    if (partial_content)
       IgnoreRangeRequest();
 
-    return true;
+    return false;
   }
 
   // TODO(rvargas): Do we need to consider other results here?.
@@ -982,39 +980,28 @@ bool HttpCache::Transaction::ValidatePartialResponse(
 
   if (partial_->IsCurrentRangeCached()) {
     // We asked for "If-None-Match: " so a 206 means a new object.
-    if (partial_response)
+    if (partial_content)
       failure = true;
 
     if (response_code == 304 && partial_->ResponseHeadersOK(headers))
-      return true;
+      return false;
   } else {
     // We asked for "If-Range: " so a 206 means just another range.
-    if (partial_response && partial_->ResponseHeadersOK(headers)) {
-      *partial_content = true;
+    if (partial_content && partial_->ResponseHeadersOK(headers))
       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(false);
+    DoomPartialEntry(true);
     mode_ = NONE;
-    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;
+    return false;
   }
 
   IgnoreRangeRequest();
-  return true;
+  return false;
 }
 
 void HttpCache::Transaction::IgnoreRangeRequest() {
@@ -1276,13 +1263,7 @@ void HttpCache::Transaction::OnNetworkInfoAvailable(int result) {
         new_response->headers->response_code() == 407) {
       auth_response_ = *new_response;
     } else {
-      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;
-      }
+      bool partial_content = ValidatePartialResponse(new_response->headers);
       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 69b785c..d58105f 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -156,11 +156,8 @@ 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 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);
+  // Makes sure that a 206 response is expected.  Returns a network error code.
+  bool ValidatePartialResponse(const HttpResponseHeaders* headers);
 
   // 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 0e693cf..dbcd3b1 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -2503,48 +2503,6 @@ 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 0ef7f6b..f253dc1 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -48,9 +48,7 @@ void PartialData::RestoreHeaders(std::string* headers) const {
   int64 end = byte_range_.IsSuffixByteRange() ?
               byte_range_.suffix_length() : byte_range_.last_byte_position();
 
-  headers->assign(extra_headers_);
-  if (byte_range_.IsValid())
-    AddRangeHeader(current_range_start_, end, headers);
+  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 3a30e0a..588e3e5 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 headers, by appending the byte range to the headers
-  // provided to SetHeaders().
+  // Restores the byte-range header that was removed during Init(), by appending
+  // the data to the provided |headers|.
   void RestoreHeaders(std::string* headers) const;
 
   // Builds the required |headers| to perform the proper cache validation for