HttpCache: don't mess up status on range requests getting a non-200/206

We would sometimes do it to a cached 301, and then get really confused trying to read
a body that wasn't there. 200 and 206 responses are the only in-caches ones (wire could
also have a 304) which could possibly result in a 206 to our client, so others should just
have range handling dropped; we do that already for things gotten on the wire but not
from cache.

Bug: 1015829
Change-Id: I6ba127aec8e88a7cbf72d7a87b94e5e7f10a3973
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899890Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Commit-Queue: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714995}

net/http/http_cache_transaction.cc

@@ -2068,8 +2068,14 @@ int HttpCache::Transaction::DoPartialHeadersReceived() {
 
   if (partial_ && mode_ != NONE && !reading_) {
     // We are about to return the headers for a byte-range request to the user,
-    // so let's fix them.
-    partial_->FixResponseHeaders(response_.headers.get(), true);
+    // so let's fix them. This only makes sense if the response is something
+    // that can be turned into a 206, of course.
+    if (response_.headers->response_code() == 200 ||
+        response_.headers->response_code() == 206) {
+      partial_->FixResponseHeaders(response_.headers.get(), true);
+    } else {
+      partial_.reset();
+    }
   }
   TransitionToState(STATE_FINISH_HEADERS);
   return OK;

net/http/http_cache_unittest.cc

@@ -391,12 +391,14 @@ class RangeTransactionServer {
     not_modified_ = false;
     modified_ = false;
     bad_200_ = false;
+    redirect_ = false;
     length_ = 80;
   }
   ~RangeTransactionServer() {
     not_modified_ = false;
     modified_ = false;
     bad_200_ = false;
+    redirect_ = false;
     length_ = 80;
   }
 
@@ -412,6 +414,9 @@ class RangeTransactionServer {
   // Sets how long the resource is. (Default is 80)
   void set_length(int64_t length) { length_ = length; }
 
+  // Sets whether to return a 301 instead of normal return.
+  void set_redirect(bool redirect) { redirect_ = redirect; }
+
   // Other than regular range related behavior (and the flags mentioned above),
   // the server reacts to requests headers like so:
   //   X-Require-Mock-Auth -> return 401.
@@ -428,12 +433,14 @@ class RangeTransactionServer {
   static bool not_modified_;
   static bool modified_;
   static bool bad_200_;
+  static bool redirect_;
   static int64_t length_;
   DISALLOW_COPY_AND_ASSIGN(RangeTransactionServer);
 };
 bool RangeTransactionServer::not_modified_ = false;
 bool RangeTransactionServer::modified_ = false;
 bool RangeTransactionServer::bad_200_ = false;
+bool RangeTransactionServer::redirect_ = false;
 int64_t RangeTransactionServer::length_ = 80;
 
 // A dummy extra header that must be preserved on a given request.
@@ -472,6 +479,13 @@ void RangeTransactionServer::RangeHandler(const HttpRequestInfo* request,
     return;
   }
 
+  if (redirect_) {
+    response_status->assign("HTTP/1.1 301 Moved Permanently");
+    response_headers->assign("Location: /elsewhere\nContent-Length: 5");
+    response_data->assign("12345");
+    return;
+  }
+
   if (not_modified_) {
     response_status->assign("HTTP/1.1 304 Not Modified");
     response_data->clear();
@@ -2785,6 +2799,77 @@ TEST_F(HttpCacheTest, RangeGET_ParallelValidationRestartDoneHeaders) {
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 }
 
+// A test of doing a range request to a cached 301 response
+TEST_F(HttpCacheTest, RangeGET_CachedRedirect) {
+  RangeTransactionServer handler;
+  handler.set_redirect(true);
+
+  MockHttpCache cache;
+  ScopedMockTransaction transaction(kRangeGET_TransactionOK);
+  transaction.request_headers = "Range: bytes = 0-\r\n" EXTRA_HEADER;
+  transaction.status = "HTTP/1.1 301 Moved Permanently";
+  transaction.response_headers = "Location: /elsewhere\nContent-Length:5";
+  transaction.data = "12345";
+  MockHttpRequest request(transaction);
+
+  TestCompletionCallback callback;
+
+  // Write to the cache.
+  {
+    std::unique_ptr<HttpTransaction> trans;
+    ASSERT_THAT(cache.CreateTransaction(&trans), IsOk());
+
+    int rv = trans->Start(&request, callback.callback(), NetLogWithSource());
+    if (rv == ERR_IO_PENDING)
+      rv = callback.WaitForResult();
+    ASSERT_THAT(rv, IsOk());
+
+    const HttpResponseInfo* info = trans->GetResponseInfo();
+    ASSERT_TRUE(info);
+
+    EXPECT_EQ(info->headers->response_code(), 301);
+
+    std::string location;
+    info->headers->EnumerateHeader(nullptr, "Location", &location);
+    EXPECT_EQ(location, "/elsewhere");
+
+    ReadAndVerifyTransaction(trans.get(), transaction);
+  }
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Active entries in the cache are not retired synchronously. Make
+  // sure the next run hits the MockHttpCache and open_count is
+  // correct.
+  base::RunLoop().RunUntilIdle();
+
+  // Read from the cache.
+  {
+    std::unique_ptr<HttpTransaction> trans;
+    ASSERT_THAT(cache.CreateTransaction(&trans), IsOk());
+
+    int rv = trans->Start(&request, callback.callback(), NetLogWithSource());
+    if (rv == ERR_IO_PENDING)
+      rv = callback.WaitForResult();
+    ASSERT_THAT(rv, IsOk());
+
+    const HttpResponseInfo* info = trans->GetResponseInfo();
+    ASSERT_TRUE(info);
+
+    EXPECT_EQ(info->headers->response_code(), 301);
+
+    std::string location;
+    info->headers->EnumerateHeader(nullptr, "Location", &location);
+    EXPECT_EQ(location, "/elsewhere");
+
+    trans->DoneReading();
+  }
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+}
+
 // A transaction that fails to validate an entry, while attempting to write
 // the response, should still get data to its consumer even if the attempt to
 // create a new entry fails.