Add ServiceWorkerCacheWriter::Resume() for paused cache writers

This method resumes a cache writer which is paused by enabling
pause_when_not_identical option. With this option enabled, a cache
writer pauses right after it finds any differences with the cached body
and the body from network, and calling Resume() to such
paused cache writers makes them resume its internal state machine and
complete the remaining read and write to the storage. This method takes
a callback, but it is triggered only when the remaining read and write
finishes asynchronously.

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

content/browser/service_worker/service_worker_cache_writer.cc

@@ -137,12 +137,12 @@ net::Error ServiceWorkerCacheWriter::MaybeWriteHeaders(
 
   headers_to_write_ = headers;
   pending_callback_ = std::move(callback);
-  DCHECK_EQ(state_, STATE_START);
+  DCHECK_EQ(STATE_START, state_);
   int result = DoLoop(net::OK);
 
   // Synchronous errors and successes always go to STATE_DONE.
   if (result != net::ERR_IO_PENDING)
-    DCHECK_EQ(state_, STATE_DONE);
+    DCHECK_EQ(STATE_DONE, state_);
 
   // ERR_IO_PENDING has to have one of the STATE_*_DONE states as the next state
   // (not STATE_DONE itself).
@@ -176,7 +176,7 @@ net::Error ServiceWorkerCacheWriter::MaybeWriteData(
 
   // Synchronous completions are always STATE_DONE.
   if (result != net::ERR_IO_PENDING)
-    DCHECK_EQ(state_, STATE_DONE);
+    DCHECK_EQ(STATE_DONE, state_);
 
   // Asynchronous completion means the state machine must be waiting in one of
   // the Done states for an IO operation to complete:
@@ -186,12 +186,7 @@ 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_PAUSING ||
            state_ == STATE_READ_HEADERS_FOR_COPY_DONE ||
            state_ == STATE_READ_DATA_FOR_COPY_DONE ||
            state_ == STATE_WRITE_HEADERS_FOR_COPY_DONE ||
@@ -202,6 +197,39 @@ net::Error ServiceWorkerCacheWriter::MaybeWriteData(
   return result >= 0 ? net::OK : static_cast<net::Error>(result);
 }
 
+net::Error ServiceWorkerCacheWriter::Resume(OnWriteCompleteCallback callback) {
+  DCHECK(pause_when_not_identical_);
+  DCHECK_EQ(STATE_PAUSING, state_);
+  DCHECK(io_pending_);
+
+  io_pending_ = false;
+  pending_callback_ = std::move(callback);
+  state_ = STATE_READ_HEADERS_FOR_COPY;
+
+  int result = DoLoop(net::OK);
+
+  // Synchronous completions are always STATE_DONE.
+  if (result != net::ERR_IO_PENDING)
+    DCHECK_EQ(STATE_DONE, state_);
+
+  // Asynchronous completion means the state machine must be waiting in one of
+  // the Done states for an IO operation to complete:
+  if (result == net::ERR_IO_PENDING) {
+    // Note that STATE_READ_HEADERS_FOR_COMPARE_DONE is excluded because the
+    // headers are compared in MaybeWriteHeaders, not here, and
+    // STATE_WRITE_HEADERS_FOR_PASSTHROUGH_DONE is excluded because that write
+    // is done by MaybeWriteHeaders.
+    DCHECK(state_ == STATE_READ_HEADERS_FOR_COPY_DONE ||
+           state_ == STATE_READ_DATA_FOR_COPY_DONE ||
+           state_ == STATE_WRITE_HEADERS_FOR_COPY_DONE ||
+           state_ == STATE_WRITE_DATA_FOR_COPY_DONE ||
+           state_ == STATE_WRITE_DATA_FOR_PASSTHROUGH_DONE)
+        << "Unexpected state: " << state_;
+  }
+
+  return result >= 0 ? net::OK : static_cast<net::Error>(result);
+}
+
 int ServiceWorkerCacheWriter::DoStart(int result) {
   bytes_written_ = 0;
   if (compare_reader_) {
@@ -265,8 +293,8 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
   // compare. Fail the comparison.
   if (result == 0 && len_to_write_ != 0) {
     comparing_ = false;
-    state_ = STATE_READ_HEADERS_FOR_COPY;
-    data_not_identical_ = true;
+    state_ =
+        pause_when_not_identical_ ? STATE_PAUSING : STATE_READ_HEADERS_FOR_COPY;
     return pause_when_not_identical_ ? net::ERR_IO_PENDING : net::OK;
   }
 
@@ -278,8 +306,8 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
     // |bytes_compared_| were identical, so copy the first |bytes_compared_|
     // over, then start writing network data back after the changed point.
     comparing_ = false;
-    state_ = STATE_READ_HEADERS_FOR_COPY;
-    data_not_identical_ = true;
+    state_ =
+        pause_when_not_identical_ ? STATE_PAUSING : STATE_READ_HEADERS_FOR_COPY;
     return pause_when_not_identical_ ? net::ERR_IO_PENDING : net::OK;
   }
 
@@ -302,8 +330,8 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
   // just the prefix.
   if (len_to_read_ == 0 && bytes_compared_ + compare_offset_ < cached_length_) {
     comparing_ = false;
-    state_ = STATE_READ_HEADERS_FOR_COPY;
-    data_not_identical_ = true;
+    state_ =
+        pause_when_not_identical_ ? STATE_PAUSING : STATE_READ_HEADERS_FOR_COPY;
     return pause_when_not_identical_ ? net::ERR_IO_PENDING : net::OK;
   }
 
@@ -506,7 +534,8 @@ void ServiceWorkerCacheWriter::AsyncDoLoop(int result) {
     std::move(callback).Run(error);
     return;
   }
-  if (pause_when_not_identical_ && data_not_identical_) {
+  if (state_ == STATE_PAUSING) {
+    DCHECK(pause_when_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

@@ -79,7 +79,13 @@ 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_; }
+  bool is_pausing() const { return state_ == STATE_PAUSING; }
+
+  // Resumes a cache writer which were paused when a block of data from the
+  // network wasn't identical to the data in the storage. This method is valid
+  // only when |pause_when_not_identical| is true in the constructor and
+  // |state_| is STATE_PAUSING.
+  net::Error Resume(OnWriteCompleteCallback callback);
 
  private:
   // States for the state machine.
@@ -109,6 +115,10 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
     STATE_READ_DATA_FOR_COMPARE,
     STATE_READ_DATA_FOR_COMPARE_DONE,
 
+    // The cache writer is paused because the network data wasn't identical with
+    // the stored data, and |pause_when_not_identical| is true.
+    STATE_PAUSING,
+
     // Control flows linearly through these states, with each pass from
     // READ_DATA_FOR_COPY to WRITE_DATA_FOR_COPY_DONE copying one block of data
     // at a time. Control loops from WRITE_DATA_FOR_COPY_DONE back to
@@ -229,11 +239,9 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
   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.
+  // and from the storage, and 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

@@ -905,12 +905,15 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_SyncWriteData) {
   // 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.
+  // We don't need |data_to_copy| because the network data and the cached data
+  // have no common blocks.
+
+  // The written data should be the same as |data_from_net|.
+  const std::vector<std::string> data_expected{"abxx"};
 
   size_t bytes_cached = 0;
   size_t bytes_from_net = 0;
+  size_t bytes_expected = 0;
 
   for (const auto& data : data_from_cache)
     bytes_cached += data.size();
@@ -918,6 +921,9 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_SyncWriteData) {
   for (const auto& data : data_from_net)
     bytes_from_net += data.size();
 
+  for (const auto& data : data_expected)
+    bytes_expected += data.size();
+
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   MockServiceWorkerResponseReader* compare_reader = ExpectReader();
   MockServiceWorkerResponseReader* copy_reader = ExpectReader();
@@ -926,6 +932,12 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_SyncWriteData) {
   for (const auto& data : data_from_cache)
     compare_reader->ExpectReadDataOk(data, false);
 
+  copy_reader->ExpectReadInfoOk(bytes_cached, false);
+
+  writer->ExpectWriteInfoOk(bytes_expected, false);
+  for (const auto& data : data_expected)
+    writer->ExpectWriteDataOk(data.size(), false);
+
   Initialize(true /* pause_when_not_identical */);
 
   net::Error error = WriteHeaders(bytes_from_net);
@@ -938,12 +950,19 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_SyncWriteData) {
   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_EQ(0U, cache_writer_->bytes_written());
+
+  // Resume |cache_writer_| with a callback. The passed callback shouldn't be
+  // called because this is a synchronous write. This time, the result should be
+  // net::OK and the expected data should be written to the storage.
+  error =
+      cache_writer_->Resume(base::BindOnce([](net::Error) { NOTREACHED(); }));
+  EXPECT_EQ(net::OK, error);
+  EXPECT_EQ(bytes_expected, cache_writer_->bytes_written());
 
   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
@@ -956,12 +975,15 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_AsyncWriteData) {
   // 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.
+  // We don't need |data_to_copy| because the network data and the cached data
+  // have no common blocks.
+
+  // The written data should be the same as |data_from_net|.
+  const std::vector<std::string> data_expected{"abxx"};
 
   size_t bytes_cached = 0;
   size_t bytes_from_net = 0;
+  size_t bytes_expected = 0;
 
   for (const auto& data : data_from_cache)
     bytes_cached += data.size();
@@ -969,6 +991,9 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_AsyncWriteData) {
   for (const auto& data : data_from_net)
     bytes_from_net += data.size();
 
+  for (const auto& data : data_expected)
+    bytes_expected += data.size();
+
   MockServiceWorkerResponseWriter* writer = ExpectWriter();
   MockServiceWorkerResponseReader* compare_reader = ExpectReader();
   MockServiceWorkerResponseReader* copy_reader = ExpectReader();
@@ -977,6 +1002,12 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_AsyncWriteData) {
   for (const auto& data : data_from_cache)
     compare_reader->ExpectReadDataOk(data, true);
 
+  copy_reader->ExpectReadInfoOk(bytes_cached, true);
+
+  writer->ExpectWriteInfoOk(bytes_expected, true);
+  for (const auto& data : data_expected)
+    writer->ExpectWriteDataOk(data.size(), true);
+
   Initialize(true /* pause_when_not_identical */);
 
   write_complete_ = false;
@@ -993,7 +1024,6 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_AsyncWriteData) {
   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.
@@ -1003,12 +1033,36 @@ TEST_F(ServiceWorkerCacheWriterTest, PauseWhenNotIdentical_AsyncWriteData) {
   compare_reader->CompletePendingRead();
   EXPECT_TRUE(write_complete_);
   EXPECT_EQ(net::ERR_IO_PENDING, last_error_);
-  EXPECT_TRUE(cache_writer_->data_not_identical());
+  EXPECT_EQ(0U, cache_writer_->bytes_written());
+
+  // Resume |cache_writer_| with a callback which updates |write_complete_| and
+  // |last_error_| when it's called.
+  // |copy_reader| does an asynchronous read here.
+  write_complete_ = false;
+  error = cache_writer_->Resume(CreateWriteCallback());
+  EXPECT_EQ(net::ERR_IO_PENDING, error);
+  EXPECT_FALSE(write_complete_);
+
+  // Complete the asynchronous read of the header. Since there's nothing to copy
+  // from the storage, |copy_reader| should finish all its jobs here.
+  copy_reader->CompletePendingRead();
+  EXPECT_TRUE(copy_reader->AllExpectedReadsDone());
+
+  // Complete the asynchronous write of the header. This doesn't finish all the
+  // write to the storage, so the callback isn't called yet.
+  writer->CompletePendingWrite();
+  EXPECT_FALSE(write_complete_);
+  EXPECT_EQ(net::ERR_IO_PENDING, last_error_);
+
+  // Complete the asynchronous write of the body. This completes all the work of
+  // |cache_writer|, so the callback is triggered.
+  writer->CompletePendingWrite();
+  EXPECT_TRUE(write_complete_);
+  EXPECT_EQ(net::OK, last_error_);
+  EXPECT_EQ(bytes_expected, cache_writer_->bytes_written());
 
   EXPECT_TRUE(writer->AllExpectedWritesDone());
   EXPECT_TRUE(compare_reader->AllExpectedReadsDone());
-  EXPECT_TRUE(copy_reader->AllExpectedReadsDone());
-  EXPECT_EQ(0U, cache_writer_->bytes_written());
 }
 
 }  // namespace