Cancel parallel download request if server doesn't send partial response

If the server doesn't support partial responses, Chrome should cancel
parallel download requests.
These requests result in the wrong write offset and makes the file larger
than the original

BUG=838627

Change-Id: I1fa878854579a1572e6e2f90f3bb932b59b333ba
Reviewed-on: https://chromium-review.googlesource.com/1048130
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557252}

components/download/internal/common/download_file_impl.cc

@@ -229,7 +229,7 @@ void DownloadFileImpl::AddInputStream(std::unique_ptr<InputStream> stream,
     CancelRequest(offset);
     return;
   }
-
+  DCHECK(source_streams_.find(offset) == source_streams_.end());
   source_streams_[offset] =
       std::make_unique<SourceStream>(offset, length, std::move(stream));
   OnSourceStreamAdded(source_streams_[offset].get());

components/download/internal/common/download_file_unittest.cc

@@ -1042,8 +1042,8 @@ TEST_F(DownloadFileTest, SecondStreamStartingOffsetAlreadyWritten) {
       .RetiresOnSaturation();
 
   download_file_->AddInputStream(
-      std::unique_ptr<MockInputStream>(additional_streams_[0]), 0,
-      DownloadSaveInfo::kLengthFullContent);
+      std::unique_ptr<MockInputStream>(additional_streams_[0]),
+      strlen(kTestData1), DownloadSaveInfo::kLengthFullContent);
 
   // The stream should get terminated and reset the callback.
   EXPECT_TRUE(sink_callback_.is_null());

components/download/internal/common/download_item_impl.cc

@@ -1479,9 +1479,8 @@ void DownloadItemImpl::Start(
   if (state_ == RESUMING_INTERNAL)
     UpdateValidatorsOnResumption(new_create_info);
 
-  // If the download is not parallel download during resumption, clear the
-  // |received_slices_|.
-  if (!job_->IsParallelizable() && !received_slices_.empty()) {
+  // If the download is not parallel, clear the |received_slices_|.
+  if (!received_slices_.empty() && !job_->IsParallelizable()) {
     destination_info_.received_bytes =
         GetMaxContiguousDataBlockSizeFromBeginning(received_slices_);
     received_slices_.clear();

components/download/internal/common/download_job_factory.cc

@@ -44,10 +44,12 @@ bool IsParallelizableDownload(const DownloadCreateInfo& create_info,
                                  net::HttpResponseInfo::CONNECTION_INFO_HTTP1_1;
   bool http_get_method =
       create_info.method == "GET" && create_info.url().SchemeIsHTTPOrHTTPS();
-
+  bool partial_response_success =
+      download_item->GetReceivedSlices().empty() || create_info.offset != 0;
   bool is_parallelizable = has_strong_validator && create_info.accept_range &&
                            has_content_length && satisfy_min_file_size &&
-                           satisfy_connection_type && http_get_method;
+                           satisfy_connection_type && http_get_method &&
+                           partial_response_success;
 
   if (!IsParallelDownloadEnabled())
     return is_parallelizable;

components/download/internal/common/download_worker.cc

@@ -137,7 +137,8 @@ void DownloadWorker::OnUrlDownloadStarted(
     Pause();
   }
 
-  delegate_->OnInputStreamReady(this, std::move(input_stream));
+  delegate_->OnInputStreamReady(this, std::move(input_stream),
+                                std::move(create_info));
 }
 
 void DownloadWorker::OnUrlDownloadStopped(UrlDownloadHandler* downloader) {

components/download/internal/common/download_worker.h

@@ -31,7 +31,8 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadWorker
     // destination file.
     virtual void OnInputStreamReady(
         DownloadWorker* worker,
-        std::unique_ptr<InputStream> input_stream) = 0;
+        std::unique_ptr<InputStream> input_stream,
+        std::unique_ptr<DownloadCreateInfo> download_create_info) = 0;
   };
 
   DownloadWorker(DownloadWorker::Delegate* delegate,

components/download/internal/common/parallel_download_job.cc

@@ -123,9 +123,14 @@ void ParallelDownloadJob::BuildParallelRequestAfterDelay() {
 
 void ParallelDownloadJob::OnInputStreamReady(
     DownloadWorker* worker,
-    std::unique_ptr<InputStream> input_stream) {
-  bool success = DownloadJob::AddInputStream(
-      std::move(input_stream), worker->offset(), worker->length());
+    std::unique_ptr<InputStream> input_stream,
+    std::unique_ptr<DownloadCreateInfo> download_create_info) {
+  // If server returns a wrong range, abort the parallel request.
+  bool success = download_create_info->offset == worker->offset();
+  if (success) {
+    success = DownloadJob::AddInputStream(std::move(input_stream),
+                                          worker->offset(), worker->length());
+  }
   RecordParallelDownloadAddStreamSuccess(success);
 
   // Destroy the request if the sink is gone.

components/download/internal/common/parallel_download_job.h

@@ -68,8 +68,10 @@ class COMPONENTS_DOWNLOAD_EXPORT ParallelDownloadJob
   friend class ParallelDownloadJobTest;
 
   // DownloadWorker::Delegate implementation.
-  void OnInputStreamReady(DownloadWorker* worker,
-                          std::unique_ptr<InputStream> input_stream) override;
+  void OnInputStreamReady(
+      DownloadWorker* worker,
+      std::unique_ptr<InputStream> input_stream,
+      std::unique_ptr<DownloadCreateInfo> download_create_info) override;
 
   // Build parallel requests after a delay, to effectively measure the single
   // stream bandwidth.

components/download/internal/common/parallel_download_job_unittest.cc

@@ -94,8 +94,10 @@ class ParallelDownloadJobForTest : public ParallelDownloadJob {
     return min_remaining_time_;
   }
 
-  void OnInputStreamReady(DownloadWorker* worker,
-                          std::unique_ptr<InputStream> input_stream) override {
+  void OnInputStreamReady(
+      DownloadWorker* worker,
+      std::unique_ptr<InputStream> input_stream,
+      std::unique_ptr<DownloadCreateInfo> download_create_info) override {
     CountOnInputStreamReady();
   }
 

content/browser/download/download_browsertest.cc

@@ -974,7 +974,8 @@ class ParallelDownloadTest : public DownloadContentTest {
   void RunResumptionTest(
       const download::DownloadItem::ReceivedSlices& received_slices,
       int64_t total_length,
-      size_t expected_request_count) {
+      size_t expected_request_count,
+      bool support_partial_response) {
     EXPECT_TRUE(
         base::FeatureList::IsEnabled(download::features::kParallelDownloading));
     GURL url = TestDownloadHttpResponse::GetNextURLForDownload();
@@ -983,6 +984,7 @@ class ParallelDownloadTest : public DownloadContentTest {
     parameters.etag = "ABC";
     parameters.size = total_length;
     parameters.last_modified = std::string();
+    parameters.support_partial_response = support_partial_response;
     // Needed to specify HTTP connection type to create parallel download.
     parameters.connection_type = net::HttpResponseInfo::CONNECTION_INFO_HTTP1_1;
     TestDownloadHttpResponse::StartServing(parameters, server_url);
@@ -999,12 +1001,17 @@ class ParallelDownloadTest : public DownloadContentTest {
     // Resume the parallel download with sparse file and received slices data.
     download->Resume();
     WaitForCompletion(download);
+    // TODO(qinmin): count the failed partial responses in DownloadJob when
+    // support_partial_response is false. EmbeddedTestServer doesn't know
+    // whether completing or canceling the response will come first.
+    if (support_partial_response) {
       test_response_handler()->WaitUntilCompletion(expected_request_count);
 
       // Verify number of requests sent to the server.
       const TestDownloadResponseHandler::CompletedRequests& completed_requests =
           test_response_handler()->completed_requests();
       EXPECT_EQ(expected_request_count, completed_requests.size());
+    }
 
     // Verify download content on disk.
     ReadAndVerifyFileContents(parameters.pattern_generator_seed,
@@ -1026,22 +1033,29 @@ class ParallelDownloadTest : public DownloadContentTest {
     parameters.connection_type = net::HttpResponseInfo::CONNECTION_INFO_HTTP1_1;
     TestRequestPauseHandler request_pause_handler;
     parameters.on_pause_handler = request_pause_handler.GetOnPauseHandler();
+    // Send some data for the first request and pause it so download won't
+    // complete before other parallel requests are created.
     parameters.pause_offset = DownloadRequestCore::kDownloadByteStreamSize;
     TestDownloadHttpResponse::StartServing(parameters, server_url);
 
     download::DownloadItem* download =
         StartDownloadAndReturnItem(shell(), server_url);
-    // Send some data for the first request and pause it so download won't
-    // complete before other parallel requests are created.
+    // TODO(qinmin): wait for the failed partial responses in DownloadJob.
+    // Waiting for the completed test server requests is unsafe. This is because
+    // completing or canceling the response first all depends on the internal
+    // write buffer size.
+    if (parameters.support_partial_response)
       test_response_handler()->WaitUntilCompletion(2u);
 
     // Now resume the first request.
     request_pause_handler.Resume();
     WaitForCompletion(download);
+    if (parameters.support_partial_response) {
       test_response_handler()->WaitUntilCompletion(3u);
       const TestDownloadResponseHandler::CompletedRequests& completed_requests =
           test_response_handler()->completed_requests();
-    EXPECT_EQ(kTestRequestCount, static_cast<int>(completed_requests.size()));
+      EXPECT_EQ(3u, completed_requests.size());
+    }
     ReadAndVerifyFileContents(parameters.pattern_generator_seed,
                               parameters.size, download->GetTargetFilePath());
   }
@@ -3182,6 +3196,17 @@ IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, OnlyFirstRequestValid) {
   RunCompletionTest(parameters);
 }
 
+// The server will send Accept-Ranges header without partial response.
+IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, NoPartialResponse) {
+  TestDownloadHttpResponse::Parameters parameters;
+  parameters.etag = "ABC";
+  parameters.size = 5097152;
+  parameters.support_byte_ranges = true;
+  parameters.support_partial_response = false;
+
+  RunCompletionTest(parameters);
+}
+
 // Verify parallel download resumption.
 IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, Resumption) {
   // Create the received slices data, the last request is not finished and the
@@ -3192,7 +3217,8 @@ IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, Resumption) {
       download::DownloadItem::ReceivedSlice(2000000, 1000,
                                             false /* finished */)};
 
-  RunResumptionTest(received_slices, 3000000, kTestRequestCount);
+  RunResumptionTest(received_slices, 3000000, kTestRequestCount,
+                    true /* support_partial_response */);
 }
 
 // Verifies that if the last slice is finished, parallel download resumption
@@ -3207,7 +3233,8 @@ IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, ResumptionLastSliceFinished) {
 
   // The server shouldn't receive an additional request, since the last slice
   // is marked as finished.
-  RunResumptionTest(received_slices, 3000000, kTestRequestCount - 1);
+  RunResumptionTest(received_slices, 3000000, kTestRequestCount - 1,
+                    true /* support_partial_response */);
 }
 
 // Verifies that if the last slice is finished, but the database record is not
@@ -3223,7 +3250,23 @@ IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, ResumptionLastSliceUnfinished) {
 
   // Client will send an out of range request where server will send back HTTP
   // range not satisfied, and download can complete.
-  RunResumptionTest(received_slices, 3000000, kTestRequestCount);
+  RunResumptionTest(received_slices, 3000000, kTestRequestCount,
+                    true /* support_partial_response */);
+}
+
+// Verify that if server doesn't support partial response, resuming a parallel
+// download should complete the download.
+IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, ResumptionNoPartialResponse) {
+  // Create the received slices data, the last request is not finished and the
+  // server will send more data to finish the last slice.
+  std::vector<download::DownloadItem::ReceivedSlice> received_slices = {
+      download::DownloadItem::ReceivedSlice(0, 1000),
+      download::DownloadItem::ReceivedSlice(1000000, 1000),
+      download::DownloadItem::ReceivedSlice(2000000, 1000,
+                                            false /* finished */)};
+
+  RunResumptionTest(received_slices, 3000000, kTestRequestCount,
+                    false /* support_partial_response */);
 }
 
 // Test to verify that the browser-side enforcement of X-Frame-Options does

content/public/test/test_download_http_response.cc

@@ -121,6 +121,7 @@ TestDownloadHttpResponse::Parameters::Parameters()
       size(102400),
       pattern_generator_seed(1),
       support_byte_ranges(true),
+      support_partial_response(true),
       connection_type(
           net::HttpResponseInfo::ConnectionInfo::CONNECTION_INFO_UNKNOWN) {}
 
@@ -136,6 +137,7 @@ TestDownloadHttpResponse::Parameters::Parameters(Parameters&& that)
       size(that.size),
       pattern_generator_seed(that.pattern_generator_seed),
       support_byte_ranges(that.support_byte_ranges),
+      support_partial_response(that.support_partial_response),
       connection_type(that.connection_type),
       static_response(std::move(that.static_response)),
       injected_errors(std::move(that.injected_errors)),
@@ -146,11 +148,12 @@ TestDownloadHttpResponse::Parameters::Parameters(Parameters&& that)
 TestDownloadHttpResponse::Parameters& TestDownloadHttpResponse::Parameters::
 operator=(Parameters&& that) {
   etag = std::move(that.etag);
-  last_modified = std::move(that.etag);
+  last_modified = std::move(that.last_modified);
   content_type = std::move(that.content_type);
   size = that.size;
   pattern_generator_seed = that.pattern_generator_seed;
   support_byte_ranges = that.support_byte_ranges;
+  support_partial_response = that.support_partial_response;
   static_response = std::move(that.static_response);
   injected_errors = std::move(that.injected_errors);
   inject_error_cb = that.inject_error_cb;
@@ -297,6 +300,7 @@ void TestDownloadHttpResponse::ParseRequestHeader() {
   // Adjust the response range according to request range. The first byte offset
   // of the request may be larger than entity body size.
   request_range_ = ranges[0];
+  if (parameters_.support_partial_response)
     range_.set_first_byte_position(request_range_.first_byte_position());
   range_.ComputeBounds(parameters_.size);
 
@@ -325,7 +329,7 @@ void TestDownloadHttpResponse::SendResponseHeaders() {
 std::string TestDownloadHttpResponse::GetDefaultResponseHeaders() {
   std::string headers;
   // Send partial response.
-  if (parameters_.support_byte_ranges &&
+  if (parameters_.support_partial_response && parameters_.support_byte_ranges &&
       request_.headers.find(net::HttpRequestHeaders::kIfRange) !=
           request_.headers.end() &&
       request_.headers.at(net::HttpRequestHeaders::kIfRange) ==
@@ -335,7 +339,7 @@ std::string TestDownloadHttpResponse::GetDefaultResponseHeaders() {
   }
 
   // Send precondition failed for "If-Match" request header.
-  if (parameters_.support_byte_ranges &&
+  if (parameters_.support_partial_response && parameters_.support_byte_ranges &&
       request_.headers.find(net::HttpRequestHeaders::kIfMatch) !=
           request_.headers.end()) {
     if (request_.headers.at(net::HttpRequestHeaders::kIfMatch) !=

content/public/test/test_download_http_response.h

@@ -108,6 +108,12 @@ class TestDownloadHttpResponse {
     // response, or contains 'Content-Range' header for HTTP 206 response.
     bool support_byte_ranges;
 
+    // Whether the server supports partial range responses. A server can claim
+    // it support byte ranges, but actually doesn't send partial responses. In
+    // that case, Set |support_byte_ranges| to true and this variable to false
+    // to simulate the case.
+    bool support_partial_response;
+
     // The connection type in the response.
     net::HttpResponseInfo::ConnectionInfo connection_type;