Add a helper method to truncate SourceStream's length

content/browser/download/download_file_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_file_impl.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 13 matching lines @@
 #include "crypto/secure_hash.h"
 #include "crypto/sha2.h"
 #include "net/base/io_buffer.h"
 #include "net/log/net_log.h"
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_source.h"
 #include "net/log/net_log_source_type.h"
 
 namespace content {
 
+namespace {
+
 const int kUpdatePeriodMs = 500;
 const int kMaxTimeBlockingFileThreadMs = 1000;
 
 // These constants control the default retry behavior for failing renames. Each
 // retry is performed after a delay that is twice the previous delay. The
 // initial delay is specified by kInitialRenameRetryDelayMs.
 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,
     std::unique_ptr<ByteStreamReader> stream_reader)
     : offset_(offset),
       length_(length),
       bytes_written_(0),
       finished_(false),
       index_(0u),
       stream_reader_(std::move(stream_reader)) {}
 
 DownloadFileImpl::SourceStream::~SourceStream() = default;
 
 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,
     std::unique_ptr<ByteStreamReader> stream_reader,
     const std::vector<DownloadItem::ReceivedSlice>& received_slices,
     const net::NetLogWithSource& download_item_net_log,
     bool is_sparse_file,
     base::WeakPtr<DownloadDestinationObserver> observer)
     : net_log_(
           net::NetLogWithSource::Make(download_item_net_log.net_log(),
@@ skipping 82 matching lines @@
                                                           const char* data,
                                                           size_t data_len) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   WillWriteToDisk(data_len);
   return file_.WriteDataToFile(offset, data, data_len);
 }
 
 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) {
     for (const auto& received_slice : received_slices_) {
       if (received_slice.offset <= source_stream->offset() &&
           received_slice.offset + received_slice.received_bytes >
               source_stream->offset()) {
         *bytes_to_write = 0;
         return true;
       }
@@ skipping 267 matching lines @@
   }
 }
 
 void DownloadFileImpl::RegisterAndActivateStream(SourceStream* source_stream) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   ByteStreamReader* stream_reader = source_stream->stream_reader();
   if (stream_reader) {
     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_++;
   }
 }
 
 int64_t DownloadFileImpl::TotalBytesReceived() const {
   return file_.bytes_so_far();
 }
 
 void DownloadFileImpl::SendUpdate() {
@@ skipping 35 matching lines @@
   // Check if the slice is added as a new slice, or merged with an existing one.
   bool slice_added = (offset == received_slices_[index].offset);
   // Update the index of exising SourceStreams.
   for (auto& stream : source_streams_) {
     SourceStream* source_stream = stream.second.get();
     if (source_stream->offset() > offset) {
       if (slice_added && source_stream->bytes_written() > 0)
         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);
     }
   }
 }
 
 bool DownloadFileImpl::IsDownloadCompleted() {
   SourceStream* stream_for_last_slice = nullptr;
   int64_t last_slice_offset = 0;
   for (auto& stream : source_streams_) {
     SourceStream* source_stream = stream.second.get();
     if (source_stream->offset() >= last_slice_offset &&
@@ skipping 35 matching lines @@
 
 void DownloadFileImpl::HandleStreamError(SourceStream* source_stream,
                                          DownloadInterruptReason reason) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   source_stream->stream_reader()->RegisterCallback(base::Closure());
   source_stream->set_finished(true);
   num_active_streams_--;
 
   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
     // from the client thus causing the initial request and the download going
     // nowhere.
     // TODO(qinmin): make all streams half open so that they can recover
     // failures from their neighbors.
     SourceStream* preceding_neighbor = FindPrecedingNeighbor(source_stream);
     while (preceding_neighbor) {
       int64_t upper_range = source_stream->offset() + source_stream->length();
@@ skipping 64 matching lines @@
     const base::FilePath& new_path,
     const RenameCompletionCallback& completion_callback)
     : option(option),
       new_path(new_path),
       retries_left(kMaxRenameRetries),
       completion_callback(completion_callback) {}
 
 DownloadFileImpl::RenameParameters::~RenameParameters() {}
 
 }  // namespace content