[go: nahoru, domu]

Reland "HttpCache: fix troubles trying to do ranges with empty bodies"

This is a reland of commit bf289d66c255355a5524d5a2e7583e46135b2c5a

Original change's description:
> HttpCache: fix troubles trying to do ranges with empty bodies
>
> In FixResponseHeaders(), don't try to construct a Range: header when we have an empty body, since that can't represent an empty interval, just remove it and add a Content-Length:0 instead. Also only convert 206s to 200, not other things.
>
> In ShouldValidateCache(), make sure to treat empty body as EOF.
>
> Bug: 1375128, 813061
> Change-Id: I10a50cdf468c17c97b95eb91c09cc43bd8dad262
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4210436
> Commit-Queue: Maks Orlovich <morlovich@chromium.org>
> Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1103284}

Bug: 1375128, 813061
Change-Id: I98428c995bc7b3824f0e41dcb14cd0955237042d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4472488
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Auto-Submit: Adam Rice <ricea@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1135321}
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index e3f5577..ebe25df 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -9126,6 +9126,47 @@
   RemoveMockTransaction(&transaction);
 }
 
+// Similar to UnknownRangeGET_2, except that the resource size is empty.
+// Regression test for crbug.com/813061, and probably https://crbug.com/1375128
+TEST_F(HttpCacheTest, UnknownRangeGET_3) {
+  MockHttpCache cache;
+  std::string headers;
+
+  ScopedMockTransaction transaction(kSimpleGET_Transaction);
+  transaction.response_headers =
+      "Cache-Control: max-age=10000\n"
+      "Content-Length: 0\n",
+  transaction.data = "";
+  transaction.test_mode = TEST_MODE_SYNC_CACHE_START |
+                          TEST_MODE_SYNC_CACHE_READ |
+                          TEST_MODE_SYNC_CACHE_WRITE;
+
+  // Write the empty resource to the cache.
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  EXPECT_EQ(
+      "HTTP/1.1 200 OK\nCache-Control: max-age=10000\nContent-Length: 0\n",
+      headers);
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Make sure we are done with the previous transaction.
+  base::RunLoop().RunUntilIdle();
+
+  // Write and read from the cache. This used to trigger a DCHECK
+  // (or loop infinitely with it off).
+  transaction.request_headers = "Range: bytes = -20\r\n" EXTRA_HEADER;
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  EXPECT_EQ(
+      "HTTP/1.1 200 OK\nCache-Control: max-age=10000\nContent-Length: 0\n",
+      headers);
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+}
+
 // Tests that receiving Not Modified when asking for an open range doesn't mess
 // up things.
 TEST_F(HttpCacheTest, UnknownRangeGET_304) {
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index 7b3bc39a..f58ef98 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -354,20 +354,21 @@
   if (truncated_)
     return;
 
-  if (byte_range_.IsValid() && success) {
-    headers->UpdateWithNewRange(byte_range_, resource_size_, !sparse_entry_);
-    return;
-  }
-
-  if (byte_range_.IsValid()) {
+  if (!success) {
     headers->ReplaceStatusLine("HTTP/1.1 416 Requested Range Not Satisfiable");
     headers->SetHeader(
         kRangeHeader, base::StringPrintf("bytes 0-0/%" PRId64, resource_size_));
     headers->SetHeader(kLengthHeader, "0");
+    return;
+  }
+
+  if (byte_range_.IsValid() && resource_size_) {
+    headers->UpdateWithNewRange(byte_range_, resource_size_, !sparse_entry_);
   } else {
-    // TODO(rvargas): Is it safe to change the protocol version?
-    headers->ReplaceStatusLine("HTTP/1.1 200 OK");
-    DCHECK_NE(resource_size_, 0);
+    if (headers->response_code() == net::HTTP_PARTIAL_CONTENT) {
+      // TODO(rvargas): Is it safe to change the protocol version?
+      headers->ReplaceStatusLine("HTTP/1.1 200 OK");
+    }
     headers->RemoveHeader(kRangeHeader);
     headers->SetHeader(kLengthHeader,
                        base::StringPrintf("%" PRId64, resource_size_));
@@ -433,6 +434,9 @@
 }
 
 int PartialData::GetNextRangeLen() {
+  if (!resource_size_) {
+    return 0;
+  }
   int64_t range_len =
       byte_range_.HasLastBytePosition()
           ? byte_range_.last_byte_position() - current_range_start_ + 1