service worker: Add an interface to notify resource write completion

This CL introduces a mojo interface that works as a client of
ServiceWorkerResourceReader. Call sites of the reader can pass
a remote that implements the interface to receive OnComplete()
notification. This kind of notification is required to replace
old resource reader in ServiceWorkerInstalledScriptReader.

Bug: 1055677
Change-Id: I1038298942d892bcc69a9dd9edebae73a63d29c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2279304
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786571}

components/services/storage/public/mojom/service_worker_storage_control.mojom

@@ -56,6 +56,14 @@ enum ServiceWorkerStorageOriginState {
   kDelete,
 };
 
+// A notifier interface for ServiceWorkerResourceReader::ReadData().
+interface ServiceWorkerDataPipeStateNotifier {
+  // Called when ReadData() completes. On success, |status| is the number of
+  // bytes written. On failure, |status| is a //net error, which is a negative
+  // number.
+  OnComplete(int32 status);
+};
+
 // An interface that reads a service worker script (resource) to storage.
 interface ServiceWorkerResourceReader {
   // Reads the response head of the resource associated with this reader.
@@ -66,7 +74,9 @@ interface ServiceWorkerResourceReader {
        network.mojom.URLResponseHead? response_head,
        mojo_base.mojom.BigBuffer? metadata);
   // Reads the content of the resource associated with this reader.
-  ReadData(int64 size) => (handle<data_pipe_consumer> pipe);
+  ReadData(int64 size,
+           pending_remote<ServiceWorkerDataPipeStateNotifier> notifier) =>
+      (handle<data_pipe_consumer> pipe);
 };
 
 // An interface that writes a service worker script (resource) to storage.

content/browser/service_worker/service_worker_cache_writer.cc

@@ -624,7 +624,8 @@ int ServiceWorkerCacheWriter::ReadResponseHead(
   return adapter->result();
 }
 
-class ServiceWorkerCacheWriter::DataPipeReader {
+class ServiceWorkerCacheWriter::DataPipeReader
+    : public storage::mojom::ServiceWorkerDataPipeStateNotifier {
  public:
   DataPipeReader(storage::mojom::ServiceWorkerResourceReader* reader,
                  ServiceWorkerCacheWriter* owner,
@@ -646,8 +647,9 @@ class ServiceWorkerCacheWriter::DataPipeReader {
     if (!data_.is_valid()) {
       // This is the initial call of Read(). Call ReadData() to get a data pipe
       // to read the body.
+      DCHECK(!receiver_.is_bound());
       reader_->ReadData(
-          -1,
+          -1, receiver_.BindNewPipeAndPassRemote(),
           base::BindOnce(&ServiceWorkerCacheWriter::DataPipeReader::OnReadData,
                          weak_factory_.GetWeakPtr()));
       return;
@@ -685,6 +687,11 @@ class ServiceWorkerCacheWriter::DataPipeReader {
     ReadInternal(MOJO_RESULT_OK);
   }
 
+  // storage::mojom::ServiceWorkerDataPipeStateNotifier override:
+  void OnComplete(int32_t status) override {
+    // TODO(https://crbug.com/1055677): notify of errors.
+  }
+
   // Parameters set on Read().
   net::IOBuffer* buffer_ = nullptr;
   uint32_t num_bytes_to_read_ = 0;
@@ -701,6 +708,9 @@ class ServiceWorkerCacheWriter::DataPipeReader {
   mojo::SimpleWatcher watcher_;
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
+  mojo::Receiver<storage::mojom::ServiceWorkerDataPipeStateNotifier> receiver_{
+      this};
+
   base::WeakPtrFactory<DataPipeReader> weak_factory_{this};
 };
 

content/browser/service_worker/service_worker_resource_ops.cc

@@ -4,6 +4,7 @@
 
 #include "content/browser/service_worker/service_worker_resource_ops.h"
 
+#include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/system/data_pipe.h"
 #include "services/network/public/cpp/net_adapters.h"
 #include "third_party/blink/public/common/blob/blob_utils.h"
@@ -86,15 +87,20 @@ void DidReadInfo(
 
 class ServiceWorkerResourceReaderImpl::DataReader {
  public:
-  DataReader(base::WeakPtr<ServiceWorkerResourceReaderImpl> owner,
-             size_t total_bytes_to_read,
-             mojo::ScopedDataPipeProducerHandle producer_handle)
+  DataReader(
+      base::WeakPtr<ServiceWorkerResourceReaderImpl> owner,
+      size_t total_bytes_to_read,
+      mojo::PendingRemote<storage::mojom::ServiceWorkerDataPipeStateNotifier>
+          notifier,
+      mojo::ScopedDataPipeProducerHandle producer_handle)
       : owner_(std::move(owner)),
         total_bytes_to_read_(total_bytes_to_read),
+        notifier_(std::move(notifier)),
         producer_handle_(std::move(producer_handle)),
         watcher_(FROM_HERE,
                  mojo::SimpleWatcher::ArmingPolicy::MANUAL,
                  base::SequencedTaskRunnerHandle::Get()) {
+    DCHECK(notifier_);
     watcher_.Watch(producer_handle_.get(), MOJO_HANDLE_SIGNAL_WRITABLE,
                    base::BindRepeating(&DataReader::OnWritable,
                                        weak_factory_.GetWeakPtr()));
@@ -166,6 +172,11 @@ class ServiceWorkerResourceReaderImpl::DataReader {
   void Complete(int status) {
     watcher_.Cancel();
     producer_handle_.reset();
+
+    if (notifier_.is_connected()) {
+      notifier_->OnComplete(status);
+    }
+
     if (owner_) {
       owner_->DidReadDataComplete();
     }
@@ -174,6 +185,7 @@ class ServiceWorkerResourceReaderImpl::DataReader {
   base::WeakPtr<ServiceWorkerResourceReaderImpl> owner_;
   const size_t total_bytes_to_read_;
   size_t current_bytes_read_ = 0;
+  mojo::Remote<storage::mojom::ServiceWorkerDataPipeStateNotifier> notifier_;
   mojo::ScopedDataPipeProducerHandle producer_handle_;
   mojo::SimpleWatcher watcher_;
   scoped_refptr<network::NetToMojoPendingBuffer> pending_buffer_;
@@ -197,8 +209,11 @@ void ServiceWorkerResourceReaderImpl::ReadResponseHead(
                                                std::move(callback)));
 }
 
-void ServiceWorkerResourceReaderImpl::ReadData(int64_t size,
-                                               ReadDataCallback callback) {
+void ServiceWorkerResourceReaderImpl::ReadData(
+    int64_t size,
+    mojo::PendingRemote<storage::mojom::ServiceWorkerDataPipeStateNotifier>
+        notifier,
+    ReadDataCallback callback) {
   MojoCreateDataPipeOptions options;
   options.struct_size = sizeof(MojoCreateDataPipeOptions);
   options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
@@ -215,6 +230,7 @@ void ServiceWorkerResourceReaderImpl::ReadData(int64_t size,
   }
 
   data_reader_ = std::make_unique<DataReader>(weak_factory_.GetWeakPtr(), size,
+                                              std::move(notifier),
                                               std::move(producer_handle));
   std::move(callback).Run(std::move(consumer_handle));
 }

content/browser/service_worker/service_worker_resource_ops.h

@@ -32,7 +32,11 @@ class ServiceWorkerResourceReaderImpl
  private:
   // storage::mojom::ServiceWorkerResourceReader implementations:
   void ReadResponseHead(ReadResponseHeadCallback callback) override;
-  void ReadData(int64_t size, ReadDataCallback callback) override;
+  void ReadData(
+      int64_t size,
+      mojo::PendingRemote<storage::mojom::ServiceWorkerDataPipeStateNotifier>
+          notifier,
+      ReadDataCallback callback) override;
 
   void DidReadDataComplete();
 

content/browser/service_worker/service_worker_storage_control_impl_unittest.cc

@@ -43,6 +43,11 @@ struct ReadResponseHeadResult {
   base::Optional<mojo_base::BigBuffer> metadata;
 };
 
+struct ReadDataResult {
+  int status;
+  std::string data;
+};
+
 struct GetRegistrationsForOriginResult {
   DatabaseStatus status;
   std::vector<storage::mojom::SerializedServiceWorkerRegistrationPtr>
@@ -95,19 +100,56 @@ ReadResponseHeadResult ReadResponseHead(
   return result;
 }
 
-std::string ReadResponseData(
+class MockServiceWorkerDataPipeStateNotifier
+    : public storage::mojom::ServiceWorkerDataPipeStateNotifier {
+ public:
+  MockServiceWorkerDataPipeStateNotifier() = default;
+
+  mojo::PendingRemote<storage::mojom::ServiceWorkerDataPipeStateNotifier>
+  BindNewPipeAndPassRemote() {
+    return receiver_.BindNewPipeAndPassRemote();
+  }
+
+  int32_t WaitUntilComplete() {
+    if (!complete_status_.has_value()) {
+      loop_.Run();
+      DCHECK(complete_status_.has_value());
+    }
+    return *complete_status_;
+  }
+
+ private:
+  void OnComplete(int32_t status) override {
+    complete_status_ = status;
+    if (loop_.running())
+      loop_.Quit();
+  }
+
+  base::Optional<int32_t> complete_status_;
+  base::RunLoop loop_;
+  mojo::Receiver<storage::mojom::ServiceWorkerDataPipeStateNotifier> receiver_{
+      this};
+};
+
+ReadDataResult ReadResponseData(
     storage::mojom::ServiceWorkerResourceReader* reader,
     int data_size) {
+  MockServiceWorkerDataPipeStateNotifier notifier;
   mojo::ScopedDataPipeConsumerHandle data_consumer;
   base::RunLoop loop;
-  reader->ReadData(data_size, base::BindLambdaForTesting(
-                                  [&](mojo::ScopedDataPipeConsumerHandle pipe) {
-                                    data_consumer = std::move(pipe);
-                                    loop.Quit();
-                                  }));
+  reader->ReadData(
+      data_size, notifier.BindNewPipeAndPassRemote(),
+      base::BindLambdaForTesting([&](mojo::ScopedDataPipeConsumerHandle pipe) {
+        data_consumer = std::move(pipe);
+        loop.Quit();
+      }));
   loop.Run();
 
-  return ReadDataPipe(std::move(data_consumer));
+  ReadDataResult result;
+  result.data = ReadDataPipe(std::move(data_consumer));
+  result.status = notifier.WaitUntilComplete();
+
+  return result;
 }
 
 int WriteResponseHead(storage::mojom::ServiceWorkerResourceWriter* writer,
@@ -612,7 +654,7 @@ class ServiceWorkerStorageControlImplTest : public testing::Test {
     return result;
   }
 
-  std::string ReadResource(int64_t resource_id, int data_size) {
+  ReadDataResult ReadResource(int64_t resource_id, int data_size) {
     mojo::Remote<storage::mojom::ServiceWorkerResourceReader> reader =
         CreateResourceReader(resource_id);
     return ReadResponseData(reader.get(), data_size);
@@ -1007,8 +1049,9 @@ TEST_F(ServiceWorkerStorageControlImplTest, WriteAndReadResource) {
               ssl_info.cert->serial_number());
     EXPECT_EQ(result.metadata, base::nullopt);
 
-    std::string data = ReadResponseData(reader.get(), data_size);
-    EXPECT_EQ(data, kData);
+    ReadDataResult data_result = ReadResponseData(reader.get(), data_size);
+    ASSERT_EQ(data_result.status, data_size);
+    EXPECT_EQ(data_result.data, kData);
   }
 
   const auto kMetadata = base::as_bytes(base::make_span("metadata"));
@@ -1491,12 +1534,16 @@ TEST_F(ServiceWorkerStorageControlImplTest, TrackRunningVersion) {
 
   // Resources shouldn't be purged because there is an active reference.
   {
-    std::string read_resource_data1 =
+    ReadDataResult read_resource_result1 =
         ReadResource(resource_id1, resource_data1.size());
-    ASSERT_EQ(read_resource_data1, resource_data1);
-    std::string read_resource_data2 =
+    ASSERT_EQ(read_resource_result1.status,
+              static_cast<int32_t>(resource_data1.size()));
+    EXPECT_EQ(read_resource_result1.data, resource_data1);
+    ReadDataResult read_resource_result2 =
         ReadResource(resource_id2, resource_data2.size());
-    ASSERT_EQ(read_resource_data2, resource_data2);
+    ASSERT_EQ(read_resource_result2.status,
+              static_cast<int32_t>(resource_data2.size()));
+    EXPECT_EQ(read_resource_result2.data, resource_data2);
   }
 
   // Drop the second reference.
@@ -1505,12 +1552,14 @@ TEST_F(ServiceWorkerStorageControlImplTest, TrackRunningVersion) {
 
   // Resources should have been purged.
   {
-    std::string read_resource_data1 =
+    ReadDataResult read_resource_result1 =
         ReadResource(resource_id1, resource_data1.size());
-    ASSERT_EQ(read_resource_data1, "");
-    std::string read_resource_data2 =
+    ASSERT_EQ(read_resource_result1.status, net::ERR_CACHE_MISS);
+    EXPECT_EQ(read_resource_result1.data, "");
+    ReadDataResult read_resource_result2 =
         ReadResource(resource_id2, resource_data2.size());
-    ASSERT_EQ(read_resource_data2, "");
+    ASSERT_EQ(read_resource_result2.status, net::ERR_CACHE_MISS);
+    EXPECT_EQ(read_resource_result2.data, "");
   }
 }
 

content/browser/service_worker/service_worker_test_utils.cc

@@ -559,8 +559,11 @@ void MockServiceWorkerResourceReader::ReadResponseHead(
   pending_read_response_head_callback_ = std::move(callback);
 }
 
-void MockServiceWorkerResourceReader::ReadData(int64_t,
-                                               ReadDataCallback callback) {
+void MockServiceWorkerResourceReader::ReadData(
+    int64_t,
+    mojo::PendingRemote<
+        storage::mojom::ServiceWorkerDataPipeStateNotifier> /*notifier*/,
+    ReadDataCallback callback) {
   DCHECK(!body_.is_valid());
   mojo::ScopedDataPipeConsumerHandle consumer;
   MojoCreateDataPipeOptions options;

content/browser/service_worker/service_worker_test_utils.h

@@ -239,7 +239,11 @@ class MockServiceWorkerResourceReader
   void ReadResponseHead(
       storage::mojom::ServiceWorkerResourceReader::ReadResponseHeadCallback
           callback) override;
-  void ReadData(int64_t, ReadDataCallback callback) override;
+  void ReadData(
+      int64_t,
+      mojo::PendingRemote<storage::mojom::ServiceWorkerDataPipeStateNotifier>
+          notifier,
+      ReadDataCallback callback) override;
 
   // Test helpers. ExpectReadResponseHead() and ExpectReadData() give precise
   // control over both the data to be written and the result to return.