Add a helper method to truncate SourceStream's length

content/browser/download/download_file_impl.cc

Index: content/browser/download/download_file_impl.cc
diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc
index 4b47ae1f08e8bf0263a9e43c7fe8b8ec53113cae..630352e1b2ed6f5d6cbc2eabb1aa2c5808560b02 100644
--- a/content/browser/download/download_file_impl.cc
+++ b/content/browser/download/download_file_impl.cc
@@ -31,6 +31,8 @@
 
 namespace content {
 
+namespace {
+
 const int kUpdatePeriodMs = 500;
 const int kMaxTimeBlockingFileThreadMs = 1000;
 
@@ -42,6 +44,12 @@ const int kInitialRenameRetryDelayMs = 200;
 // Number of times a failing rename is retried before giving up.
 const int kMaxRenameRetries = 3;
 
+// Because DownloadSaveInfo::kLengthFullContent is 0, we should avoid using
+// 0 for length if we found that a stream can no longer write any data.
+const int kNoBytesToWrite = -1;
+
+}  // namespace
+
 DownloadFileImpl::SourceStream::SourceStream(
     int64_t offset,
     int64_t length,
@@ -59,6 +67,26 @@ void DownloadFileImpl::SourceStream::OnWriteBytesToDisk(int64_t bytes_write) {
   bytes_written_ += bytes_write;
 }
 
+void DownloadFileImpl::SourceStream::TruncateLengthWithWrittenDataBlock(
+    int64_t offset,
+    int64_t bytes_written) {
+  DCHECK_GT(bytes_written, 0);
+  if (offset < offset_) {

[xingliu, 2017/03/24 19:13:47]

nit%: Should we also do it when offset == offset_ ?

[qinmin, 2017/03/24 20:39:01]

Done. == is originally captured in the next if block, moved it here.
Results are the same, but we can remove the 
if (length_ == 0) block below

+    if (offset + bytes_written > offset_)

[xingliu, 2017/03/24 19:13:47]

nit%: what about if (..&& ..)

[qinmin, 2017/03/24 20:39:01]

No, we want to early return here if offset < offset_.
Otherwise, it will go into the next if block since (length > offset- offset_) is
always true

[xingliu, 2017/03/24 21:14:08]

My bad, I see.

+      length_ = kNoBytesToWrite;
+    return;
+  }
+
+  if (length_ == DownloadSaveInfo::kLengthFullContent ||
+      length_ > offset - offset_) {
+    length_ = offset - offset;

[xingliu, 2017/03/24 19:13:47]

should it be (offset_ - offset) ?

Also if length_ is already kNoBytesToWrite, is it possible to flush it with a
new valid value, since this function calls on every ReceivedBytes.

[qinmin, 2017/03/24 20:39:01]

No, (offset_-offset) is negative in in this if block.  If length_ is
kNoBytesToWrite, it will not satisfy any of the conditions here, so it shouldn't
change.

Added an early return condition at the top so we don't proceed if length_is
already kNoBytesToWrite.

[xingliu, 2017/03/24 21:14:08]

offset - offset is always 0, I think we are trying to truncate here?

[qinmin, 2017/03/24 21:38:31]

should be offset-offset_, fixed.

+    // Because DownloadSaveInfo::kLengthFullContent is 0, we should avoid using
+    // 0 here.
+    if (length_ == 0)
+      length_ = kNoBytesToWrite;
+  }
+}
+
 DownloadFileImpl::DownloadFileImpl(
     std::unique_ptr<DownloadSaveInfo> save_info,
     const base::FilePath& default_download_directory,
@@ -161,6 +189,11 @@ DownloadInterruptReason DownloadFileImpl::WriteDataToFile(int64_t offset,
 bool DownloadFileImpl::CalculateBytesToWrite(SourceStream* source_stream,
                                              size_t bytes_available_to_write,
                                              size_t* bytes_to_write) {
+  if (source_stream->length() == kNoBytesToWrite) {
+    *bytes_to_write = 0;
+    return true;
+  }
+
   // If a new slice finds that its target position has already been written,
   // terminate the stream.
   if (source_stream->bytes_written() == 0) {
@@ -448,6 +481,11 @@ void DownloadFileImpl::RegisterAndActivateStream(SourceStream* source_stream) {
     stream_reader->RegisterCallback(base::Bind(&DownloadFileImpl::StreamActive,
                                                weak_factory_.GetWeakPtr(),
                                                source_stream));
+    // Truncate |source_stream|'s length if necessary.
+    for (const auto& received_slice : received_slices_) {
+      source_stream->TruncateLengthWithWrittenDataBlock(
+          received_slice.offset, received_slice.received_bytes);
+    }
     StreamActive(source_stream);
     num_active_streams_++;
   }
@@ -503,12 +541,8 @@ void DownloadFileImpl::AddNewSlice(int64_t offset, int64_t length) {
         source_stream->set_index(source_stream->index() + 1);
     } else if (source_stream->offset() == offset) {
       source_stream->set_index(index);
-    } else if (source_stream->length() ==
-                   DownloadSaveInfo::kLengthFullContent ||
-               source_stream->length() > offset - source_stream->offset()) {
-      // The newly introduced slice will impact the length of the SourceStreams
-      // preceding it.
-      source_stream->set_length(offset - source_stream->offset());
+    } else {
+      source_stream->TruncateLengthWithWrittenDataBlock(offset, length);
     }
   }
 }
@@ -564,7 +598,7 @@ void DownloadFileImpl::HandleStreamError(SourceStream* source_stream,
 
   bool can_recover_from_error = false;
 
-  if (is_sparse_file_) {
+  if (is_sparse_file_ && source_stream->length() != kNoBytesToWrite) {
     // If a neighboring stream request is available, check if it can help
     // download all the data left by |source stream| or has already done so. We
     // want to avoid the situation that a server always fail additional requests