[ServiceWorkerCacheWriter] fix update of bytes_compared_

This patch fixes the incorrect updating algorithm of |bytes_compared_|
in DoReadDataForCompareDone. This variable represents the amount of data
from the network which is confirmed to be identical with the cached data,
and is updated every time the comparison finishes for a chunk of data
from the network. Previously it was incremented only by the amount of
bytes read from the cache, which can be less than the size of the chunk
of data from the network in the cases where the cache writer does not
finish the comparison in single read from the cache because the network
data is too large.

Bug: 871655
Change-Id: Ie3e0f192713b5d38582628c72175e4f7aa6771da
Reviewed-on: https://chromium-review.googlesource.com/1172222Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Momoko Hattori <momohatt@google.com>
Cr-Commit-Position: refs/heads/master@{#584353}

content/browser/service_worker/service_worker_cache_writer.cc

@@ -298,9 +298,7 @@ int ServiceWorkerCacheWriter::DoReadDataForCompareDone(int result) {
     return net::OK;
   }
 
-  // bytes_compared_ only gets incremented when a full block is compared, to
-  // avoid having to use only parts of the buffered network data.
-  bytes_compared_ += result;
+  bytes_compared_ += compare_offset_;
   state_ = STATE_DONE;
   return net::OK;
 }

content/browser/service_worker/service_worker_cache_writer.h

@@ -202,14 +202,25 @@ class CONTENT_EXPORT ServiceWorkerCacheWriter {
 
   size_t cached_length_;
 
+  // The amount of data from the network (|data_to_write_|) which has already
+  // been compared with data from storage (|data_to_read_|). This is
+  // initialized to 0 for every new arrival of network data.
+  size_t compare_offset_;
+
+  // Count of bytes which has been read from the network for comparison, and
+  // known as identical with the stored scripts. It is incremented only when a
+  // full block of network data is compared, to avoid having to use only
+  // fragments of the buffered network data.
   size_t bytes_compared_;
+
+  // Count of bytes copied from |copy_reader_| to |writer_|.
   size_t bytes_copied_;
+
+  // Count of bytes written back to |writer_|.
   size_t bytes_written_;
 
   bool did_replace_;
 
-  size_t compare_offset_;
-
   std::unique_ptr<ServiceWorkerResponseReader> compare_reader_;
   std::unique_ptr<ServiceWorkerResponseReader> copy_reader_;
   std::unique_ptr<ServiceWorkerResponseWriter> writer_;

content/browser/service_worker/service_worker_cache_writer_unittest.cc

@@ -832,5 +832,66 @@ TEST_F(ServiceWorkerCacheWriterTest, CompareFailedCopyLong) {
   EXPECT_TRUE(copy_reader->AllExpectedReadsDone());
 }
 
+// Tests behavior when the compare reader does not complete in single try and
+// needs to issue another read.
+TEST_F(ServiceWorkerCacheWriterTest, MultipleComparisonInSingleWrite) {
+  // Data for |compare_reader|.
+  const std::vector<std::string> data_from_cache{"a", "b", "c"};
+
+  // Data for |writer|. The first 2 bytes are provided in a larger chunk than
+  // the |compare_reader| does.
+  const std::vector<std::string> data_from_net{"ab", "x"};
+
+  // Data for |copy_reader|. The comparison between cache and network data fails
+  // at the 3rd byte, so the cache writer will read only first 2 bytes from the
+  // |copy_reader|.
+  const std::vector<std::string> data_to_copy{"ab"};
+
+  // The written data is expected to be identical with |data_from_net|.
+  const std::vector<std::string> data_expected{"ab", "x"};
+
+  size_t bytes_cached = 0;
+  size_t bytes_from_net = 0;
+  size_t bytes_common = 0;
+
+  for (const auto& data : data_from_cache)
+    bytes_cached += data.size();
+
+  for (const auto& data : data_from_net)
+    bytes_from_net += data.size();
+
+  for (const auto& data : data_to_copy)
+    bytes_common += 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);
+
+  copy_reader->ExpectReadInfoOk(bytes_common, false);
+  for (const auto& data : data_to_copy)
+    copy_reader->ExpectReadDataOk(data, false);
+
+  writer->ExpectWriteInfoOk(bytes_from_net, false);
+  for (const auto& data : data_expected)
+    writer->ExpectWriteDataOk(data.size(), false);
+
+  Initialize();
+
+  net::Error error = WriteHeaders(bytes_from_net);
+  EXPECT_EQ(net::OK, error);
+  for (const auto& data : data_from_net) {
+    error = WriteData(data);
+    EXPECT_EQ(net::OK, error);
+  }
+
+  EXPECT_TRUE(writer->AllExpectedWritesDone());
+  EXPECT_TRUE(compare_reader->AllExpectedReadsDone());
+  EXPECT_TRUE(copy_reader->AllExpectedReadsDone());
+}
+
 }  // namespace
 }  // namespace content