HttpCache: be more conservative when trying to reconstruct a full resource from partial one.

(With partial resource being a 206, not a truncated one).

Previously we would happily use a first chunk from cache, then fail in panic when validation
of the second chunk failed; this was especially bad for servers whose validation fails to
match for resources for which headers suggest it should.

After this change, in this case, we force a validation of the first chunk, and if it fails
re-fetch the entire resource. This is far from ideal, but we have limitations when it comes
to converting a pre-existing 206 entry to a 200 one that make using somewhat-better-in-this-case
If-Range really painful (and there is no multi-range support, that would be useful for these
sorts of reconstructions, either).

(Heavily affects the lazyload experiment)

Bug: 888742
Change-Id: I7f3f3b40dc1e2a98dd093ac09c98fe43eee5f3bd
Reviewed-on: https://chromium-review.googlesource.com/c/1291595
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606092}

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 96463e1..170f026 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc

@@ -2514,8 +2514,23 @@
     skip_validation = !partial_->initial_validation();
   }
 
+  // If this is the first request (!reading_) of a 206 entry (is_sparse_) that
+  // doesn't actually cover the entire file (which with !reading would require
+  // partial->IsLastRange()), and the user is requesting the whole thing
+  // (!partial_->range_requested()), make sure to validate the first chunk,
+  // since afterwards it will be too late if it's actually out-of-date (or the
+  // server bungles invalidation). This is limited to the whole-file request
+  // as a targeted fix for https://crbug.com/888742 while avoiding extra
+  // requests in other cases, but the problem can occur more generally as well;
+  // it's just a lot less likely with applications actively using ranges.
+  // See https://crbug.com/902724 for the more general case.
+  bool first_read_of_full_from_partial =
+      is_sparse_ && !reading_ &&
+      (partial_ && !partial_->range_requested() && !partial_->IsLastRange());
+
   if (partial_ && (is_sparse_ || truncated_) &&
-      (!partial_->IsCurrentRangeCached() || invalid_range_)) {
+      (!partial_->IsCurrentRangeCached() || invalid_range_ ||
+       first_read_of_full_from_partial)) {
     // 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.

diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 2ec18a5..fcf0857 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc

@@ -473,8 +473,9 @@
   if (!request->extra_headers.GetHeader(HttpRequestHeaders::kRange,
                                         &range_header) ||
       !HttpUtil::ParseRangeHeader(range_header, &ranges) || bad_200_ ||
-      ranges.size() != 1) {
-    // This is not a byte range request. We return 200.
+      ranges.size() != 1 ||
+      (modified_ && request->extra_headers.HasHeader("If-Range"))) {
+    // This is not a byte range request, or a failed If-Range. We return 200.
     response_status->assign("HTTP/1.1 200 OK");
     response_headers->assign("Date: Wed, 28 Nov 2007 09:40:09 GMT");
     response_data->assign("Not a range");
@@ -1505,6 +1506,113 @@
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 }
 
+TEST_F(HttpCacheTest, RangeGET_FullAfterPartial) {
+  MockHttpCache cache;
+
+  // Request a prefix.
+  {
+    ScopedMockTransaction transaction_pre(kRangeGET_TransactionOK);
+    transaction_pre.request_headers = "Range: bytes = 0-9\r\n" EXTRA_HEADER;
+    transaction_pre.data = "rg: 00-09 ";
+    MockHttpRequest request_pre(transaction_pre);
+
+    HttpResponseInfo response_pre;
+    RunTransactionTestWithRequest(cache.http_cache(), transaction_pre,
+                                  request_pre, &response_pre);
+    ASSERT_TRUE(response_pre.headers != nullptr);
+    EXPECT_EQ(206, response_pre.headers->response_code());
+    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 request the full thing, but set validation to fail. This would
+    // previously fail in the middle of data and truncate it; current behavior
+    // restarts it, somewhat wastefully but gets the data back.
+    RangeTransactionServer handler;
+    handler.set_modified(true);
+
+    ScopedMockTransaction transaction_all(kRangeGET_TransactionOK);
+    transaction_all.request_headers = EXTRA_HEADER;
+    transaction_all.data = "Not a range";
+    MockHttpRequest request_all(transaction_all);
+
+    HttpResponseInfo response_all;
+    RunTransactionTestWithRequest(cache.http_cache(), transaction_all,
+                                  request_all, &response_all);
+    ASSERT_TRUE(response_all.headers != nullptr);
+    EXPECT_EQ(200, response_all.headers->response_code());
+    // 1 from previous test, failed validation, and re-try.
+    EXPECT_EQ(3, cache.network_layer()->transaction_count());
+    EXPECT_EQ(1, cache.disk_cache()->open_count());
+    EXPECT_EQ(1, cache.disk_cache()->create_count());
+  }
+}
+
+TEST_F(HttpCacheTest, RangeGET_FullAfterPartialReuse) {
+  MockHttpCache cache;
+
+  // Request a prefix.
+  {
+    ScopedMockTransaction transaction_pre(kRangeGET_TransactionOK);
+    transaction_pre.request_headers = "Range: bytes = 0-9\r\n" EXTRA_HEADER;
+    transaction_pre.data = "rg: 00-09 ";
+    MockHttpRequest request_pre(transaction_pre);
+
+    HttpResponseInfo response_pre;
+    RunTransactionTestWithRequest(cache.http_cache(), transaction_pre,
+                                  request_pre, &response_pre);
+    ASSERT_TRUE(response_pre.headers != nullptr);
+    EXPECT_EQ(206, response_pre.headers->response_code());
+    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 request the full thing, revalidating successfully, so the full
+    // file gets stored via a sparse-entry.
+    ScopedMockTransaction transaction_all(kRangeGET_TransactionOK);
+    transaction_all.request_headers = EXTRA_HEADER;
+    transaction_all.data =
+        "rg: 00-09 rg: 10-19 rg: 20-29 rg: 30-39 rg: 40-49"
+        " rg: 50-59 rg: 60-69 rg: 70-79 ";
+    MockHttpRequest request_all(transaction_all);
+
+    HttpResponseInfo response_all;
+    RunTransactionTestWithRequest(cache.http_cache(), transaction_all,
+                                  request_all, &response_all);
+    ASSERT_TRUE(response_all.headers != nullptr);
+    EXPECT_EQ(200, response_all.headers->response_code());
+    // 1 from previous test, validation, and second chunk
+    EXPECT_EQ(3, cache.network_layer()->transaction_count());
+    EXPECT_EQ(1, cache.disk_cache()->open_count());
+    EXPECT_EQ(1, cache.disk_cache()->create_count());
+  }
+
+  {
+    // Grab it again, should not need re-validation.
+    ScopedMockTransaction transaction_all2(kRangeGET_TransactionOK);
+    transaction_all2.request_headers = EXTRA_HEADER;
+    transaction_all2.data =
+        "rg: 00-09 rg: 10-19 rg: 20-29 rg: 30-39 rg: 40-49"
+        " rg: 50-59 rg: 60-69 rg: 70-79 ";
+    MockHttpRequest request_all2(transaction_all2);
+
+    HttpResponseInfo response_all2;
+    RunTransactionTestWithRequest(cache.http_cache(), transaction_all2,
+                                  request_all2, &response_all2);
+    ASSERT_TRUE(response_all2.headers != nullptr);
+    EXPECT_EQ(200, response_all2.headers->response_code());
+
+    // Only one more cache open, no new network traffic.
+    EXPECT_EQ(3, cache.network_layer()->transaction_count());
+    EXPECT_EQ(2, cache.disk_cache()->open_count());
+    EXPECT_EQ(1, cache.disk_cache()->create_count());
+  }
+}
+
 // Tests that we can have parallel validation on range requests.
 TEST_F(HttpCacheTest, RangeGET_ParallelValidationNoMatch) {
   MockHttpCache cache;

diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index c485fb0b..e4b7a6b 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc

@@ -37,6 +37,7 @@
       cached_start_(0),
       cached_min_len_(0),
       resource_size_(0),
+      range_requested_(false),
       range_present_(false),
       final_range_(false),
       sparse_entry_(true),
@@ -48,8 +49,11 @@
 
 bool PartialData::Init(const HttpRequestHeaders& headers) {
   std::string range_header;
-  if (!headers.GetHeader(HttpRequestHeaders::kRange, &range_header))
+  if (!headers.GetHeader(HttpRequestHeaders::kRange, &range_header)) {
+    range_requested_ = false;
     return false;
+  }
+  range_requested_ = true;
 
   std::vector<HttpByteRange> ranges;
   if (!HttpUtil::ParseRangeHeader(range_header, &ranges) || ranges.size() != 1)

diff --git a/net/http/partial_data.h b/net/http/partial_data.h
index 9120ff4..0ca5926 100644
--- a/net/http/partial_data.h
+++ b/net/http/partial_data.h

@@ -125,6 +125,8 @@
 
   bool initial_validation() const { return initial_validation_; }
 
+  bool range_requested() const { return range_requested_; }
+
  private:
   // Returns the length to use when scanning the cache.
   int GetNextRangeLen();
@@ -150,6 +152,7 @@
   HttpByteRange byte_range_;  // The range requested by the user.
   // The clean set of extra headers (no ranges).
   HttpRequestHeaders extra_headers_;
+  bool range_requested_;  // ###
   bool range_present_;  // True if next range entry is already stored.
   bool final_range_;
   bool sparse_entry_;