Service worker can be updated when the script is truncated

Previously ServiceWorkerNewScriptLoader returns kErrorExists when the main
worker script was just truncated because it calls CommitCompleted() when the
Mojo data pipe is closed. However, it's a problem because it doesn't tell the
cache writer about the end of the body so that the cache writer isn't aware of
the truncation.

This CL is to trigger MaybeWriteData() at connection close for the cache writer
to give a chance to run script comparison at the end of the body.

Bug: 986688
Change-Id: I8a816ab1ab1a2e236c1a21c1f0950437cd1e50ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1714584Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Auto-Submit: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680369}

content/browser/service_worker/service_worker_new_script_loader.cc

@@ -670,14 +670,9 @@ void ServiceWorkerNewScriptLoader::OnNetworkDataAvailable(MojoResult) {
       WriteData(std::move(pending_buffer), bytes_available);
       return;
     case MOJO_RESULT_FAILED_PRECONDITION:
-      // Closed by peer. This indicates all the data from the network service
-      // are read or there is an error. In the error case, the reason is
-      // notified via OnComplete().
-      body_writer_state_ = WriterState::kCompleted;
-      if (network_loader_state_ == NetworkLoaderState::kCompleted) {
-        CommitCompleted(network::URLLoaderCompletionStatus(net::OK),
-                        std::string() /* status_message */);
-      }
+      // Call WriteData() with null buffer to let the cache writer know that
+      // body from the network reaches to the end.
+      WriteData(/*pending_buffer=*/nullptr, /*bytes_available=*/0);
       return;
     case MOJO_RESULT_SHOULD_WAIT:
       network_watcher_.ArmOrNotify();
@@ -693,7 +688,8 @@ void ServiceWorkerNewScriptLoader::WriteData(
   // next time.
   uint32_t bytes_written = std::min<uint32_t>(kReadBufferSize, bytes_available);
 
-  auto buffer = base::MakeRefCounted<WrappedIOBuffer>(pending_buffer->buffer());
+  auto buffer = base::MakeRefCounted<WrappedIOBuffer>(
+      pending_buffer ? pending_buffer->buffer() : nullptr);
   MojoResult result = client_producer_->WriteData(
       buffer->data(), &bytes_written, MOJO_WRITE_DATA_FLAG_NONE);
   switch (result) {
@@ -719,6 +715,8 @@ void ServiceWorkerNewScriptLoader::WriteData(
 
   // Write the buffer in the service worker script storage up to the size we
   // successfully wrote to the data pipe (i.e., |bytes_written|).
+  // A null buffer and zero |bytes_written| are passed when this is the end of
+  // the body.
   net::Error error = cache_writer_->MaybeWriteData(
       buffer.get(), base::strict_cast<size_t>(bytes_written),
       base::BindOnce(&ServiceWorkerNewScriptLoader::OnWriteDataComplete,
@@ -745,9 +743,23 @@ void ServiceWorkerNewScriptLoader::OnWriteDataComplete(
                     ServiceWorkerConsts::kServiceWorkerFetchScriptError);
     return;
   }
-  DCHECK(pending_buffer);
   ServiceWorkerMetrics::CountWriteResponseResult(
       ServiceWorkerMetrics::WRITE_OK);
+
+  if (bytes_written == 0) {
+    // Zero |bytes_written| with net::OK means that all data has been read from
+    // the network and the Mojo data pipe has been closed. Thus we can complete
+    // the request if OnComplete() has already been received.
+    DCHECK(!pending_buffer);
+    body_writer_state_ = WriterState::kCompleted;
+    if (network_loader_state_ == NetworkLoaderState::kCompleted) {
+      CommitCompleted(network::URLLoaderCompletionStatus(net::OK),
+                      std::string() /* status_message */);
+    }
+    return;
+  }
+
+  DCHECK(pending_buffer);
   pending_buffer->CompleteRead(bytes_written);
   // Get the consumer handle from a previous read operation if we have one.
   network_consumer_ = pending_buffer->ReleaseHandle();

content/browser/service_worker/service_worker_new_script_loader_unittest.cc

@@ -109,16 +109,16 @@ class MockNetworkURLLoaderFactory final
     }
     client->OnReceiveResponse(response_head);
 
-    // Pass the response body to the client.
-    if (!response.body.empty()) {
-      uint32_t bytes_written = response.body.size();
-      mojo::DataPipe data_pipe;
-      MojoResult result = data_pipe.producer_handle->WriteData(
-          response.body.data(), &bytes_written,
-          MOJO_WRITE_DATA_FLAG_ALL_OR_NONE);
-      ASSERT_EQ(MOJO_RESULT_OK, result);
-      client->OnStartLoadingResponseBody(std::move(data_pipe.consumer_handle));
-    }
+    uint32_t bytes_written = response.body.size();
+    mojo::ScopedDataPipeConsumerHandle consumer;
+    mojo::ScopedDataPipeProducerHandle producer;
+    ASSERT_EQ(MOJO_RESULT_OK,
+              mojo::CreateDataPipe(nullptr, &producer, &consumer));
+    MojoResult result = producer->WriteData(
+        response.body.data(), &bytes_written, MOJO_WRITE_DATA_FLAG_ALL_OR_NONE);
+    ASSERT_EQ(MOJO_RESULT_OK, result);
+    client->OnStartLoadingResponseBody(std::move(consumer));
+
     network::URLLoaderCompletionStatus status;
     status.error_code = net::OK;
     client->OnComplete(status);
@@ -303,10 +303,11 @@ TEST_F(ServiceWorkerNewScriptLoaderTest, Success) {
       mojo::BlockingCopyToString(client->response_body_release(), &response));
   EXPECT_EQ(mock_server_->Get(kScriptURL).body, response);
 
-  // The response should also be stored in the storage.
+  // WRITE_OK should be recorded once plus one as we record a single write
+  // success and the end of the body.
   EXPECT_TRUE(VerifyStoredResponse(kScriptURL));
   histogram_tester.ExpectUniqueSample(kHistogramWriteResponseResult,
-                                      ServiceWorkerMetrics::WRITE_OK, 1);
+                                      ServiceWorkerMetrics::WRITE_OK, 2);
 }
 
 TEST_F(ServiceWorkerNewScriptLoaderTest, Success_EmptyBody) {
@@ -327,12 +328,13 @@ TEST_F(ServiceWorkerNewScriptLoaderTest, Success_EmptyBody) {
 
   // The client should have received the response.
   EXPECT_TRUE(client->has_received_response());
-  EXPECT_FALSE(client->response_body().is_valid());
+  EXPECT_TRUE(client->response_body().is_valid());
 
   // The response should also be stored in the storage.
   EXPECT_TRUE(VerifyStoredResponse(kScriptURL));
-  // We don't record write response result if body is empty.
-  histogram_tester.ExpectTotalCount(kHistogramWriteResponseResult, 0);
+  // WRITE_OK should be recorded once as we record the end of the body.
+  histogram_tester.ExpectUniqueSample(kHistogramWriteResponseResult,
+                                      ServiceWorkerMetrics::WRITE_OK, 1);
 }
 
 TEST_F(ServiceWorkerNewScriptLoaderTest, Success_LargeBody) {
@@ -368,9 +370,10 @@ TEST_F(ServiceWorkerNewScriptLoaderTest, Success_LargeBody) {
 
   // The response should also be stored in the storage.
   EXPECT_TRUE(VerifyStoredResponse(kScriptURL));
-  // WRITE_OK should be recorded twice as we record every single write success.
+  // WRITE_OK should be recorded twice plus one as we record every single write
+  // success and the end of the body.
   histogram_tester.ExpectUniqueSample(kHistogramWriteResponseResult,
-                                      ServiceWorkerMetrics::WRITE_OK, 2);
+                                      ServiceWorkerMetrics::WRITE_OK, 3);
 }
 
 TEST_F(ServiceWorkerNewScriptLoaderTest, Error_404) {
@@ -530,10 +533,11 @@ TEST_F(ServiceWorkerNewScriptLoaderTest, Success_PathRestriction) {
       mojo::BlockingCopyToString(client->response_body_release(), &response));
   EXPECT_EQ(mock_server_->Get(kScriptURL).body, response);
 
-  // The response should also be stored in the storage.
+  // WRITE_OK should be recorded once plus one as we record a single write
+  // success and the end of the body.
   EXPECT_TRUE(VerifyStoredResponse(kScriptURL));
   histogram_tester.ExpectUniqueSample(kHistogramWriteResponseResult,
-                                      ServiceWorkerMetrics::WRITE_OK, 1);
+                                      ServiceWorkerMetrics::WRITE_OK, 2);
 }
 
 TEST_F(ServiceWorkerNewScriptLoaderTest, Error_PathRestriction) {

third_party/blink/web_tests/TestExpectations

@@ -2395,13 +2395,6 @@ crbug.com/411164 [ Win ] http/tests/security/powerfulFeatureRestrictions/service
 
 crbug.com/686118 http/tests/security/setDomainRelaxationForbiddenForURLScheme.html [ Crash Pass ]
 
-# Service worker updates need to succeed when the script shrinks.
-crbug.com/986688 external/wpt/service-workers/service-worker/update.https.html [ Skip ]
-crbug.com/986688 virtual/blink-cors/external/wpt/service-workers/service-worker/update.https.html [ Skip ]
-crbug.com/986688 virtual/cache-storage-sequence/external/wpt/service-workers/service-worker/update.https.html [ Skip ]
-crbug.com/986688 virtual/not-omt-sw-fetch/external/wpt/service-workers/service-worker/update.https.html [ Skip ]
-crbug.com/986688 virtual/omt-worker-fetch/external/wpt/service-workers/service-worker/update.https.html [ Skip ]
-
 # In external/wpt/html/, we prefer checking in failure
 # expectation files. The following tests with [ Failure ] don't have failure
 # expectation files because they contain local path names.

third_party/blink/web_tests/external/wpt/service-workers/service-worker/update.https-expected.txt

@@ -5,5 +5,6 @@ FAIL update() should fail when a response for the main script is redirect. asser
 PASS update() should fail when a new script contains a syntax error.
 PASS update() should resolve when the install event throws.
 PASS update() should fail when the pending uninstall flag is set.
+PASS update() should succeed when the script shrinks.
 Harness: the test ran to completion.