ServiceWorker: Always call OnStartLoadingResponseBody() when received response

When send response info to client of resume type ServiceWorkerNewScriptLoader.
OnStartLoadingResponseBody() is also called no matter the body is empty
or not.

Bug: 648295
Change-Id: Id97ab5e3965e5860e7172098291f10b1bfe4a151
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1577928Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Ting Shao <ting.shao@intel.com>
Cr-Commit-Position: refs/heads/master@{#653562}

content/browser/service_worker/service_worker_cache_writer.cc

@@ -597,8 +597,15 @@ int ServiceWorkerCacheWriter::WriteInfoToResponseWriter(
 
 int ServiceWorkerCacheWriter::WriteInfo(
     scoped_refptr<HttpResponseInfoIOBuffer> response_info) {
-  if (write_observer_)
-    write_observer_->WillWriteInfo(response_info);
+  if (!write_observer_)
+    return WriteInfoToResponseWriter(std::move(response_info));
+
+  int result = write_observer_->WillWriteInfo(response_info);
+  if (result != net::OK) {
+    DCHECK_NE(result, net::ERR_IO_PENDING);
+    state_ = STATE_DONE;
+    return result;
+  }
 
   return WriteInfoToResponseWriter(std::move(response_info));
 }

content/browser/service_worker/service_worker_cache_writer.h

@@ -45,7 +45,8 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
   class WriteObserver {
    public:
     // Called before response info is written to storage.
-    virtual void WillWriteInfo(
+    // Returns net::OK if success. Other values are treated as errors.
+    virtual int WillWriteInfo(
         scoped_refptr<HttpResponseInfoIOBuffer> response_info) = 0;
 
     // Called before response data is written to storage.

content/browser/service_worker/service_worker_cache_writer_unittest.cc

@@ -30,9 +30,10 @@ class MockServiceWorkerCacheWriterObserver
   MockServiceWorkerCacheWriterObserver() : data_length_(0), result_(net::OK) {}
   ~MockServiceWorkerCacheWriterObserver() {}
 
-  void WillWriteInfo(
+  int WillWriteInfo(
       scoped_refptr<HttpResponseInfoIOBuffer> response_info) override {
     response_info_ = std::move(response_info);
+    return net::OK;
   }
 
   int WillWriteData(scoped_refptr<net::IOBuffer> data,

content/browser/service_worker/service_worker_new_script_loader.cc

@@ -478,7 +478,7 @@ void ServiceWorkerNewScriptLoader::OnComplete(
 
 // End of URLLoaderClient ------------------------------------------------------
 
-void ServiceWorkerNewScriptLoader::WillWriteInfo(
+int ServiceWorkerNewScriptLoader::WillWriteInfo(
     scoped_refptr<HttpResponseInfoIOBuffer> response_info) {
   DCHECK_EQ(type_, Type::kResume);
   DCHECK(response_info);
@@ -492,6 +492,22 @@ void ServiceWorkerNewScriptLoader::WillWriteInfo(
   ServiceWorkerUtils::SendHttpResponseInfoToClient(
       info, original_options_, request_start_, base::TimeTicks::Now(),
       response_info->response_data_size, client_.get());
+
+  mojo::ScopedDataPipeConsumerHandle client_consumer;
+  if (mojo::CreateDataPipe(nullptr, &client_producer_, &client_consumer) !=
+      MOJO_RESULT_OK) {
+    // Reports error to cache writer and finally the loader would process this
+    // failure in OnCacheWriterResumed()
+    return net::ERR_INSUFFICIENT_RESOURCES;
+  }
+
+  // Pass the consumer handle to the client.
+  client_->OnStartLoadingResponseBody(std::move(client_consumer));
+  client_producer_watcher_.Watch(
+      client_producer_.get(), MOJO_HANDLE_SIGNAL_WRITABLE,
+      base::BindRepeating(&ServiceWorkerNewScriptLoader::OnClientWritable,
+                          weak_factory_.GetWeakPtr()));
+  return net::OK;
 }
 
 void ServiceWorkerNewScriptLoader::OnClientWritable(MojoResult) {
@@ -539,28 +555,12 @@ int ServiceWorkerNewScriptLoader::WillWriteData(
     base::OnceCallback<void(net::Error)> callback) {
   DCHECK_EQ(type_, Type::kResume);
   DCHECK(!write_observer_complete_callback_);
+  DCHECK(client_producer_);
+
   data_to_send_ = std::move(data);
   data_length_ = length;
   bytes_sent_to_client_ = 0;
   write_observer_complete_callback_ = std::move(callback);
-
-  // Send a data pipe to the client if it's the first call of WillWriteData().
-  if (!client_producer_) {
-    mojo::ScopedDataPipeConsumerHandle client_consumer;
-    if (mojo::CreateDataPipe(nullptr, &client_producer_, &client_consumer) !=
-        MOJO_RESULT_OK) {
-      // Report error to cache writer and finally the loader would process this
-      // failure in OnCacheWriterResumed().
-      return net::ERR_INSUFFICIENT_RESOURCES;
-    }
-
-    // Pass the consumer handle for responding with the response to the client.
-    client_->OnStartLoadingResponseBody(std::move(client_consumer));
-    client_producer_watcher_.Watch(
-        client_producer_.get(), MOJO_HANDLE_SIGNAL_WRITABLE,
-        base::BindRepeating(&ServiceWorkerNewScriptLoader::OnClientWritable,
-                            weak_factory_.GetWeakPtr()));
-  }
   client_producer_watcher_.ArmOrNotify();
   return net::ERR_IO_PENDING;
 }

content/browser/service_worker/service_worker_new_script_loader.h

@@ -157,7 +157,7 @@ class CONTENT_EXPORT ServiceWorkerNewScriptLoader
 
   // Implements ServiceWorkerCacheWriter::WriteObserver.
   // These two methods are only used for resume loaders.
-  void WillWriteInfo(
+  int WillWriteInfo(
       scoped_refptr<HttpResponseInfoIOBuffer> response_info) override;
   int WillWriteData(scoped_refptr<net::IOBuffer> data,
                     int length,

content/browser/service_worker/service_worker_new_script_loader_unittest.cc

@@ -920,17 +920,13 @@ class ServiceWorkerNewScriptLoaderResumeTest
   }
 
   // Verify the received response.
-  void CheckReceivedResponse(bool has_body, const std::string& expected_body) {
+  void CheckReceivedResponse(const std::string& expected_body) {
     EXPECT_TRUE(client_->has_received_response());
-    EXPECT_EQ(has_body, client_->response_body().is_valid());
+    EXPECT_TRUE(client_->response_body().is_valid());
 
     // The response should also be stored in the storage.
     EXPECT_TRUE(ServiceWorkerUpdateCheckTestUtils::VerifyStoredResponse(
-        LookupResourceId(kScriptURL), context()->storage(),
-        has_body ? expected_body : ""));
-
-    if (!has_body)
-      return;
+        LookupResourceId(kScriptURL), context()->storage(), expected_body));
 
     std::string response;
     EXPECT_TRUE(mojo::BlockingCopyToString(client_->response_body_release(),
@@ -980,7 +976,7 @@ TEST_F(ServiceWorkerNewScriptLoaderResumeTest, FirstBlockDifferent) {
   EXPECT_EQ(net::OK, client_->completion_status().error_code);
 
   // The client should have received the response.
-  CheckReceivedResponse(true, kNewData);
+  CheckReceivedResponse(kNewData);
 }
 
 // Tests resume type loader when the script data block in the middle is
@@ -1012,7 +1008,7 @@ TEST_F(ServiceWorkerNewScriptLoaderResumeTest, MiddleBlockDifferent) {
   EXPECT_EQ(net::OK, client_->completion_status().error_code);
 
   // The client should have received the response.
-  CheckReceivedResponse(true, kNewData);
+  CheckReceivedResponse(kNewData);
 }
 
 // Tests resume type loader when the last script data block is different.
@@ -1039,7 +1035,7 @@ TEST_F(ServiceWorkerNewScriptLoaderResumeTest, LastBlockDifferent) {
   EXPECT_EQ(net::OK, client_->completion_status().error_code);
 
   // The client should have received the response.
-  CheckReceivedResponse(true, kNewData);
+  CheckReceivedResponse(kNewData);
 }
 
 // Tests resume type loader when the last script data block is different and
@@ -1064,7 +1060,7 @@ TEST_F(ServiceWorkerNewScriptLoaderResumeTest, LastBlockDifferentCompleted) {
   EXPECT_EQ(net::OK, client_->completion_status().error_code);
 
   // The client should have received the response.
-  CheckReceivedResponse(true, kNewData);
+  CheckReceivedResponse(kNewData);
 }
 
 // Tests resume type loader when the new script has more data appended.
@@ -1095,7 +1091,7 @@ TEST_F(ServiceWorkerNewScriptLoaderResumeTest, NewScriptLargerThanOld) {
   EXPECT_EQ(net::OK, client_->completion_status().error_code);
 
   // The client should have received the response.
-  CheckReceivedResponse(true, kNewData);
+  CheckReceivedResponse(kNewData);
 }
 
 // Tests resume type loader when the script changed to have no body.
@@ -1117,7 +1113,7 @@ TEST_F(ServiceWorkerNewScriptLoaderResumeTest, NewScriptEmptyBody) {
 
   EXPECT_EQ(net::OK, client_->completion_status().error_code);
 
-  CheckReceivedResponse(false, kNewData);
+  CheckReceivedResponse(kNewData);
 }
 
 // Tests resume type loader could report error when the resumed network