service worker: s13n: remove special range request handling

Consider the scenario:
1. Page makes a range request
2. Service worker responds with 200 status and a big blob.

Previously, S13nServiceWorker loading code returned to the page a 200
status and attempted to return only the requested bytes from the blob.
This was incorrect: if a 200 status response is returned, all the
bytes should be present. Furthermore, since the service worker itself
didn't return a 206 status response, it is probably not
correct for the loading code to attempt to convert it to one.

This CL changes the code to return exactly what the service worker
returned.

Bug: 892227
Change-Id: Id032ba73ae0a61b300f2d45c1cbf2ba041481788
Reviewed-on: https://chromium-review.googlesource.com/c/1267057Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597608}

content/browser/service_worker/service_worker_navigation_loader.cc

@@ -433,7 +433,7 @@ void ServiceWorkerNavigationLoader::StartResponse(
     body_as_blob_.Bind(std::move(response->blob->blob));
     mojo::ScopedDataPipeConsumerHandle data_pipe;
     int error = ServiceWorkerLoaderHelpers::ReadBlobResponseBody(
-        &body_as_blob_, response->blob->size, resource_request_.headers,
+        &body_as_blob_, response->blob->size,
         base::BindOnce(&ServiceWorkerNavigationLoader::OnBlobReadingComplete,
                        weak_factory_.GetWeakPtr()),
         &data_pipe);

content/common/service_worker/service_worker_loader_helpers.cc

@@ -44,71 +44,6 @@ class BlobCompleteCaller : public blink::mojom::BlobReaderClient {
   BlobCompleteCallback callback_;
 };
 
-// Sets |has_range_out| to true if |headers| specify a single range request, and
-// |offset_out| and |length_out| to the range. If the range has an unbounded
-// end, |length_out| is set to uint64_t's max value. Returns true on valid input
-// (regardless of |has_range_out|), and false if there is more than one range or
-// if the bounds overflow.
-bool ExtractSinglePartHttpRange(const net::HttpRequestHeaders& headers,
-                                bool* has_range_out,
-                                uint64_t* offset_out,
-                                uint64_t* length_out) {
-  std::string range_header;
-  *has_range_out = false;
-  if (!headers.GetHeader(net::HttpRequestHeaders::kRange, &range_header))
-    return true;
-
-  std::vector<net::HttpByteRange> ranges;
-  if (!net::HttpUtil::ParseRangeHeader(range_header, &ranges))
-    return true;
-
-  // Multi-part (or invalid) ranges are not supported.
-  if (ranges.size() != 1)
-    return false;
-
-  // Safely parse the single range to our more-sane output format.
-  *has_range_out = true;
-  const net::HttpByteRange& byte_range = ranges[0];
-
-  // The first byte must be non-negative.
-  if (byte_range.first_byte_position() < 0)
-    return false;
-
-  // The last byte can be -1 to specify unbounded end.
-  if (byte_range.last_byte_position() == -1) {
-    // The range [0, -1] is the same as the entire range (no range specified).
-    if (byte_range.first_byte_position() == 0 &&
-        byte_range.last_byte_position() == -1) {
-      *has_range_out = false;
-      return true;
-    }
-
-    // Otherwise, return the range with unbounded end.
-    *length_out = std::numeric_limits<uint64_t>::max();
-    return true;
-  }
-
-  // The last byte must be non-negative.
-  if (byte_range.last_byte_position() < 0)
-    return false;
-
-  uint64_t first_byte_position =
-      static_cast<uint64_t>(byte_range.first_byte_position());
-  uint64_t last_byte_position =
-      static_cast<uint64_t>(byte_range.last_byte_position());
-
-  base::CheckedNumeric<uint64_t> length = last_byte_position;
-  length -= first_byte_position;
-  length += 1;
-
-  if (!length.IsValid())
-    return false;
-
-  *offset_out = static_cast<uint64_t>(byte_range.first_byte_position());
-  *length_out = length.ValueOrDie();
-  return true;
-}
-
 }  // namespace
 
 // static
@@ -199,31 +134,18 @@ ServiceWorkerLoaderHelpers::ComputeRedirectInfo(
 int ServiceWorkerLoaderHelpers::ReadBlobResponseBody(
     blink::mojom::BlobPtr* blob,
     uint64_t blob_size,
-    const net::HttpRequestHeaders& headers,
     base::OnceCallback<void(int)> on_blob_read_complete,
     mojo::ScopedDataPipeConsumerHandle* handle_out) {
-  bool byte_range_set = false;
-  uint64_t offset = 0;
-  uint64_t length = 0;
-  // We don't support multiple range requests in one single URL request,
-  // because we need to do multipart encoding here.
-  // TODO(falken): Support multipart byte range requests.
-  if (!ExtractSinglePartHttpRange(headers, &byte_range_set, &offset, &length))
-    return net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
-
+  // TODO(falken): Change to CreateDataPipe() and return an error if allocation
+  // failed.
   mojo::DataPipe data_pipe(blink::BlobUtils::GetDataPipeCapacity(blob_size));
   blink::mojom::BlobReaderClientPtr blob_reader_client;
   mojo::MakeStrongBinding(
       std::make_unique<BlobCompleteCaller>(std::move(on_blob_read_complete)),
       mojo::MakeRequest(&blob_reader_client));
 
-  if (byte_range_set) {
-    (*blob)->ReadRange(offset, length, std::move(data_pipe.producer_handle),
-                       std::move(blob_reader_client));
-  } else {
-    (*blob)->ReadAll(std::move(data_pipe.producer_handle),
-                     std::move(blob_reader_client));
-  }
+  (*blob)->ReadAll(std::move(data_pipe.producer_handle),
+                   std::move(blob_reader_client));
   *handle_out = std::move(data_pipe.consumer_handle);
   return net::OK;
 }

content/common/service_worker/service_worker_loader_helpers.h

@@ -40,14 +40,13 @@ class ServiceWorkerLoaderHelpers {
       const network::ResourceRequest& original_request,
       const network::ResourceResponseHead& response_head);
 
-  // Reads |blob| using the range in |headers| (if any), writing into
-  // |handle_out|. Calls |on_blob_read_complete| when done or if an error
-  // occurred. Returns a net error code if the inputs were invalid and reading
-  // couldn't start. In that case |on_blob_read_complete| isn't called.
+  // Reads |blob| into |handle_out|. Calls |on_blob_read_complete| when done or
+  // if an error occurred. Currently this always returns net::OK but
+  // the plan is to return an error if reading couldn't start, in
+  // which case |on_blob_read_complete| isn't called.
   static int ReadBlobResponseBody(
       blink::mojom::BlobPtr* blob,
       uint64_t blob_size,
-      const net::HttpRequestHeaders& headers,
       base::OnceCallback<void(int net_error)> on_blob_read_complete,
       mojo::ScopedDataPipeConsumerHandle* handle_out);
 };

content/renderer/service_worker/service_worker_subresource_loader.cc

@@ -586,7 +586,7 @@ int ServiceWorkerSubresourceLoader::StartBlobReading(
   UMA_HISTOGRAM_TIMES("ServiceWorker.SubresourceStartBlobReadingDelay", delay);
 
   return ServiceWorkerLoaderHelpers::ReadBlobResponseBody(
-      &body_as_blob_, body_as_blob_size_, resource_request_.headers,
+      &body_as_blob_, body_as_blob_size_,
       base::BindOnce(&ServiceWorkerSubresourceLoader::OnBlobReadingComplete,
                      weak_factory_.GetWeakPtr()),
       body_pipe);

content/renderer/service_worker/service_worker_subresource_loader_unittest.cc

@@ -11,6 +11,7 @@
 
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "content/common/service_worker/service_worker_container.mojom.h"
@@ -190,6 +191,7 @@ class FakeControllerServiceWorker : public mojom::ControllerServiceWorker {
     redirect_location_header_ = redirect_location_header;
   }
 
+  // Tells this controller to respond to fetch events with a blob response body.
   void RespondWithBlob(base::Optional<std::vector<uint8_t>> metadata,
                        std::string body) {
     response_mode_ = ResponseMode::kBlob;
@@ -201,6 +203,14 @@ class FakeControllerServiceWorker : public mojom::ControllerServiceWorker {
         mojo::MakeRequest(&blob_body_->blob));
   }
 
+  // Tells this controller to respond to fetch events with a 206 partial
+  // response, returning a blob composed of the requested bytes of |body|
+  // according to the request headers.
+  void RespondWithBlobRange(std::string body) {
+    response_mode_ = ResponseMode::kBlobRange;
+    blob_range_body_ = body;
+  }
+
   void ReadRequestBody(std::string* out_string) {
     ASSERT_TRUE(request_body_);
     std::vector<network::DataElement>* elements =
@@ -270,6 +280,44 @@ class FakeControllerServiceWorker : public mojom::ControllerServiceWorker {
             blink::mojom::ServiceWorkerEventStatus::COMPLETED,
             base::TimeTicks());
         break;
+
+      case ResponseMode::kBlobRange: {
+        // Parse the Range header.
+        std::string range_header;
+        std::vector<net::HttpByteRange> ranges;
+        ASSERT_TRUE(params->request.headers.GetHeader(
+            net::HttpRequestHeaders::kRange, &range_header));
+        ASSERT_TRUE(net::HttpUtil::ParseRangeHeader(range_header, &ranges));
+        ASSERT_EQ(1u, ranges.size());
+        ASSERT_TRUE(ranges[0].ComputeBounds(blob_range_body_.size()));
+        const net::HttpByteRange& range = ranges[0];
+
+        // Build a Blob composed of the requested bytes from |blob_range_body_|.
+        size_t start = static_cast<size_t>(range.first_byte_position());
+        size_t end = static_cast<size_t>(range.last_byte_position());
+        size_t size = end - start + 1;
+        std::string body = blob_range_body_.substr(start, size);
+        auto blob = blink::mojom::SerializedBlob::New();
+        blob->uuid = "dummy-blob-uuid";
+        blob->size = size;
+        mojo::MakeStrongBinding(std::make_unique<FakeBlob>(base::nullopt, body),
+                                mojo::MakeRequest(&blob->blob));
+
+        // Respond with a 206 response.
+        auto response = OkResponse(std::move(blob));
+        response->status_code = 206;
+        response->headers.emplace(
+            "Content-Range", base::StringPrintf("bytes %zu-%zu/%zu", start, end,
+                                                blob_range_body_.size()));
+        response_callback->OnResponse(
+            std::move(response),
+            blink::mojom::ServiceWorkerFetchEventTiming::New());
+        std::move(callback).Run(
+            blink::mojom::ServiceWorkerEventStatus::COMPLETED,
+            base::TimeTicks::Now());
+        break;
+      }
+
       case ResponseMode::kFallbackResponse:
         response_callback->OnFallback(
             blink::mojom::ServiceWorkerFetchEventTiming::New());
@@ -320,6 +368,7 @@ class FakeControllerServiceWorker : public mojom::ControllerServiceWorker {
     kAbort,
     kStream,
     kBlob,
+    kBlobRange,
     kFallbackResponse,
     kErrorResponse,
     kRedirectResponse
@@ -339,6 +388,9 @@ class FakeControllerServiceWorker : public mojom::ControllerServiceWorker {
   // For ResponseMode::kBlob.
   blink::mojom::SerializedBlobPtr blob_body_;
 
+  // For ResponseMode::kBlobRange.
+  std::string blob_range_body_;
+
   // For ResponseMode::kRedirectResponse
   std::string redirect_location_header_;
 
@@ -539,6 +591,30 @@ class ServiceWorkerSubresourceLoaderTest : public ::testing::Test {
     // cover this case in fetch-event.https.html.
   }
 
+  // Performs a range request using |range_header| and returns the resulting
+  // client after completion.
+  std::unique_ptr<network::TestURLLoaderClient> DoRangeRequest(
+      const std::string& range_header) {
+    network::mojom::URLLoaderFactoryPtr factory =
+        CreateSubresourceLoaderFactory();
+    network::ResourceRequest request =
+        CreateRequest(GURL("https://www.example.com/big-file"));
+    request.headers.SetHeader("Range", range_header);
+    network::mojom::URLLoaderPtr loader;
+    std::unique_ptr<network::TestURLLoaderClient> client;
+    StartRequest(factory, request, &loader, &client);
+    client->RunUntilComplete();
+    return client;
+  }
+
+  std::string TakeResponseBody(network::TestURLLoaderClient* client) {
+    std::string body;
+    EXPECT_TRUE(client->response_body().is_valid());
+    EXPECT_TRUE(
+        mojo::BlockingCopyToString(client->response_body_release(), &body));
+    return body;
+  }
+
   TestBrowserThreadBundle thread_bundle_;
   scoped_refptr<network::SharedURLLoaderFactory> loader_factory_;
   scoped_refptr<ControllerServiceWorkerConnector> connector_;
@@ -1212,5 +1288,73 @@ TEST_F(ServiceWorkerSubresourceLoaderTest, FallbackWithRequestBody_DataPipe) {
   RunFallbackWithRequestBodyTest(std::move(request_body), kData);
 }
 
+// Test a range request that the service worker responds to with a 200
+// (non-ranged) response. The client should get the entire response as-is from
+// the service worker.
+TEST_F(ServiceWorkerSubresourceLoaderTest, RangeRequest_200Response) {
+  // Construct the Blob to respond with.
+  const std::string kResponseBody = "Here is sample text for the Blob.";
+  fake_controller_.RespondWithBlob(base::nullopt, kResponseBody);
+
+  // Perform the request.
+  std::unique_ptr<network::TestURLLoaderClient> client =
+      DoRangeRequest("bytes=5-13");
+  EXPECT_EQ(net::OK, client->completion_status().error_code);
+
+  // Test the response.
+  const network::ResourceResponseHead& info = client->response_head();
+  ExpectResponseInfo(info, *CreateResponseInfoFromServiceWorker());
+  EXPECT_EQ(33, info.content_length);
+  EXPECT_FALSE(info.headers->HasHeader("Content-Range"));
+  EXPECT_EQ(kResponseBody, TakeResponseBody(client.get()));
+}
+
+// Test a range request that the service worker responds to with a 206 ranged
+// response. The client should get the partial response as-is from the service
+// worker.
+TEST_F(ServiceWorkerSubresourceLoaderTest, RangeRequest_206Response) {
+  // Tell the controller to respond with a 206 response.
+  const std::string kResponseBody = "Here is sample text for the Blob.";
+  fake_controller_.RespondWithBlobRange(kResponseBody);
+
+  // Perform the request.
+  std::unique_ptr<network::TestURLLoaderClient> client =
+      DoRangeRequest("bytes=5-13");
+  EXPECT_EQ(net::OK, client->completion_status().error_code);
+
+  // Test the response.
+  const network::ResourceResponseHead& info = client->response_head();
+  EXPECT_EQ(206, info.headers->response_code());
+  std::string range;
+  ASSERT_TRUE(info.headers->GetNormalizedHeader("Content-Range", &range));
+  EXPECT_EQ("bytes 5-13/33", range);
+  EXPECT_EQ(9, info.content_length);
+  EXPECT_EQ("is sample", TakeResponseBody(client.get()));
+}
+
+// Test a range request that the service worker responds to with a 206 ranged
+// response. The requested range has an unbounded end. The client should get the
+// partial response as-is from the service worker.
+TEST_F(ServiceWorkerSubresourceLoaderTest,
+       RangeRequest_UnboundedRight_206Response) {
+  // Tell the controller to respond with a 206 response.
+  const std::string kResponseBody = "Here is sample text for the Blob.";
+  fake_controller_.RespondWithBlobRange(kResponseBody);
+
+  // Perform the request.
+  std::unique_ptr<network::TestURLLoaderClient> client =
+      DoRangeRequest("bytes=5-");
+  EXPECT_EQ(net::OK, client->completion_status().error_code);
+
+  // Test the response.
+  const network::ResourceResponseHead& info = client->response_head();
+  EXPECT_EQ(206, info.headers->response_code());
+  std::string range;
+  ASSERT_TRUE(info.headers->GetNormalizedHeader("Content-Range", &range));
+  EXPECT_EQ("bytes 5-32/33", range);
+  EXPECT_EQ(28, info.content_length);
+  EXPECT_EQ("is sample text for the Blob.", TakeResponseBody(client.get()));
+}
+
 }  // namespace service_worker_subresource_loader_unittest
 }  // namespace content