Http cache: Properly reset the status related to partial requests on restart.

DoRestartPartialRequest handles failures to add validation headers to a byte
range request. Part of that process requires restoring the byte range headers
present on the original request.

This CL adds that mnissing line and slightly refactors that restart logic to
make more evident what has to be done.

BUG=442318
TEST=net_unittests

Review URL: https://codereview.chromium.org/836063002

Cr-Commit-Position: refs/heads/master@{#310381}

3 files changed, 104 insertions, 11 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 50e9da1..43db2cd 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -2751,10 +2751,8 @@ bool HttpCache::Transaction::ValidatePartialResponse() {
   if (failure) {
     // We cannot truncate this entry, it has to be deleted.
     UpdateTransactionPattern(PATTERN_NOT_COVERED);
-    bool is_sparse = is_sparse_;
-    DoomPartialEntry(false);
     mode_ = NONE;
-    if (is_sparse || truncated_) {
+    if (is_sparse_ || truncated_) {
       // There was something cached to start with, either sparsed data (206), or
       // a truncated 200, which means that we probably modified the request,
       // adding a byte range or modifying the range requested by the caller.
@@ -2762,14 +2760,12 @@ bool HttpCache::Transaction::ValidatePartialResponse() {
         // We have not returned anything to the caller yet so it should be safe
         // to issue another network request, this time without us messing up the
         // headers.
-        partial_->RestoreHeaders(&custom_request_->extra_headers);
-        partial_.reset();
-        truncated_ = false;
+        ResetPartialState(true);
         return false;
       }
       LOG(WARNING) << "Failed to revalidate partial entry";
     }
-    partial_.reset();
+    DoomPartialEntry(true);
     return true;
   }
 
@@ -2996,6 +2992,7 @@ void HttpCache::Transaction::DoomPartialEntry(bool delete_object) {
   cache_->DoneWithEntry(entry_, this, false);
   entry_ = NULL;
   is_sparse_ = false;
+  truncated_ = false;
   if (delete_object)
     partial_.reset(NULL);
 }
@@ -3025,15 +3022,30 @@ int HttpCache::Transaction::DoPartialCacheReadCompleted(int 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.
   net_log_.AddEvent(NetLog::TYPE_HTTP_CACHE_RESTART_PARTIAL_REQUEST);
-  DoomPartialEntry(!range_requested_);
+
+  // WRITE + Doom + STATE_INIT_ENTRY == STATE_CREATE_ENTRY (without an attempt
+  // to Doom the entry again).
   mode_ = WRITE;
-  truncated_ = false;
-  next_state_ = STATE_INIT_ENTRY;
+  ResetPartialState(!range_requested_);
+  next_state_ = STATE_CREATE_ENTRY;
   return OK;
 }
 
+void HttpCache::Transaction::ResetPartialState(bool delete_object) {
+  partial_->RestoreHeaders(&custom_request_->extra_headers);
+  DoomPartialEntry(delete_object);
+
+  if (!delete_object) {
+    // The simplest way to re-initialize partial_ is to create a new object.
+    partial_.reset(new PartialData());
+    if (partial_->Init(request_->extra_headers))
+      partial_->SetHeaders(custom_request_->extra_headers);
+    else
+      partial_.reset();
+  }
+}
+
 void HttpCache::Transaction::ResetNetworkTransaction() {
   DCHECK(!old_network_trans_load_timing_);
   DCHECK(network_trans_);
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 0b4792b..9f486e9 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -386,6 +386,10 @@ class HttpCache::Transaction : public HttpTransaction {
   // between the byte range request and the cached entry.
   int DoRestartPartialRequest();
 
+  // Resets the relavant internal state to remove traces of internal processing
+  // related to range requests. Deletes |partial_| if |delete_object| is true.
+  void ResetPartialState(bool delete_object);
+
   // Resets |network_trans_|, which must be non-NULL.  Also updates
   // |old_network_trans_load_timing_|, which must be NULL when this is called.
   void ResetNetworkTransaction();
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index d38d455..471766c 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -3827,6 +3827,83 @@ TEST(HttpCache, RangeGET_NoValidation_LogsRestart) {
       log, net::NetLog::TYPE_HTTP_CACHE_RESTART_PARTIAL_REQUEST));
 }
 
+// Tests that a regular request (no range) with a sparse entry that doesn't have
+// strong validators results in a full response.
+TEST(HttpCache, GET_NoValidation) {
+  MockHttpCache cache;
+  std::string headers;
+
+  // Write to the cache (40-49).
+  ScopedMockTransaction transaction(kRangeGET_TransactionOK);
+  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 the cached data is not used.
+  // Don't ask for a range. The cache will attempt to use the cached data but
+  // should discard it as it cannot be validated. A regular request should go
+  // to the server and a new entry should be created.
+  transaction.request_headers = EXTRA_HEADER;
+  transaction.data = "Not a range";
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  EXPECT_EQ(0U, headers.find("HTTP/1.1 200 OK\n"));
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+
+  // The last response was saved.
+  RunTransactionTest(cache.http_cache(), transaction);
+  EXPECT_EQ(3, cache.network_layer()->transaction_count());
+  EXPECT_EQ(2, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+}
+
+// Verifies that when asking for a range that would require the cache to modify
+// the range to ask, the actual server request matches the user's one.
+TEST(HttpCache, RangeGET_NoValidation2) {
+  MockHttpCache cache;
+  std::string headers;
+
+  // Write to the cache (40-49).
+  ScopedMockTransaction transaction(kRangeGET_TransactionOK);
+  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 the cached data is not used.
+  // Ask for a range that extends before and after the cached data so that the
+  // cache would normally mix data from three sources. After deleting the entry,
+  // the response will come from a single network request.
+  transaction.request_headers = "Range: bytes = 20-59\r\n" EXTRA_HEADER;
+  transaction.data = "rg: 20-29 rg: 30-39 rg: 40-49 rg: 50-59 ";
+  transaction.response_headers = kRangeGET_TransactionOK.response_headers;
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  Verify206Response(headers, 20, 59);
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+
+  // The last response was saved.
+  RunTransactionTest(cache.http_cache(), transaction);
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(2, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+}
+
 // Tests that we cache partial responses that lack content-length.
 TEST(HttpCache, RangeGET_NoContentLength) {
   MockHttpCache cache;