Allow each download stream to take some data for validation purpose

Currently for each download stream, the data read from the network
was immediately written to the disk.
This CL allows each SourceStream to read some data before the file
write offset for content validation purpose. And data will only be
written after all the initial validation data are consumed.
A new field called starting_file_write_offset_ is added to SourceStream
to identify this checkpoint for file validation.

BUG=965215

Change-Id: Ia46de1a588bc0c2fb4e6eb19e62ef794d706ad1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1625461
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664030}

components/download/internal/common/parallel_download_utils.cc

@@ -102,7 +102,7 @@ bool CanRecoverFromError(
     // the error stream.
     if (error_stream->length() > 0) {
       return error_stream->offset() + error_stream->length() <=
-             preceding_neighbor->offset() + preceding_neighbor->bytes_written();
+             preceding_neighbor->offset() + preceding_neighbor->bytes_read();
     }
 
     return false;

components/download/internal/common/parallel_download_utils_unittest.cc

@@ -43,7 +43,8 @@ class ParallelDownloadUtilsRecoverErrorTest
     EXPECT_CALL(*input_stream_, GetCompletionStatus())
         .WillRepeatedly(Return(DOWNLOAD_INTERRUPT_REASON_NONE));
     return std::make_unique<DownloadFileImpl::SourceStream>(
-        offset, length, std::unique_ptr<MockInputStream>(input_stream_));
+        offset, length, offset,
+        std::unique_ptr<MockInputStream>(input_stream_));
   }
 
  protected:
@@ -166,7 +167,7 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Even if it has written some data.
-  preceding_stream->OnWriteBytesToDisk(1000u);
+  preceding_stream->OnBytesConsumed(1000u, 1000u);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Now capped the length of preceding stream with different values.
@@ -177,14 +178,15 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
   preceding_stream->set_finished(false);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
   preceding_stream->set_finished(true);
-  preceding_stream->OnWriteBytesToDisk(kErrorStreamOffset - preceding_offset);
+  int64_t bytes_consumed = kErrorStreamOffset - preceding_offset;
+  preceding_stream->OnBytesConsumed(bytes_consumed, bytes_consumed);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Inject an error results in failure, even if data written exceeds the first
   // byte of error stream.
   EXPECT_CALL(*input_stream_, GetCompletionStatus())
       .WillRepeatedly(Return(DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE));
-  preceding_stream->OnWriteBytesToDisk(1000u);
+  preceding_stream->OnBytesConsumed(1000u, 1000u);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Make preceding stream can reach the first byte of error stream.
@@ -194,9 +196,9 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
   preceding_stream->set_finished(false);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
   preceding_stream->set_finished(true);
-  preceding_stream->OnWriteBytesToDisk(kErrorStreamOffset - preceding_offset);
+  preceding_stream->OnBytesConsumed(bytes_consumed, bytes_consumed);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
-  preceding_stream->OnWriteBytesToDisk(1);
+  preceding_stream->OnBytesConsumed(1, 1);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Preceding stream that never download data won't recover the error stream.
@@ -229,11 +231,13 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
   // Since the preceding stream can never reach the starting offset, for an
   // unfinished stream, we rely on length instead of bytes written.
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
-  preceding_stream->OnWriteBytesToDisk(kErrorStreamOffset - preceding_offset);
+  int64_t bytes_consumed = kErrorStreamOffset - preceding_offset;
+  preceding_stream->OnBytesConsumed(bytes_consumed, bytes_consumed);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
-  preceding_stream->OnWriteBytesToDisk(kErrorStreamLength - 1);
+  preceding_stream->OnBytesConsumed(kErrorStreamLength - 1,
+                                    kErrorStreamLength - 1);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
-  preceding_stream->OnWriteBytesToDisk(1);
+  preceding_stream->OnBytesConsumed(1, 1);
 
   // Create preceding stream that can reach the upper bound of error stream.
   // Since it's unfinished, it potentially can take over error stream's work
@@ -248,11 +252,12 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
   // Finished preceding stream only checks data written.
   preceding_stream = CreateSourceStream(preceding_offset, 1);
   preceding_stream->set_finished(true);
-  preceding_stream->OnWriteBytesToDisk(kErrorStreamOffset - preceding_offset);
+  preceding_stream->OnBytesConsumed(bytes_consumed, bytes_consumed);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
-  preceding_stream->OnWriteBytesToDisk(kErrorStreamLength - 1);
+  preceding_stream->OnBytesConsumed(kErrorStreamLength - 1,
+                                    kErrorStreamLength - 1);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
-  preceding_stream->OnWriteBytesToDisk(1);
+  preceding_stream->OnBytesConsumed(1, 1);
   EXPECT_TRUE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Even if inject an error, since data written has cover the upper bound of

components/download/public/common/download_file_impl.h

@@ -99,17 +99,19 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadFileImpl : public DownloadFile {
    public:
     SourceStream(int64_t offset,
                  int64_t length,
+                 int64_t starting_file_write_offset,
                  std::unique_ptr<InputStream> stream);
     ~SourceStream();
 
     void Initialize();
 
-    // Called after successfully writing a buffer to disk.
-    void OnWriteBytesToDisk(int64_t bytes_write);
+    // Called after successfully reading and writing a buffer from stream.
+    void OnBytesConsumed(int64_t bytes_read, int64_t bytes_written);
 
     // Given a data block that is already written, truncate the length of this
-    // object to avoid overwriting that block.
-    void TruncateLengthWithWrittenDataBlock(int64_t offset,
+    // object to avoid overwriting that block. Data used for validation purpose
+    // will not be truncated.
+    void TruncateLengthWithWrittenDataBlock(int64_t received_slice_offset,
                                             int64_t bytes_written);
 
     // Registers the callback that will be called when data is ready.
@@ -136,8 +138,15 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadFileImpl : public DownloadFile {
     InputStream::StreamState Read(scoped_refptr<net::IOBuffer>* data,
                                   size_t* length);
 
+    // Returning the remaining bytes to validate.
+    size_t GetRemainingBytesToValidate();
+
     int64_t offset() const { return offset_; }
     int64_t length() const { return length_; }
+    int64_t starting_file_write_offset() const {
+      return starting_file_write_offset_;
+    }
+    int64_t bytes_read() const { return bytes_read_; }
     int64_t bytes_written() const { return bytes_written_; }
     bool is_finished() const { return finished_; }
     void set_finished(bool finish) { finished_ = finish; }
@@ -145,15 +154,24 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadFileImpl : public DownloadFile {
     void set_index(size_t index) { index_ = index; }
 
    private:
-    // Starting position for the stream to write to disk.
+    // Starting position of the stream, this is from the network response.
     int64_t offset_;
 
     // The maximum length to write to the disk. If set to 0, keep writing until
     // the stream depletes.
     int64_t length_;
 
-    // Number of bytes written to disk from the stream.
-    // Next write position is (|offset_| + |bytes_written_|).
+    // All the data received before this offset are used for validation purpose
+    // and will not be written to disk. This value should always be no less than
+    // |offset_|.
+    int64_t starting_file_write_offset_;
+
+    // Number of bytes read from the stream.
+    // Next read position is (|offset_| + |bytes_read_|).
+    int64_t bytes_read_;
+
+    // Number of bytes written to the disk. This does not include the bytes used
+    // for validation.
     int64_t bytes_written_;
 
     // If all the data read from the stream has been successfully written to
@@ -172,11 +190,13 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadFileImpl : public DownloadFile {
 
  protected:
   // For test class overrides.
-  // Write data from the offset to the file.
-  // On OS level, it will seek to the |offset| and write from there.
-  virtual DownloadInterruptReason WriteDataToFile(int64_t offset,
-                                                  const char* data,
-                                                  size_t data_len);
+  // Validate the first |bytes_to_validate| bytes and write the next
+  // |bytes_to_write| bytes of data from the offset to the file.
+  virtual DownloadInterruptReason ValidateAndWriteDataToFile(
+      int64_t offset,
+      const char* data,
+      size_t bytes_to_validate,
+      size_t bytes_to_write);
 
   virtual base::TimeDelta GetRetryDelayForFailedRename(int attempt_number);
 
@@ -232,13 +252,14 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadFileImpl : public DownloadFile {
   void WillWriteToDisk(size_t data_len);
 
   // For a given SourceStream object and the bytes available to write, determine
-  // the actual number of bytes it can write to the disk. For parallel
-  // downloading, if the first disk IO writes to a location that is already
-  // written by another stream, the current stream should stop writing. Returns
-  // true if the stream can write no more data and should be finished, returns
-  // false otherwise.
+  // the number of bytes to validate and the number of bytes it can write to the
+  // disk. For parallel downloading, if the first disk IO writes to a location
+  // that is already written by another stream, the current stream should stop
+  // writing. Returns true if the stream can write no more data and should be
+  // finished, returns false otherwise.
   bool CalculateBytesToWrite(SourceStream* source_stream,
                              size_t bytes_available_to_write,
+                             size_t* bytes_to_validate,
                              size_t* bytes_to_write);
 
   // Called when a new SourceStream object is added.

components/download/public/common/download_save_info.cc

@@ -15,4 +15,8 @@ DownloadSaveInfo::~DownloadSaveInfo() = default;
 
 DownloadSaveInfo::DownloadSaveInfo(DownloadSaveInfo&& that) = default;
 
+int64_t DownloadSaveInfo::GetStartingFileWriteOffset() {
+  return file_offset >= 0 ? file_offset : offset;
+}
+
 }  // namespace download

components/download/public/common/download_save_info.h

@@ -30,6 +30,8 @@ struct COMPONENTS_DOWNLOAD_EXPORT DownloadSaveInfo {
   ~DownloadSaveInfo();
   DownloadSaveInfo(DownloadSaveInfo&& that);
 
+  int64_t GetStartingFileWriteOffset();
+
   // If non-empty, contains the full target path of the download that has been
   // determined prior to download initiation. This is considered to be a trusted
   // path.
@@ -42,9 +44,18 @@ struct COMPONENTS_DOWNLOAD_EXPORT DownloadSaveInfo {
   // If valid, contains the source data stream for the file contents.
   base::File file;
 
-  // The file offset at which to start the download.
+  // The offset sent to the server when requesting the download. During
+  // resumption, |offset| could be smaller than the downloaded content length.
+  // This is because download may request some data to validate whether the
+  // content has changed.
   int64_t offset = 0;
 
+  // The file offset to start writing to disk. If this value is negative,
+  // download stream will be writing to the disk starting at |offset|.
+  // Otherwise, this value will be used. Data received before |file_offset| are
+  // used for validation purpose.
+  int64_t file_offset = -1;
+
   // The number of the bytes to download from |offset|.
   // Ask to retrieve segment of the download file when length is greater than 0.
   // Request the rest of the file starting from |offset|, when length is

content/public/test/test_file_error_injector.cc

@@ -52,9 +52,11 @@ class DownloadFileWithError : public download::DownloadFileImpl {
                   bool is_parallelizable) override;
 
   // DownloadFile interface.
-  download::DownloadInterruptReason WriteDataToFile(int64_t offset,
-                                                    const char* data,
-                                                    size_t data_len) override;
+  download::DownloadInterruptReason ValidateAndWriteDataToFile(
+      int64_t offset,
+      const char* data,
+      size_t bytes_to_validate,
+      size_t bytes_to_write) override;
 
   download::DownloadInterruptReason HandleStreamCompletionStatus(
       SourceStream* source_stream) override;
@@ -174,13 +176,15 @@ void DownloadFileWithError::Initialize(
                                          received_slices, is_parallelizable);
 }
 
-download::DownloadInterruptReason DownloadFileWithError::WriteDataToFile(
-    int64_t offset,
-    const char* data,
-    size_t data_len) {
+download::DownloadInterruptReason
+DownloadFileWithError::ValidateAndWriteDataToFile(int64_t offset,
+                                                  const char* data,
+                                                  size_t bytes_to_validate,
+                                                  size_t bytes_to_write) {
   return ShouldReturnError(
       TestFileErrorInjector::FILE_OPERATION_WRITE,
-      download::DownloadFileImpl::WriteDataToFile(offset, data, data_len));
+      download::DownloadFileImpl::ValidateAndWriteDataToFile(
+          offset, data, bytes_to_validate, bytes_to_write));
 }
 
 download::DownloadInterruptReason