Service worker: match OnStartLoadingResponseBody with OnReceiveResponse.

Some URLLoaders aren't always sending a response's body datapipe to their
client after sending URLLoaderClient::OnReceiveResponse(response_head).
It even happens to send URLLoaderClient::OnComplete(net::OK) after that.

Most of the time, they do not send a datapipe, because the response's
body is empty.

The goal is to align URLLoaders so that they will always send a data
pipe by using OnStartLoadingResponseBody(response_body).

This CL is about ServiceWorker's URLLoaders.

This CL is a prerequisite for:
https://chromium-review.googlesource.com/c/chromium/src/+/1172290

Bug: 826868, 831155
Change-Id: Ib28fc2067240d611ec149099b5365f70610a9369
Reviewed-on: https://chromium-review.googlesource.com/c/1323109
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607273}

content/browser/service_worker/service_worker_navigation_loader.cc

@@ -127,11 +127,11 @@ void ServiceWorkerNavigationLoader::FallbackToNetwork() {
   // The URLJobWrapper only calls this if this loader never intercepted the
   // request. Fallback to network after interception uses |fallback_callback_|
   // instead.
-  DCHECK_EQ(status_, Status::kNotStarted);
   DCHECK_EQ(response_type_, ResponseType::NOT_DETERMINED);
-
   response_type_ = ResponseType::FALLBACK_TO_NETWORK;
+
   TransitionToStatus(Status::kCompleted);
+
   std::move(loader_callback_).Run({});
 }
 
@@ -249,6 +249,20 @@ void ServiceWorkerNavigationLoader::CommitResponseHeaders() {
   url_loader_client_->OnReceiveResponse(response_head_);
 }
 
+void ServiceWorkerNavigationLoader::CommitResponseBody(
+    mojo::ScopedDataPipeConsumerHandle response_body) {
+  TransitionToStatus(Status::kSentBody);
+  url_loader_client_->OnStartLoadingResponseBody(std::move(response_body));
+}
+
+void ServiceWorkerNavigationLoader::CommitResponseBodyEmpty() {
+  TransitionToStatus(Status::kSentBody);
+  mojo::DataPipe pipe;
+  url_loader_client_->OnStartLoadingResponseBody(
+      std::move(pipe.consumer_handle));
+  // pipe.producer_handle is closed here.
+}
+
 void ServiceWorkerNavigationLoader::CommitCompleted(int error_code) {
   TRACE_EVENT_WITH_FLOW1("ServiceWorker",
                          "ServiceWorkerNavigationLoader::CommitCompleted", this,
@@ -414,8 +428,7 @@ void ServiceWorkerNavigationLoader::StartResponse(
                            "result", "stream response");
     stream_waiter_ = std::make_unique<StreamWaiter>(
         this, std::move(version), std::move(body_as_stream->callback_request));
-    url_loader_client_->OnStartLoadingResponseBody(
-        std::move(body_as_stream->stream));
+    CommitResponseBody(std::move(body_as_stream->stream));
     // StreamWaiter will call CommitCompleted() when done.
     return;
   }
@@ -431,6 +444,7 @@ void ServiceWorkerNavigationLoader::StartResponse(
                        weak_factory_.GetWeakPtr()),
         &data_pipe);
     if (error != net::OK) {
+      CommitResponseBodyEmpty();
       CommitCompleted(error);
       return;
     }
@@ -439,7 +453,7 @@ void ServiceWorkerNavigationLoader::StartResponse(
                            TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
                            "result", "blob response");
 
-    url_loader_client_->OnStartLoadingResponseBody(std::move(data_pipe));
+    CommitResponseBody(std::move(data_pipe));
     // We continue in OnBlobReadingComplete().
     return;
   }
@@ -449,7 +463,7 @@ void ServiceWorkerNavigationLoader::StartResponse(
                          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
                          "result", "no body");
 
-  // The response has no body.
+  CommitResponseBodyEmpty();
   CommitCompleted(net::OK);
 }
 
@@ -589,14 +603,17 @@ void ServiceWorkerNavigationLoader::TransitionToStatus(Status new_status) {
     case Status::kSentHeader:
       DCHECK_EQ(status_, Status::kStarted);
       break;
+    case Status::kSentBody:
+      DCHECK_EQ(status_, Status::kSentHeader);
+      break;
     case Status::kCompleted:
-      // kNotStarted -> kCompleted happens on network fallback before
-      // interception.
-      // kStarted -> kCompleted happens on error or network fallback after
-      // interception.
-      // kSentHeader -> kCompleted happens in the success case or error
-      // while sending the body.
-      DCHECK_NE(status_, Status::kCompleted);
+      DCHECK(
+          // Network fallback before interception.
+          status_ == Status::kNotStarted ||
+          // Network fallback after interception.
+          status_ == Status::kStarted ||
+          // Success case or error while sending the response's body.
+          status_ == Status::kSentBody);
       break;
   }
 #endif  // DCHECK_IS_ON()

content/browser/service_worker/service_worker_navigation_loader.h

@@ -106,9 +106,11 @@ class CONTENT_EXPORT ServiceWorkerNavigationLoader
     // |binding_| is bound and the fetch event is being dispatched to the
     // service worker.
     kStarted,
-    // The response head has been sent to |url_loader_client_|. The response
-    // body is being streamed.
+    // The response head has been sent to |url_loader_client_|.
     kSentHeader,
+    // The data pipe for the response body has been sent to
+    // |url_loader_client_|. The body is being written to the pipe.
+    kSentBody,
     // OnComplete() was called on |url_loader_client_|, or fallback to network
     // occurred so the request was not handled.
     kCompleted,
@@ -134,6 +136,12 @@ class CONTENT_EXPORT ServiceWorkerNavigationLoader
 
   // Calls url_loader_client_->OnReceiveResponse() with |response_head_|.
   void CommitResponseHeaders();
+
+  // Calls url_loader_client_->OnStartLoadingResponseBody() with
+  // |response_body| or with an empty data pipe.
+  void CommitResponseBody(mojo::ScopedDataPipeConsumerHandle response_body);
+  void CommitResponseBodyEmpty();
+
   // Calls url_loader_client_->OnComplete().
   void CommitCompleted(int error_code);
 

content/renderer/service_worker/service_worker_subresource_loader.cc

@@ -200,8 +200,7 @@ void ServiceWorkerSubresourceLoader::StartRequest(
       TRACE_ID_WITH_SCOPE(kServiceWorkerSubresourceLoaderScope,
                           TRACE_ID_LOCAL(request_id_)),
       TRACE_EVENT_FLAG_FLOW_OUT, "url", resource_request.url.spec());
-  DCHECK_EQ(Status::kNotStarted, status_);
-  status_ = Status::kStarted;
+  TransitionToStatus(Status::kStarted);
 
   DCHECK(!ServiceWorkerUtils::IsMainResourceType(
       static_cast<ResourceType>(resource_request.resource_type)));
@@ -408,6 +407,7 @@ void ServiceWorkerSubresourceLoader::OnFallback(
     response_head_.was_fetched_via_service_worker = true;
     response_head_.was_fallback_required_by_service_worker = true;
     CommitResponseHeaders();
+    CommitResponseBodyEmpty();
     CommitCompleted(net::OK);
     return;
   }
@@ -479,8 +479,7 @@ void ServiceWorkerSubresourceLoader::StartResponse(
     }
     response_head_.encoded_data_length = 0;
     url_loader_client_->OnReceiveRedirect(*redirect_info_, response_head_);
-    // Set status to complete, but we expect to restart in FollowRedirect.
-    status_ = Status::kCompleted;
+    TransitionToStatus(Status::kSentRedirect);
     return;
   }
 
@@ -493,8 +492,7 @@ void ServiceWorkerSubresourceLoader::StartResponse(
     DCHECK(url_loader_client_.is_bound());
     stream_waiter_ = std::make_unique<StreamWaiter>(
         this, std::move(body_as_stream->callback_request));
-    url_loader_client_->OnStartLoadingResponseBody(
-        std::move(body_as_stream->stream));
+    CommitResponseBody(std::move(body_as_stream->stream));
     return;
   }
 
@@ -507,13 +505,14 @@ void ServiceWorkerSubresourceLoader::StartResponse(
     body_as_blob_size_ = response->blob->size;
 
     // If parallel reading is enabled, then start reading the body blob
-    // immediately.  This will allow the body to start buffering in the
+    // immediately. This will allow the body to start buffering in the
     // pipe while the side data is read.
     mojo::ScopedDataPipeConsumerHandle data_pipe;
     if (base::FeatureList::IsEnabled(
             blink::features::kServiceWorkerParallelSideDataReading)) {
       int error = StartBlobReading(&data_pipe);
       if (error != net::OK) {
+        CommitResponseBodyEmpty();
         CommitCompleted(error);
         return;
       }
@@ -525,18 +524,31 @@ void ServiceWorkerSubresourceLoader::StartResponse(
     return;
   }
 
-  // The response has no body.
+  CommitResponseBodyEmpty();
   CommitCompleted(net::OK);
 }
 
 void ServiceWorkerSubresourceLoader::CommitResponseHeaders() {
-  DCHECK_EQ(Status::kStarted, status_);
+  TransitionToStatus(Status::kSentHeader);
   DCHECK(url_loader_client_.is_bound());
-  status_ = Status::kSentHeader;
   // TODO(kinuko): Fill the ssl_info.
   url_loader_client_->OnReceiveResponse(response_head_);
 }
 
+void ServiceWorkerSubresourceLoader::CommitResponseBody(
+    mojo::ScopedDataPipeConsumerHandle response_body) {
+  TransitionToStatus(Status::kSentBody);
+  url_loader_client_->OnStartLoadingResponseBody(std::move(response_body));
+}
+
+void ServiceWorkerSubresourceLoader::CommitResponseBodyEmpty() {
+  TransitionToStatus(Status::kSentBody);
+  mojo::DataPipe pipe;
+  url_loader_client_->OnStartLoadingResponseBody(
+      std::move(pipe.consumer_handle));
+  // pipe.producer_handle is closed here.
+}
+
 void ServiceWorkerSubresourceLoader::CommitCompleted(int error_code) {
   TRACE_EVENT_WITH_FLOW1(
       "ServiceWorker", "ServiceWorkerSubresourceLoader::CommitCompleted",
@@ -547,11 +559,10 @@ void ServiceWorkerSubresourceLoader::CommitCompleted(int error_code) {
   if (error_code == net::OK)
     RecordTimingMetrics(true /* handled */);
 
-  DCHECK_LT(status_, Status::kCompleted);
+  TransitionToStatus(Status::kCompleted);
   DCHECK(url_loader_client_.is_bound());
   body_as_blob_.reset();
   stream_waiter_.reset();
-  status_ = Status::kCompleted;
   network::URLLoaderCompletionStatus status;
   status.error_code = error_code;
   status.completion_time = base::TimeTicks::Now();
@@ -651,7 +662,7 @@ void ServiceWorkerSubresourceLoader::FollowRedirect(
   resource_request_.referrer_policy = redirect_info_->new_referrer_policy;
 
   // Restart the request.
-  status_ = Status::kNotStarted;
+  TransitionToStatus(Status::kNotStarted);
   redirect_info_.reset();
   response_callback_binding_.Close();
   StartRequest(resource_request_);
@@ -726,7 +737,7 @@ void ServiceWorkerSubresourceLoader::OnBlobSideDataReadingComplete(
       "ServiceWorker.SubresourceNotifyStartLoadingResponseBodyDelay", delay);
 
   DCHECK(data_pipe.is_valid());
-  url_loader_client_->OnStartLoadingResponseBody(std::move(data_pipe));
+  CommitResponseBody(std::move(data_pipe));
 
   // If the blob reading completed before the side data reading, then we
   // must manually finalize the blob reading now.
@@ -817,4 +828,37 @@ void ServiceWorkerSubresourceLoaderFactory::OnConnectionError() {
   delete this;
 }
 
+void ServiceWorkerSubresourceLoader::TransitionToStatus(Status new_status) {
+#if DCHECK_IS_ON()
+  switch (new_status) {
+    case Status::kNotStarted:
+      DCHECK_EQ(status_, Status::kSentRedirect);
+      break;
+    case Status::kStarted:
+      DCHECK_EQ(status_, Status::kNotStarted);
+      break;
+    case Status::kSentRedirect:
+      DCHECK_EQ(status_, Status::kStarted);
+      break;
+    case Status::kSentHeader:
+      DCHECK_EQ(status_, Status::kStarted);
+      break;
+    case Status::kSentBody:
+      DCHECK_EQ(status_, Status::kSentHeader);
+      break;
+    case Status::kCompleted:
+      DCHECK(
+          // Network fallback before interception.
+          status_ == Status::kNotStarted ||
+          // Network fallback after interception.
+          status_ == Status::kStarted ||
+          // Success case or error while sending the response's body.
+          status_ == Status::kSentBody);
+      break;
+  }
+#endif  // DCHECK_IS_ON()
+
+  status_ = new_status;
+}
+
 }  // namespace content

content/renderer/service_worker/service_worker_subresource_loader.h

@@ -61,6 +61,22 @@ class CONTENT_EXPORT ServiceWorkerSubresourceLoader
 
  private:
   class StreamWaiter;
+  enum class Status {
+    kNotStarted,
+    // |binding_| is bound and the fetch event is being dispatched to the
+    // service worker.
+    kStarted,
+    // A redirect happened, waiting for FollowRedirect().
+    kSentRedirect,
+    // The response head has been sent to |url_loader_client_|.
+    kSentHeader,
+    // The data pipe for the response body has been sent to
+    // |url_loader_client_|. The body is being written to the pipe.
+    kSentBody,
+    // OnComplete() was called on |url_loader_client_|, or fallback to network
+    // occurred so the request was not handled.
+    kCompleted,
+  };
 
   void OnConnectionError();
 
@@ -108,6 +124,12 @@ class CONTENT_EXPORT ServiceWorkerSubresourceLoader
 
   // Calls url_loader_client_->OnReceiveResponse() with |response_head_|.
   void CommitResponseHeaders();
+
+  // Calls url_loader_client_->OnStartLoadingResponseBody() with
+  // |response_body| or with an empty data pipe.
+  void CommitResponseBody(mojo::ScopedDataPipeConsumerHandle response_body);
+  void CommitResponseBodyEmpty();
+
   // Calls url_loader_client_->OnComplete(). Expected to be called after
   // CommitResponseHeaders (i.e. status_ == kSentHeader).
   void CommitCompleted(int error_code);
@@ -117,6 +139,8 @@ class CONTENT_EXPORT ServiceWorkerSubresourceLoader
   // occurred. |handled| is true when a fetch handler handled a request.
   void RecordTimingMetrics(bool handled);
 
+  void TransitionToStatus(Status new_status);
+
   network::ResourceResponseHead response_head_;
   base::Optional<net::RedirectInfo> redirect_info_;
   int redirect_limit_;
@@ -159,12 +183,6 @@ class CONTENT_EXPORT ServiceWorkerSubresourceLoader
   // For network fallback.
   scoped_refptr<network::SharedURLLoaderFactory> fallback_factory_;
 
-  enum class Status {
-    kNotStarted,
-    kStarted,
-    kSentHeader,
-    kCompleted,
-  };
   Status status_ = Status::kNotStarted;
 
   // The task runner where this loader is running.