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