Remove ServiceWorkerResponseReader from ServiceWorkerCacheWriter

This CL removes old ServiceWorkerResponseReader and starts using
new mojofied ServiceWorkerResourceReader in prod code.

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

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

@@ -63,7 +63,7 @@ interface ServiceWorkerResourceReader {
   // Also returns metadata associated with the resource if it exists.
   ReadResponseHead() =>
       (int32 status,
-       network.mojom.URLResponseHead response_head,
+       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);

content/browser/service_worker/service_worker_cache_writer.cc

@@ -161,19 +161,6 @@ int ServiceWorkerCacheWriter::DoLoop(int status) {
   return status;
 }
 
-ServiceWorkerCacheWriter::ServiceWorkerCacheWriter(
-    std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-    std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-    std::unique_ptr<ServiceWorkerResponseWriter> writer,
-    bool pause_when_not_identical)
-    : state_(STATE_START),
-      io_pending_(false),
-      comparing_(false),
-      pause_when_not_identical_(pause_when_not_identical),
-      legacy_compare_reader_(std::move(compare_reader)),
-      legacy_copy_reader_(std::move(copy_reader)),
-      writer_(std::move(writer)) {}
-
 ServiceWorkerCacheWriter::ServiceWorkerCacheWriter(
     mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader,
     mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader,
@@ -189,17 +176,6 @@ ServiceWorkerCacheWriter::ServiceWorkerCacheWriter(
 
 ServiceWorkerCacheWriter::~ServiceWorkerCacheWriter() {}
 
-std::unique_ptr<ServiceWorkerCacheWriter>
-ServiceWorkerCacheWriter::CreateForCopy(
-    std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-    std::unique_ptr<ServiceWorkerResponseWriter> writer) {
-  DCHECK(copy_reader);
-  DCHECK(writer);
-  return base::WrapUnique(new ServiceWorkerCacheWriter(
-      nullptr /* compare_reader */, std::move(copy_reader), std::move(writer),
-      false /* pause_when_not_identical*/));
-}
-
 std::unique_ptr<ServiceWorkerCacheWriter>
 ServiceWorkerCacheWriter::CreateForCopy(
     mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader,
@@ -217,24 +193,8 @@ ServiceWorkerCacheWriter::CreateForWriteBack(
     std::unique_ptr<ServiceWorkerResponseWriter> writer) {
   DCHECK(writer);
   return base::WrapUnique(new ServiceWorkerCacheWriter(
-      nullptr /* compare_reader */, nullptr /* copy_reader */,
-      std::move(writer), false /* pause_when_not_identical*/));
-}
-
-std::unique_ptr<ServiceWorkerCacheWriter>
-ServiceWorkerCacheWriter::CreateForComparison(
-    std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-    std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-    std::unique_ptr<ServiceWorkerResponseWriter> writer,
-    bool pause_when_not_identical) {
-  // |compare_reader| reads data for the comparison. |copy_reader| reads
-  // data for copy.
-  DCHECK(compare_reader);
-  DCHECK(copy_reader);
-  DCHECK(writer);
-  return base::WrapUnique(new ServiceWorkerCacheWriter(
-      std::move(compare_reader), std::move(copy_reader), std::move(writer),
-      pause_when_not_identical));
+      /*compare_reader=*/{}, /*copy_reader=*/{}, std::move(writer),
+      /* pause_when_not_identical=*/false));
 }
 
 std::unique_ptr<ServiceWorkerCacheWriter>
@@ -381,8 +341,7 @@ net::Error ServiceWorkerCacheWriter::StartCopy(
 }
 
 bool ServiceWorkerCacheWriter::IsCopying() const {
-  return !(compare_reader_ || legacy_compare_reader_) &&
-         (copy_reader_ || legacy_copy_reader_);
+  return !compare_reader_ && copy_reader_;
 }
 
 int64_t ServiceWorkerCacheWriter::WriterResourceId() const {
@@ -392,7 +351,7 @@ int64_t ServiceWorkerCacheWriter::WriterResourceId() const {
 
 int ServiceWorkerCacheWriter::DoStart(int result) {
   bytes_written_ = 0;
-  if (compare_reader_ || legacy_compare_reader_) {
+  if (compare_reader_) {
     state_ = STATE_READ_HEADERS_FOR_COMPARE;
     comparing_ = true;
   } else if (IsCopying()) {
@@ -409,13 +368,10 @@ int ServiceWorkerCacheWriter::DoStart(int result) {
 int ServiceWorkerCacheWriter::DoReadHeadersForCompare(int result) {
   DCHECK_GE(result, 0);
   DCHECK(response_head_to_write_);
+  DCHECK(compare_reader_);
 
   state_ = STATE_READ_HEADERS_FOR_COMPARE_DONE;
-  if (compare_reader_) {
-    return ReadResponseHead(compare_reader_.get());
-  } else {
-    return ReadResponseHead(legacy_compare_reader_);
-  }
+  return ReadResponseHead(compare_reader_.get());
 }
 
 int ServiceWorkerCacheWriter::DoReadHeadersForCompareDone(int result) {
@@ -440,13 +396,9 @@ int ServiceWorkerCacheWriter::DoReadDataForCompare(int result) {
   compare_offset_ = 0;
   // If this was an EOF, don't issue a read.
   if (len_to_write_ > 0) {
-    if (compare_reader_) {
-      result = ReadDataHelper(compare_reader_.get(), compare_data_pipe_reader_,
-                              data_to_read_.get(), len_to_read_);
-    } else {
-      result = ReadDataHelper(legacy_compare_reader_, data_to_read_.get(),
-                              len_to_read_);
-    }
+    DCHECK(compare_reader_);
+    result = ReadDataHelper(compare_reader_.get(), compare_data_pipe_reader_,
+                            data_to_read_.get(), len_to_read_);
   }
   return result;
 }
@@ -496,15 +448,10 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
   // Compare isn't complete yet. Issue another read for the remaining data. Note
   // that this reuses the same IOBuffer.
   if (compare_offset_ < static_cast<size_t>(len_to_read_)) {
+    DCHECK(compare_reader_);
     state_ = STATE_READ_DATA_FOR_COMPARE_DONE;
-    if (compare_reader_) {
-      return ReadDataHelper(compare_reader_.get(), compare_data_pipe_reader_,
-                            data_to_read_.get(),
-                            len_to_read_ - compare_offset_);
-    } else {
-      return ReadDataHelper(legacy_compare_reader_, data_to_read_.get(),
-                            len_to_read_ - compare_offset_);
-    }
+    return ReadDataHelper(compare_reader_.get(), compare_data_pipe_reader_,
+                          data_to_read_.get(), len_to_read_ - compare_offset_);
   }
 
   // Cached entry is longer than the network entry but the prefix matches. Copy
@@ -523,15 +470,11 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
 
 int ServiceWorkerCacheWriter::DoReadHeadersForCopy(int result) {
   DCHECK_GE(result, 0);
-  DCHECK(copy_reader_ || legacy_copy_reader_);
+  DCHECK(copy_reader_);
   bytes_copied_ = 0;
   data_to_copy_ = base::MakeRefCounted<net::IOBuffer>(kCopyBufferSize);
   state_ = STATE_READ_HEADERS_FOR_COPY_DONE;
-  if (copy_reader_) {
-    return ReadResponseHead(copy_reader_.get());
-  } else {
-    return ReadResponseHead(legacy_copy_reader_);
-  }
+  return ReadResponseHead(copy_reader_.get());
 }
 
 int ServiceWorkerCacheWriter::DoReadHeadersForCopyDone(int result) {
@@ -590,12 +533,8 @@ int ServiceWorkerCacheWriter::DoReadDataForCopy(int result) {
     return net::OK;
   }
   state_ = STATE_READ_DATA_FOR_COPY_DONE;
-  if (copy_reader_) {
-    return ReadDataHelper(copy_reader_.get(), copy_data_pipe_reader_,
-                          data_to_copy_.get(), to_read);
-  } else {
-    return ReadDataHelper(legacy_copy_reader_, data_to_copy_.get(), to_read);
-  }
+  return ReadDataHelper(copy_reader_.get(), copy_data_pipe_reader_,
+                        data_to_copy_.get(), to_read);
 }
 
 int ServiceWorkerCacheWriter::DoReadDataForCopyDone(int result) {
@@ -678,32 +617,6 @@ int ServiceWorkerCacheWriter::ReadResponseHead(
   return adapter->result();
 }
 
-int ServiceWorkerCacheWriter::ReadResponseHead(
-    const std::unique_ptr<ServiceWorkerResponseReader>& reader) {
-  auto adapter = base::MakeRefCounted<ReadResponseHeadCallbackAdapter>(
-      weak_factory_.GetWeakPtr());
-  reader->ReadResponseHead(base::BindOnce(
-      &ReadResponseHeadCallbackAdapter::DidReadResponseInfo, adapter));
-  adapter->SetAsync();
-  return adapter->result();
-}
-
-int ServiceWorkerCacheWriter::ReadDataHelper(
-    const std::unique_ptr<ServiceWorkerResponseReader>& reader,
-    net::IOBuffer* buf,
-    int buf_len) {
-  net::CompletionOnceCallback run_callback = base::BindOnce(
-      &ServiceWorkerCacheWriter::AsyncDoLoop, weak_factory_.GetWeakPtr());
-  scoped_refptr<AsyncOnlyCompletionCallbackAdaptor> adaptor(
-      new AsyncOnlyCompletionCallbackAdaptor(std::move(run_callback)));
-  reader->ReadData(
-      buf, buf_len,
-      base::BindOnce(&AsyncOnlyCompletionCallbackAdaptor::WrappedCallback,
-                     adaptor));
-  adaptor->set_async(true);
-  return adaptor->result();
-}
-
 class ServiceWorkerCacheWriter::DataPipeReader {
  public:
   DataPipeReader(storage::mojom::ServiceWorkerResourceReader* reader,

content/browser/service_worker/service_worker_cache_writer.h

@@ -22,7 +22,6 @@
 
 namespace content {
 
-class ServiceWorkerResponseReader;
 class ServiceWorkerResponseWriter;
 
 // This class is responsible for possibly updating the ServiceWorker script
@@ -71,11 +70,6 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
 
   // Create a cache writer instance that copies a script already in storage. The
   // script is read by |copy_reader|.
-  // Non-storage service:
-  static std::unique_ptr<ServiceWorkerCacheWriter> CreateForCopy(
-      std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-      std::unique_ptr<ServiceWorkerResponseWriter> writer);
-  // Storage service:
   static std::unique_ptr<ServiceWorkerCacheWriter> CreateForCopy(
       mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader,
       std::unique_ptr<ServiceWorkerResponseWriter> writer);
@@ -95,13 +89,6 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
   // resumed later. If |pause_when_not_identical| is false, and the data is
   // different, it would be written to storage directly. |copy_reader| is used
   // for copying identical data blocks during writing.
-  // Non-storage service:
-  static std::unique_ptr<ServiceWorkerCacheWriter> CreateForComparison(
-      std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-      std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-      std::unique_ptr<ServiceWorkerResponseWriter> writer,
-      bool pause_when_not_identical);
-  // Storage service:
   static std::unique_ptr<ServiceWorkerCacheWriter> CreateForComparison(
       mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader,
       mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader,
@@ -221,13 +208,6 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
     STATE_DONE,
   };
 
-  // Non-storage service:
-  ServiceWorkerCacheWriter(
-      std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-      std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-      std::unique_ptr<ServiceWorkerResponseWriter> writer,
-      bool pause_when_not_identical);
-  // Storage service:
   ServiceWorkerCacheWriter(
       mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader,
       mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader,
@@ -268,18 +248,8 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
   int DoDone(int result);
 
   // Wrappers for asynchronous calls. These are responsible for scheduling a
-  // callback to drive the state machine if needed. These either:
-  //   a) Return ERR_IO_PENDING, and schedule a callback to run the state
-  //      machine's Run() later, or
-  //   b) Return some other value and do not schedule a callback.
-  // Non-storage service:
-  int ReadResponseHead(
-      const std::unique_ptr<ServiceWorkerResponseReader>& reader);
-  int ReadDataHelper(const std::unique_ptr<ServiceWorkerResponseReader>& reader,
-                     net::IOBuffer* buf,
-                     int buf_len);
-  // Storage service:
-  // These are always case a) above.
+  // callback to drive the state machine if needed. These return ERR_IO_PENDING,
+  // and schedule a callback to run the state machine's Run() later.
   int ReadResponseHead(storage::mojom::ServiceWorkerResourceReader* reader);
   int ReadDataHelper(storage::mojom::ServiceWorkerResourceReader* reader,
                      std::unique_ptr<DataPipeReader>& data_pipe_reader,
@@ -355,10 +325,6 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
 
   WriteObserver* write_observer_ = nullptr;
 
-  // Non-storage service:
-  std::unique_ptr<ServiceWorkerResponseReader> legacy_compare_reader_;
-  std::unique_ptr<ServiceWorkerResponseReader> legacy_copy_reader_;
-  // Storage service:
   mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader_;
   std::unique_ptr<DataPipeReader> compare_data_pipe_reader_;
   mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader_;

content/browser/service_worker/service_worker_registry.h

@@ -233,6 +233,9 @@ class CONTENT_EXPORT ServiceWorkerRegistry {
       const std::string& key_prefix,
       GetUserDataForAllRegistrationsCallback callback);
 
+  mojo::Remote<storage::mojom::ServiceWorkerStorageControl>&
+  GetRemoteStorageControl();
+
   // Disables the internal storage to prepare for error recovery.
   void PrepareForDeleteAndStarOver();
 
@@ -356,9 +359,6 @@ class CONTENT_EXPORT ServiceWorkerRegistry {
   void OnStoragePolicyChanged();
   bool ShouldPurgeOnShutdown(const url::Origin& origin);
 
-  mojo::Remote<storage::mojom::ServiceWorkerStorageControl>&
-  GetRemoteStorageControl();
-
   // The ServiceWorkerContextCore object must outlive this.
   ServiceWorkerContextCore* const context_;
 

content/browser/service_worker/service_worker_script_loader_factory.cc

@@ -196,11 +196,13 @@ void ServiceWorkerScriptLoaderFactory::CopyScript(
     int64_t resource_id,
     base::OnceCallback<void(int64_t, net::Error)> callback,
     int64_t new_resource_id) {
-  ServiceWorkerStorage* storage = context_->storage();
+  mojo::Remote<storage::mojom::ServiceWorkerResourceReader> reader;
+  context_->registry()->GetRemoteStorageControl()->CreateResourceReader(
+      resource_id, reader.BindNewPipeAndPassReceiver());
 
   cache_writer_ = ServiceWorkerCacheWriter::CreateForCopy(
-      storage->CreateResponseReader(resource_id),
-      storage->CreateResponseWriter(new_resource_id));
+      std::move(reader),
+      context_->storage()->CreateResponseWriter(new_resource_id));
 
   scoped_refptr<ServiceWorkerVersion> version = worker_host_->version();
   version->script_cache_map()->NotifyStartedCaching(url, new_resource_id);

content/browser/service_worker/service_worker_single_script_update_checker.cc

@@ -106,8 +106,8 @@ ServiceWorkerSingleScriptUpdateChecker::ServiceWorkerSingleScriptUpdateChecker(
     ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
         browser_context_getter,
     scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
-    std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-    std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
+    mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader,
+    mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader,
     std::unique_ptr<ServiceWorkerResponseWriter> writer,
     ResultCallback callback)
     : script_url_(script_url),

content/browser/service_worker/service_worker_single_script_update_checker.h

@@ -121,8 +121,8 @@ class CONTENT_EXPORT ServiceWorkerSingleScriptUpdateChecker
       ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
           browser_context_getter,
       scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
-      std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-      std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
+      mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader,
+      mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader,
       std::unique_ptr<ServiceWorkerResponseWriter> writer,
       ResultCallback callback);
 

content/browser/service_worker/service_worker_single_script_update_checker_unittest.cc

@@ -90,8 +90,8 @@ class ServiceWorkerSingleScriptUpdateCheckerTest : public testing::Test {
   CreateSingleScriptUpdateCheckerWithoutHttpCache(
       const char* url,
       const GURL& scope,
-      std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-      std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
+      std::unique_ptr<MockServiceWorkerResponseReader> compare_reader,
+      std::unique_ptr<MockServiceWorkerResponseReader> copy_reader,
       std::unique_ptr<ServiceWorkerResponseWriter> writer,
       network::TestURLLoaderFactory* loader_factory,
       base::Optional<CheckResult>* out_check_result) {
@@ -103,6 +103,17 @@ class ServiceWorkerSingleScriptUpdateCheckerTest : public testing::Test {
         loader_factory, out_check_result);
   }
 
+  mojo::Remote<storage::mojom::ServiceWorkerResourceReader> WrapReader(
+      std::unique_ptr<MockServiceWorkerResponseReader> reader) {
+    mojo::Remote<storage::mojom::ServiceWorkerResourceReader> remote;
+    remote.Bind(reader->BindNewPipeAndPassRemote(base::BindOnce(
+        [](std::unique_ptr<MockServiceWorkerResponseReader>) {
+          // Keep |reader| until mojo connection is destroyed.
+        },
+        std::move(reader))));
+    return remote;
+  }
+
   // Note that |loader_factory| should be alive as long as the single script
   // update checker is running.
   std::unique_ptr<ServiceWorkerSingleScriptUpdateChecker>
@@ -113,8 +124,8 @@ class ServiceWorkerSingleScriptUpdateCheckerTest : public testing::Test {
       bool force_bypass_cache,
       blink::mojom::ServiceWorkerUpdateViaCache update_via_cache,
       base::TimeDelta time_since_last_check,
-      std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
-      std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
+      std::unique_ptr<MockServiceWorkerResponseReader> compare_reader,
+      std::unique_ptr<MockServiceWorkerResponseReader> copy_reader,
       std::unique_ptr<ServiceWorkerResponseWriter> writer,
       network::TestURLLoaderFactory* loader_factory,
       base::Optional<CheckResult>* out_check_result) {
@@ -131,7 +142,8 @@ class ServiceWorkerSingleScriptUpdateCheckerTest : public testing::Test {
                             browser_context_.get()),
         base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
             loader_factory),
-        std::move(compare_reader), std::move(copy_reader), std::move(writer),
+        WrapReader(std::move(compare_reader)),
+        WrapReader(std::move(copy_reader)), std::move(writer),
         base::BindOnce(
             [](base::Optional<CheckResult>* out_check_result_param,
                const GURL& script_url,

content/browser/service_worker/service_worker_test_utils.cc

@@ -776,11 +776,22 @@ ServiceWorkerUpdateCheckTestUtils::CreatePausedCacheWriter(
     uint32_t consumed_size,
     int64_t old_resource_id,
     int64_t new_resource_id) {
+  mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader;
+  worker_test_helper->context()
+      ->registry()
+      ->GetRemoteStorageControl()
+      ->CreateResourceReader(old_resource_id,
+                             compare_reader.BindNewPipeAndPassReceiver());
+
+  mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader;
+  worker_test_helper->context()
+      ->registry()
+      ->GetRemoteStorageControl()
+      ->CreateResourceReader(old_resource_id,
+                             copy_reader.BindNewPipeAndPassReceiver());
+
   auto cache_writer = ServiceWorkerCacheWriter::CreateForComparison(
-      worker_test_helper->context()->storage()->CreateResponseReader(
-          old_resource_id),
-      worker_test_helper->context()->storage()->CreateResponseReader(
-          old_resource_id),
+      std::move(compare_reader), std::move(copy_reader),
       worker_test_helper->context()->storage()->CreateResponseWriter(
           new_resource_id),
       true /* pause_when_not_identical */);

content/browser/service_worker/service_worker_update_checker.cc

@@ -266,11 +266,16 @@ void ServiceWorkerUpdateChecker::OnResourceIdAssignedForOneScriptCheck(
   const bool is_main_script = url == main_script_url_;
 
   ServiceWorkerStorage* storage = version_to_update_->context()->storage();
+  ServiceWorkerRegistry* registry = version_to_update_->context()->registry();
 
   // We need two identical readers for comparing and reading the resource for
   // |resource_id| from the storage.
-  auto compare_reader = storage->CreateResponseReader(resource_id);
-  auto copy_reader = storage->CreateResponseReader(resource_id);
+  mojo::Remote<storage::mojom::ServiceWorkerResourceReader> compare_reader;
+  registry->GetRemoteStorageControl()->CreateResourceReader(
+      resource_id, compare_reader.BindNewPipeAndPassReceiver());
+  mojo::Remote<storage::mojom::ServiceWorkerResourceReader> copy_reader;
+  registry->GetRemoteStorageControl()->CreateResourceReader(
+      resource_id, copy_reader.BindNewPipeAndPassReceiver());
 
   auto writer = storage->CreateResponseWriter(new_resource_id);
   running_checker_ = std::make_unique<ServiceWorkerSingleScriptUpdateChecker>(