Add pause_when_not_identical option for ServiceWorkerCacheWriter

This patch adds pause_when_not_identical option for making
ServiceWorkerCacheWriter::MaybeWriteData() return before writing the
new body into the storage when the scripts are not identical.
This option is necessary for extending byte-for-byte update check
of service worker importScripts() resources.

Bug: 648295
Change-Id: If33412cd6e63393ad3fdae514b1588a33f17399a
Reviewed-on: https://chromium-review.googlesource.com/1198651
Commit-Queue: Momoko Hattori <momohatt@google.com>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589492}

content/browser/service_worker/service_worker_cache_writer.cc

@@ -117,11 +117,12 @@ int ServiceWorkerCacheWriter::DoLoop(int status) {
 ServiceWorkerCacheWriter::ServiceWorkerCacheWriter(
     std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
     std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-    std::unique_ptr<ServiceWorkerResponseWriter> writer)
+    std::unique_ptr<ServiceWorkerResponseWriter> writer,
+    bool pause_when_not_identical)
     : state_(STATE_START),
       io_pending_(false),
       comparing_(false),
-      did_replace_(false),
+      pause_when_not_identical_(pause_when_not_identical),
       compare_reader_(std::move(compare_reader)),
       copy_reader_(std::move(copy_reader)),
       writer_(std::move(writer)),
@@ -185,6 +186,12 @@ net::Error ServiceWorkerCacheWriter::MaybeWriteData(
     // STATE_WRITE_HEADERS_FOR_PASSTHROUGH_DONE is excluded because that write
     // is done by MaybeWriteHeaders.
     DCHECK(state_ == STATE_READ_DATA_FOR_COMPARE_DONE ||
+           // |state_| can be STATE_READ_HEADERS_FOR_COPY only when the
+           // |pause_when_not_identical_| option is enabled, and the cache
+           // writer finds that the network data is not identical with the
+           // stored data.
+           (state_ == STATE_READ_HEADERS_FOR_COPY &&
+            pause_when_not_identical_ && data_not_identical_) ||
            state_ == STATE_READ_HEADERS_FOR_COPY_DONE ||
            state_ == STATE_READ_DATA_FOR_COPY_DONE ||
            state_ == STATE_WRITE_HEADERS_FOR_COPY_DONE ||
@@ -192,7 +199,6 @@ net::Error ServiceWorkerCacheWriter::MaybeWriteData(
            state_ == STATE_WRITE_DATA_FOR_PASSTHROUGH_DONE)
         << "Unexpected state: " << state_;
   }
-
   return result >= 0 ? net::OK : static_cast<net::Error>(result);
 }
 
@@ -260,7 +266,8 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
   if (result == 0 && len_to_write_ != 0) {
     comparing_ = false;
     state_ = STATE_READ_HEADERS_FOR_COPY;
-    return net::OK;
+    data_not_identical_ = true;
+    return pause_when_not_identical_ ? net::ERR_IO_PENDING : net::OK;
   }
 
   // Compare the data from the ServiceWorker script cache to the data from the
@@ -272,7 +279,8 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
     // over, then start writing network data back after the changed point.
     comparing_ = false;
     state_ = STATE_READ_HEADERS_FOR_COPY;
-    return net::OK;
+    data_not_identical_ = true;
+    return pause_when_not_identical_ ? net::ERR_IO_PENDING : net::OK;
   }
 
   compare_offset_ += result;
@@ -295,7 +303,8 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
   if (len_to_read_ == 0 && bytes_compared_ + compare_offset_ < cached_length_) {
     comparing_ = false;
     state_ = STATE_READ_HEADERS_FOR_COPY;
-    return net::OK;
+    data_not_identical_ = true;
+    return pause_when_not_identical_ ? net::ERR_IO_PENDING : net::OK;
   }
 
   bytes_compared_ += compare_offset_;
@@ -492,10 +501,14 @@ void ServiceWorkerCacheWriter::AsyncDoLoop(int result) {
   // later invocation of AsyncDoLoop.
   if (result != net::ERR_IO_PENDING) {
     OnWriteCompleteCallback callback = std::move(pending_callback_);
-    pending_callback_.Reset();
     net::Error error = result >= 0 ? net::OK : static_cast<net::Error>(result);
     io_pending_ = false;
     std::move(callback).Run(error);
+    return;
+  }
+  if (pause_when_not_identical_ && data_not_identical_) {
+    OnWriteCompleteCallback callback = std::move(pending_callback_);
+    std::move(callback).Run(net::ERR_IO_PENDING);
   }
 }
 

content/browser/service_worker/service_worker_cache_writer.h

@@ -42,10 +42,16 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
   // The |compare_reader| may be null, in which case this instance will
   // unconditionally write back data supplied to |MaybeWriteHeaders| and
   // |MaybeWriteData|.
+  //
+  // When |pause_when_not_identical| is true and the cache writer detects a
+  // difference between bodies from the network and from the storage, the
+  // comparison stops immediately and the cache writer returns
+  // net::ERR_IO_PENDING, with nothing written to the storage.
   ServiceWorkerCacheWriter(
       std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
       std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
-      std::unique_ptr<ServiceWorkerResponseWriter> writer);
+      std::unique_ptr<ServiceWorkerResponseWriter> writer,
+      bool pause_when_not_identical);
 
   ~ServiceWorkerCacheWriter();
 
@@ -73,6 +79,7 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
   // Returns a count of bytes written back to the cache.
   size_t bytes_written() const { return bytes_written_; }
   bool did_replace() const { return did_replace_; }
+  bool data_not_identical() const { return data_not_identical_; }
 
  private:
   // States for the state machine.
@@ -219,7 +226,14 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
   // Count of bytes written back to |writer_|.
   size_t bytes_written_;
 
-  bool did_replace_;
+  bool did_replace_ = false;
+
+  // When the cache writer finds any differences between bodies from the network
+  // and from the storage, |data_not_identical_| is set to true. At the same
+  // time, if the |pause_when_not_identical_| is true, the cache writer pauses
+  // immediately.
+  const bool pause_when_not_identical_;
+  bool data_not_identical_ = false;
 
   std::unique_ptr<ServiceWorkerResponseReader> compare_reader_;
   std::unique_ptr<ServiceWorkerResponseReader> copy_reader_;

content/browser/service_worker/service_worker_cache_writer_unittest.cc

@@ -341,12 +341,13 @@ class ServiceWorkerCacheWriterTest : public ::testing::Test {
   }
 
   // This should be called after ExpectReader() and ExpectWriter().
-  void Initialize() {
+  void Initialize(bool pause_when_not_identical) {
     std::unique_ptr<ServiceWorkerResponseReader> compare_reader(CreateReader());
     std::unique_ptr<ServiceWorkerResponseReader> copy_reader(CreateReader());
     std::unique_ptr<ServiceWorkerResponseWriter> writer(CreateWriter());
     cache_writer_.reset(new ServiceWorkerCacheWriter(
-        std::move(compare_reader), std::move(copy_reader), std::move(writer)));
+        std::move(compare_reader), std::move(copy_reader), std::move(writer),
+        pause_when_not_identical));
   }
 
  protected:
@@ -411,7 +412,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughHeadersSync) {
   const size_t kHeaderSize = 16;
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   writer->ExpectWriteInfoOk(kHeaderSize, false);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(kHeaderSize);
   EXPECT_EQ(net::OK, error);
@@ -424,7 +425,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughHeadersAsync) {
   size_t kHeaderSize = 16;
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   writer->ExpectWriteInfoOk(kHeaderSize, true);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(kHeaderSize);
   EXPECT_EQ(net::ERR_IO_PENDING, error);
@@ -445,7 +446,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughDataSync) {
   writer->ExpectWriteInfoOk(response_size, false);
   writer->ExpectWriteDataOk(data1.size(), false);
   writer->ExpectWriteDataOk(data2.size(), false);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(response_size);
   EXPECT_EQ(net::OK, error);
@@ -467,7 +468,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughDataAsync) {
   writer->ExpectWriteInfoOk(response_size, false);
   writer->ExpectWriteDataOk(data1.size(), true);
   writer->ExpectWriteDataOk(data2.size(), true);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(response_size);
   EXPECT_EQ(net::OK, error);
@@ -490,7 +491,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughHeadersFailSync) {
   const size_t kHeaderSize = 16;
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   writer->ExpectWriteInfo(kHeaderSize, false, net::ERR_FAILED);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(kHeaderSize);
   EXPECT_EQ(net::ERR_FAILED, error);
@@ -503,7 +504,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughHeadersFailAsync) {
   size_t kHeaderSize = 16;
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   writer->ExpectWriteInfo(kHeaderSize, true, net::ERR_FAILED);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(kHeaderSize);
   EXPECT_EQ(net::ERR_IO_PENDING, error);
@@ -521,7 +522,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughDataFailSync) {
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   writer->ExpectWriteInfoOk(data.size(), false);
   writer->ExpectWriteData(data.size(), false, net::ERR_FAILED);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   EXPECT_EQ(net::OK, WriteHeaders(data.size()));
   EXPECT_EQ(net::ERR_FAILED, WriteData(data));
@@ -534,7 +535,7 @@ TEST_F(ServiceWorkerCacheWriterTest, PassthroughDataFailAsync) {
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   writer->ExpectWriteInfoOk(data.size(), false);
   writer->ExpectWriteData(data.size(), true, net::ERR_FAILED);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   EXPECT_EQ(net::OK, WriteHeaders(data.size()));
 
@@ -556,7 +557,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareHeadersSync) {
   MockServiceWorkerResponseReader* reader = ExpectReader();
 
   reader->ExpectReadInfoOk(response_size, false);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(response_size);
   EXPECT_EQ(net::OK, error);
@@ -573,7 +574,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareDataOkSync) {
 
   reader->ExpectReadInfoOk(response_size, false);
   reader->ExpectReadDataOk(data1, false);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(response_size);
   EXPECT_EQ(net::OK, error);
@@ -592,7 +593,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareHeadersFailSync) {
   MockServiceWorkerResponseReader* reader = ExpectReader();
 
   reader->ExpectReadInfo(response_size, false, net::ERR_FAILED);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   EXPECT_EQ(net::ERR_FAILED, WriteHeaders(response_size));
   EXPECT_TRUE(writer->AllExpectedWritesDone());
@@ -608,7 +609,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareDataFailSync) {
 
   reader->ExpectReadInfoOk(response_size, false);
   reader->ExpectReadData(data1.c_str(), data1.length(), false, net::ERR_FAILED);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(response_size);
   EXPECT_EQ(net::OK, error);
@@ -636,7 +637,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareShortCacheReads) {
   reader->ExpectReadDataOk(cache_data3, false);
   reader->ExpectReadDataOk(cache_data4, false);
   reader->ExpectReadDataOk(data5, false);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(kHeaderSize);
   EXPECT_EQ(net::OK, error);
@@ -658,7 +659,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareDataOkAsync) {
 
   reader->ExpectReadInfoOk(response_size, true);
   reader->ExpectReadDataOk(data1, true);
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(response_size);
   EXPECT_EQ(net::ERR_IO_PENDING, error);
@@ -686,7 +687,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareDataManyOkAsync) {
   for (size_t i = 0; i < arraysize(expected_data); ++i) {
     reader->ExpectReadDataOk(expected_data[i], true);
   }
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(response_size);
   EXPECT_EQ(net::ERR_IO_PENDING, error);
@@ -731,7 +732,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareFailedCopySync) {
   writer->ExpectWriteDataOk(net_data2.size(), false);
   writer->ExpectWriteDataOk(data3.size(), false);
 
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(net_response_size);
   EXPECT_EQ(net::OK, error);
@@ -773,7 +774,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareFailedCopyShort) {
   writer->ExpectWriteDataOk(net_data2.size(), false);
   writer->ExpectWriteDataOk(data3.size(), false);
 
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(net_response_size);
   EXPECT_EQ(net::OK, error);
@@ -817,7 +818,7 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareFailedCopyLong) {
   writer->ExpectWriteDataOk(data1.size(), false);
   writer->ExpectWriteDataOk(net_data2.size(), false);
 
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(net_size);
   EXPECT_EQ(net::OK, error);
@@ -880,7 +881,7 @@ TEST_F(ServiceWorkerCacheWriterTest, MultipleComparisonInSingleWrite) {
   for (const auto& data : data_expected)
     writer->ExpectWriteDataOk(data.size(), false);
 
-  Initialize();
+  Initialize(false /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(bytes_from_net);
   EXPECT_EQ(net::OK, error);
@@ -894,5 +895,121 @@ TEST_F(ServiceWorkerCacheWriterTest, MultipleComparisonInSingleWrite) {
   EXPECT_TRUE(copy_reader->AllExpectedReadsDone());
 }
 
+// Tests behavior when |pause_when_not_identical| is enabled and cache writer
+// finishes synchronously.
+TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_SyncWriteData) {
+  // Data from |compare_reader|.
+  const std::vector<std::string> data_from_cache{"abcd"};
+
+  // Data for |writer|. The comparison should stop at the first block of the
+  // data.
+  const std::vector<std::string> data_from_net{"abxx"};
+
+  // We don't need |data_to_copy| or |data_expected| here because the
+  // cache writer doesn't copy from the storage or write to the storage
+  // when |pause_when_not_identical| option is enabled.
+
+  size_t bytes_cached = 0;
+  size_t bytes_from_net = 0;
+
+  for (const auto& data : data_from_cache)
+    bytes_cached += data.size();
+
+  for (const auto& data : data_from_net)
+    bytes_from_net += data.size();
+
+  MockServiceWorkerResponseWriter* writer = ExpectWriter();
+  MockServiceWorkerResponseReader* compare_reader = ExpectReader();
+  MockServiceWorkerResponseReader* copy_reader = ExpectReader();
+
+  compare_reader->ExpectReadInfoOk(bytes_cached, false);
+  for (const auto& data : data_from_cache)
+    compare_reader->ExpectReadDataOk(data, false);
+
+  Initialize(true /* pause_when_not_identical */);
+
+  net::Error error = WriteHeaders(bytes_from_net);
+  EXPECT_EQ(net::OK, error);
+
+  // |cache_writer_| stops the comparison at the first block of the data.
+  // It should return net::ERR_IO_PENDING and |write_complete_| should remain
+  // false since |pause_when_not_identical| forbids proceeding to the next step.
+  write_complete_ = false;
+  error = WriteData(data_from_net[0]);
+  EXPECT_EQ(net::ERR_IO_PENDING, error);
+  EXPECT_FALSE(write_complete_);
+  EXPECT_TRUE(cache_writer_->data_not_identical());
+
+  EXPECT_TRUE(writer->AllExpectedWritesDone());
+  EXPECT_TRUE(compare_reader->AllExpectedReadsDone());
+  EXPECT_TRUE(copy_reader->AllExpectedReadsDone());
+  EXPECT_EQ(0U, cache_writer_->bytes_written());
+}
+
+// Tests behavior when |pause_when_not_identical| is enabled and cache writer
+// finishes asynchronously.
+TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_AsyncWriteData) {
+  // Data from |compare_reader|.
+  const std::vector<std::string> data_from_cache{"abcd"};
+
+  // Data for |writer|. The comparison should stop at the first block of the
+  // data.
+  const std::vector<std::string> data_from_net{"abxx"};
+
+  // We don't need |data_to_copy| or |data_expected| here because the
+  // cache writer doesn't copy from the storage or write to the storage
+  // when |pause_when_not_identical| option is enabled.
+
+  size_t bytes_cached = 0;
+  size_t bytes_from_net = 0;
+
+  for (const auto& data : data_from_cache)
+    bytes_cached += data.size();
+
+  for (const auto& data : data_from_net)
+    bytes_from_net += data.size();
+
+  MockServiceWorkerResponseWriter* writer = ExpectWriter();
+  MockServiceWorkerResponseReader* compare_reader = ExpectReader();
+  MockServiceWorkerResponseReader* copy_reader = ExpectReader();
+
+  compare_reader->ExpectReadInfoOk(bytes_cached, true);
+  for (const auto& data : data_from_cache)
+    compare_reader->ExpectReadDataOk(data, true);
+
+  Initialize(true /* pause_when_not_identical */);
+
+  write_complete_ = false;
+  net::Error error = WriteHeaders(bytes_from_net);
+  EXPECT_EQ(net::ERR_IO_PENDING, error);
+  EXPECT_FALSE(write_complete_);
+  compare_reader->CompletePendingRead();
+  EXPECT_TRUE(write_complete_);
+
+  // The comparison is suspended due to an asynchronous read of
+  // |compare_reader|, resulting in an early return. At this point, the callback
+  // shouldn't be called yet.
+  write_complete_ = false;
+  error = WriteData(data_from_net[0]);
+  EXPECT_EQ(net::ERR_IO_PENDING, error);
+  EXPECT_FALSE(write_complete_);
+  EXPECT_FALSE(cache_writer_->data_not_identical());
+
+  // When |compare_reader| succeeds in reading the stored data, |cache_writer_|
+  // then proceeds to the comparison phase.
+  // |cache_writer_| stops comparison at the first block of the data.
+  // Since |pause_when_not_identical| is enabled, it should subsequently trigger
+  // the callback and return net::ERR_IO_PENDING.
+  compare_reader->CompletePendingRead();
+  EXPECT_TRUE(write_complete_);
+  EXPECT_EQ(net::ERR_IO_PENDING, last_error_);
+  EXPECT_TRUE(cache_writer_->data_not_identical());
+
+  EXPECT_TRUE(writer->AllExpectedWritesDone());
+  EXPECT_TRUE(compare_reader->AllExpectedReadsDone());
+  EXPECT_TRUE(copy_reader->AllExpectedReadsDone());
+  EXPECT_EQ(0U, cache_writer_->bytes_written());
+}
+
 }  // namespace
 }  // namespace content

content/browser/service_worker/service_worker_new_script_loader.cc

@@ -115,7 +115,8 @@ ServiceWorkerNewScriptLoader::ServiceWorkerNewScriptLoader(
   }
   cache_writer_ = std::make_unique<ServiceWorkerCacheWriter>(
       std::move(compare_reader), std::move(copy_reader),
-      storage->CreateResponseWriter(cache_resource_id));
+      storage->CreateResponseWriter(cache_resource_id),
+      false /* pause_when_not_identical */);
 
   version_->script_cache_map()->NotifyStartedCaching(request_url_,
                                                      cache_resource_id);

content/browser/service_worker/service_worker_write_to_cache_job.cc

@@ -112,7 +112,8 @@ void ServiceWorkerWriteToCacheJob::StartAsync() {
   }
   cache_writer_ = std::make_unique<ServiceWorkerCacheWriter>(
       std::move(compare_reader), std::move(copy_reader),
-      context_->storage()->CreateResponseWriter(resource_id_));
+      context_->storage()->CreateResponseWriter(resource_id_),
+      false /* pause_when_not_identical */);
 
   version_->script_cache_map()->NotifyStartedCaching(url_, resource_id_);
   did_notify_started_ = true;