Verify that all slice are downloaded when a stream complete

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 8b2a9e8e81585fb36be77bd083558ba356fc8ffa..836dcaf3326689a120ef5fa34fe63f917109e791 100644
--- a/content/browser/download/download_file_impl.cc
+++ b/content/browser/download/download_file_impl.cc
@@ -19,6 +19,7 @@
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_net_log_parameters.h"
 #include "content/browser/download/download_stats.h"
+#include "content/browser/download/parallel_download_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/secure_hash.h"
 #include "crypto/sha2.h"
@@ -367,18 +368,8 @@ void DownloadFileImpl::StreamActive(SourceStream* source_stream) {
     // Inform observers.
     SendUpdate();
 
-    // TODO(xingliu): Use slice info to determine if the file is fully
-    // downloaded.
-    bool all_stream_complete = true;
-    for (auto& stream : source_streams_) {
-      if (!stream.second->is_finished()) {
-        all_stream_complete = false;
-        break;
-      }
-    }
-
     // All the stream reader are completed, shut down file IO processing.
-    if (all_stream_complete) {
+    if (IsDownloadCompleted()) {
       RecordFileBandwidth(bytes_seen_, disk_writes_time_,
                           base::TimeTicks::Now() - download_start_);
       weak_factory_.InvalidateWeakPtrs();
@@ -433,6 +424,47 @@ void DownloadFileImpl::WillWriteToDisk(size_t data_len) {
   rate_estimator_.Increment(data_len);
 }
 
+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 &&
+        source_stream->bytes_written() > 0) {
+      stream_for_last_slice = source_stream;
+      last_slice_offset = source_stream->offset();
+    }
+    if (!source_stream->is_finished())
+      return false;
+  }
+
+  if (!is_sparse_file_)
+    return true;
+
+  // Verify that all the file slices have been downloaded.
+  std::vector<DownloadItem::ReceivedSlice> slices_to_download =

[xingliu, 2017/03/09 21:24:23]

So if we do this, source_streams_ don't need to be fully setup in the ctor, but
can add/remove element when AddByteStream is called? This is probably better.

[qinmin, 2017/03/09 21:58:42]

The only element in the source_streams_ that needs to be setup is the initial
request. All other SourceStreams can be constructed later when needed.

+      FindSlicesToDownload(received_slices_);
+  if (slices_to_download.size() > 1) {
+    // If there are 1 or more holes in the file, download is not finished.
+    // Some streams might not have been added to |source_streams_| yet.
+    return false;
+  } else if (stream_for_last_slice) {

[xingliu, 2017/03/10 00:46:22]

nit%: use if instead of else if, since previous block has returned.

Also this block check if the last source stream is half opened. This probably
based on an assumption that we will always send a half opened request even the
last slice is finished, right?

This involves some risk since even if the last slice is done, we send a  half
opened request which doesn't download anything.

Also, maybe we should consider to use content-length to check if the final slice
is done, maybe better than send a request for whatever reason, wdyt?

[qinmin, 2017/03/10 07:11:27]

Done.

The issue is that using the content-length check is not safe. And if the
downloaded content is already longer than content-length, we will still face the
same problem.  The safest approach is to add a boolean variable into the history
db, or we do a special request like this.

[xingliu, 2017/03/10 18:17:03]

Yeah, make sense.

For special request, I doubt some server may give different response code.

+    DCHECK_EQ(slices_to_download[0].received_bytes,
+              DownloadSaveInfo::kLengthFullContent);
+    // The last stream should not have a length limit. If it has, it might

[xingliu, 2017/03/10 00:46:22]

nit%: The first stream also doesn't have length limit, do we want the first
stream to be able to cover other slices?

If so stream_for_last_slice->length() may not be 0 when first stream is done,
but the function will return false.

[qinmin, 2017/03/10 07:11:27]

I don't think the situation you mentioned will happen.
Since stream_for_last_slice's bytes_written() > 0, it means that the stream was
able to write a chunk of data to the file. Any stream with offset smaller than
stream_for_last_slice should have stopped short. Or otherwise the same chunk was
written by more than 2 workers and violates the received_slices integrity check.

If the first stream is able to reach the end, it means no other stream has
written any data to the file.  Or otherwise it will be stopped short by another
stream. As a result, stream_for_last_slice should be the first stream

+    // not reach the end of the file.
+    if (stream_for_last_slice->length() !=
+        DownloadSaveInfo::kLengthFullContent) {
+      return false;
+    }
+    DCHECK_EQ(slices_to_download[0].offset,
+              stream_for_last_slice->offset() +
+                  stream_for_last_slice->bytes_written());
+  }
+
+  return true;
+}
+
 DownloadFileImpl::RenameParameters::RenameParameters(
     RenameOption option,
     const base::FilePath& new_path,