Reland "Reland "Cancel parallel download request if server doesn't se…

…nd partial response"" This reverts commit e127247. Reason for revert: <INSERT REASONING HERE> Original change's description: > Revert "Reland "Cancel parallel download request if server doesn't send partial response"" > > This reverts commit 05267e5. > > Reason for revert: <INSERT REASONING HERE> > > Original change's description: > > Reland "Cancel parallel download request if server doesn't send partial response" > > > > This reverts commit 1307743. > > > > Reason for revert: <INSERT REASONING HERE> > > > > Original change's description: > > > Revert "Cancel parallel download request if server doesn't send partial response" > > > > > > This reverts commit 189d103. > > > > > > Reason for revert: "ParallelDownloadTest.NoPartialResponse" is flaky. See https://crbug.com/841666. > > > > > > Original change's description: > > > > 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} > > > > > > TBR=qinmin@chromium.org,xingliu@chromium.org > > > > > > Change-Id: I995c3c48fe39c535275a353130b10204009c5e30 > > > No-Presubmit: true > > > No-Tree-Checks: true > > > No-Try: true > > > Bug: 838627, 841666 > > > Reviewed-on: https://chromium-review.googlesource.com/1053470 > > > Reviewed-by: Yutaka Hirano <yhirano@chromium.org> > > > Commit-Queue: Yutaka Hirano <yhirano@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#557489} > > > > TBR=yhirano@chromium.org,qinmin@chromium.org,xingliu@chromium.org > > > > Change-Id: I47a2d4131efa8b863d9868eeca74e99d7beeaa8e > > No-Presubmit: true > > No-Tree-Checks: true > > No-Try: true > > Bug: 838627, 841666 > > Reviewed-on: https://chromium-review.googlesource.com/1054887 > > Reviewed-by: Min Qin <qinmin@chromium.org> > > Commit-Queue: Min Qin <qinmin@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#557734} > > TBR=yhirano@chromium.org,qinmin@chromium.org,xingliu@chromium.org > > Change-Id: I7a618b9533229ea1bb97ff65470e9c60fe580a49 > No-Try: true > Bug: 838627, 841666 > Reviewed-on: https://chromium-review.googlesource.com/1053729 > Reviewed-by: Max Morin <maxmorin@chromium.org> > Commit-Queue: Max Morin <maxmorin@chromium.org> > Cr-Commit-Position: refs/heads/master@{#557834} TBR=yhirano@chromium.org,qinmin@chromium.org,xingliu@chromium.org,maxmorin@chromium.org Change-Id: I4a0d7eb869a61e38d01424427976121fe7f13384 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 838627, 841666 Reviewed-on: https://chromium-review.googlesource.com/1055710 Reviewed-by: Min Qin <qinmin@chromium.org> Commit-Queue: Min Qin <qinmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#557969}
Denger-Network · May 11, 2018 · 6682345 · 6682345
1 parent ba355f6
commit 6682345
Show file tree

Hide file tree

Showing 14 changed files with 108 additions and 40 deletions.
diff --git a/components/download/internal/common/download_file_impl.cc b/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());

diff --git a/components/download/internal/common/download_file_unittest.cc b/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());

diff --git a/components/download/internal/common/download_item_impl.cc b/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();

diff --git a/components/download/internal/common/download_job_factory.cc b/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;

diff --git a/components/download/internal/common/download_worker.cc b/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) {

diff --git a/components/download/internal/common/download_worker.h b/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,

diff --git a/components/download/internal/common/parallel_download_job.cc b/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.

diff --git a/components/download/internal/common/parallel_download_job.h b/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.

diff --git a/components/download/internal/common/parallel_download_job_unittest.cc b/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();
   }
 

diff --git a/content/browser/download/byte_stream_input_stream.cc b/content/browser/download/byte_stream_input_stream.cc
@@ -4,13 +4,16 @@
 
 #include "content/browser/download/byte_stream_input_stream.h"
 
+#include "components/download/public/common/download_task_runner.h"
 #include "content/browser/byte_stream.h"
 
 namespace content {
 
 ByteStreamInputStream::ByteStreamInputStream(
     std::unique_ptr<ByteStreamReader> stream_reader)
-    : stream_reader_(std::move(stream_reader)),
+    : stream_reader_(
+          stream_reader.release(),
+          base::OnTaskRunnerDeleter(download::GetDownloadTaskRunner())),
       completion_status_(download::DOWNLOAD_INTERRUPT_REASON_NONE) {}
 
 ByteStreamInputStream::~ByteStreamInputStream() = default;

diff --git a/content/browser/download/byte_stream_input_stream.h b/content/browser/download/byte_stream_input_stream.h
@@ -30,7 +30,7 @@ class CONTENT_EXPORT ByteStreamInputStream : public download::InputStream {
 
  private:
   // ByteStreamReader to read from.
-  std::unique_ptr<ByteStreamReader> stream_reader_;
+  std::unique_ptr<ByteStreamReader, base::OnTaskRunnerDeleter> stream_reader_;
 
   // Status when the response completes.
   download::DownloadInterruptReason completion_status_;

diff --git a/content/browser/download/download_browsertest.cc b/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);
-    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());
+    // 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.
-    test_response_handler()->WaitUntilCompletion(2u);
+    // 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);
-    test_response_handler()->WaitUntilCompletion(3u);
-    const TestDownloadResponseHandler::CompletedRequests& completed_requests =
-        test_response_handler()->completed_requests();
-    EXPECT_EQ(kTestRequestCount, static_cast<int>(completed_requests.size()));
+    if (parameters.support_partial_response) {
+      test_response_handler()->WaitUntilCompletion(3u);
+      const TestDownloadResponseHandler::CompletedRequests& completed_requests =
+          test_response_handler()->completed_requests();
+      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

diff --git a/content/public/test/test_download_http_response.cc b/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,7 +300,8 @@ 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];
-  range_.set_first_byte_position(request_range_.first_byte_position());
+  if (parameters_.support_partial_response)
+    range_.set_first_byte_position(request_range_.first_byte_position());
   range_.ComputeBounds(parameters_.size);
 
   response_sent_offset_ = range_.first_byte_position();
@@ -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) !=

diff --git a/content/public/test/test_download_http_response.h b/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;