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

This reverts commit 05267e5e.

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 13077433.
> 
> 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 189d1033.
> > 
> > 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/1053729Reviewed-by: Max Morin <maxmorin@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557834}

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]),
-      strlen(kTestData1), DownloadSaveInfo::kLengthFullContent);
+      std::unique_ptr<MockInputStream>(additional_streams_[0]), 0,
+      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,8 +1479,9 @@ void DownloadItemImpl::Start(
   if (state_ == RESUMING_INTERNAL)
     UpdateValidatorsOnResumption(new_create_info);
 
-  // If the download is not parallel, clear the |received_slices_|.
-  if (!received_slices_.empty() && !job_->IsParallelizable()) {
+  // If the download is not parallel download during resumption, clear the
+  // |received_slices_|.
+  if (!job_->IsParallelizable() && !received_slices_.empty()) {
     destination_info_.received_bytes =
         GetMaxContiguousDataBlockSizeFromBeginning(received_slices_);
     received_slices_.clear();

components/download/internal/common/download_job_factory.cc

@@ -44,12 +44,10 @@ 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 &&
-                           partial_response_success;
+                           satisfy_connection_type && http_get_method;
 
   if (!IsParallelDownloadEnabled())
     return is_parallelizable;

components/download/internal/common/download_worker.cc

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

components/download/internal/common/download_worker.h

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

components/download/internal/common/parallel_download_job.cc

@@ -123,14 +123,9 @@ void ParallelDownloadJob::BuildParallelRequestAfterDelay() {
 
 void ParallelDownloadJob::OnInputStreamReady(
     DownloadWorker* worker,
-    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());
-  }
+    std::unique_ptr<InputStream> input_stream) {
+  bool 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,10 +68,8 @@ class COMPONENTS_DOWNLOAD_EXPORT ParallelDownloadJob
   friend class ParallelDownloadJobTest;
 
   // DownloadWorker::Delegate implementation.
-  void OnInputStreamReady(
-      DownloadWorker* worker,
-      std::unique_ptr<InputStream> input_stream,
-      std::unique_ptr<DownloadCreateInfo> download_create_info) override;
+  void OnInputStreamReady(DownloadWorker* worker,
+                          std::unique_ptr<InputStream> input_stream) override;
 
   // Build parallel requests after a delay, to effectively measure the single
   // stream bandwidth.

components/download/internal/common/parallel_download_job_unittest.cc

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

content/browser/download/byte_stream_input_stream.cc

@@ -4,16 +4,13 @@
 
 #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_(
-          stream_reader.release(),
-          base::OnTaskRunnerDeleter(download::GetDownloadTaskRunner())),
+    : stream_reader_(std::move(stream_reader)),
       completion_status_(download::DOWNLOAD_INTERRUPT_REASON_NONE) {}
 
 ByteStreamInputStream::~ByteStreamInputStream() = default;

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, base::OnTaskRunnerDeleter> stream_reader_;
+  std::unique_ptr<ByteStreamReader> stream_reader_;
 
   // Status when the response completes.
   download::DownloadInterruptReason completion_status_;

content/browser/download/download_browsertest.cc

@@ -974,8 +974,7 @@ class ParallelDownloadTest : public DownloadContentTest {
   void RunResumptionTest(
       const download::DownloadItem::ReceivedSlices& received_slices,
       int64_t total_length,
-      size_t expected_request_count,
-      bool support_partial_response) {
+      size_t expected_request_count) {
     EXPECT_TRUE(
         base::FeatureList::IsEnabled(download::features::kParallelDownloading));
     GURL url = TestDownloadHttpResponse::GetNextURLForDownload();
@@ -984,7 +983,6 @@ 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);
@@ -1001,17 +999,12 @@ 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());
-    }
+    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,
@@ -1033,29 +1026,22 @@ 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);
-    // 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);
+    // 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);
 
     // 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(3u, completed_requests.size());
-    }
+    test_response_handler()->WaitUntilCompletion(3u);
+    const TestDownloadResponseHandler::CompletedRequests& completed_requests =
+        test_response_handler()->completed_requests();
+    EXPECT_EQ(kTestRequestCount, static_cast<int>(completed_requests.size()));
     ReadAndVerifyFileContents(parameters.pattern_generator_seed,
                               parameters.size, download->GetTargetFilePath());
   }
@@ -3196,17 +3182,6 @@ 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
@@ -3217,8 +3192,7 @@ IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, Resumption) {
       download::DownloadItem::ReceivedSlice(2000000, 1000,
                                             false /* finished */)};
 
-  RunResumptionTest(received_slices, 3000000, kTestRequestCount,
-                    true /* support_partial_response */);
+  RunResumptionTest(received_slices, 3000000, kTestRequestCount);
 }
 
 // Verifies that if the last slice is finished, parallel download resumption
@@ -3233,8 +3207,7 @@ 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,
-                    true /* support_partial_response */);
+  RunResumptionTest(received_slices, 3000000, kTestRequestCount - 1);
 }
 
 // Verifies that if the last slice is finished, but the database record is not
@@ -3250,23 +3223,7 @@ 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,
-                    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 */);
+  RunResumptionTest(received_slices, 3000000, kTestRequestCount);
 }
 
 // Test to verify that the browser-side enforcement of X-Frame-Options does

content/public/test/test_download_http_response.cc

@@ -121,7 +121,6 @@ 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) {}
 
@@ -137,7 +136,6 @@ 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)),
@@ -148,12 +146,11 @@ TestDownloadHttpResponse::Parameters::Parameters(Parameters&& that)
 TestDownloadHttpResponse::Parameters& TestDownloadHttpResponse::Parameters::
 operator=(Parameters&& that) {
   etag = std::move(that.etag);
-  last_modified = std::move(that.last_modified);
+  last_modified = std::move(that.etag);
   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;
@@ -300,8 +297,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_.set_first_byte_position(request_range_.first_byte_position());
   range_.ComputeBounds(parameters_.size);
 
   response_sent_offset_ = range_.first_byte_position();
@@ -329,7 +325,7 @@ void TestDownloadHttpResponse::SendResponseHeaders() {
 std::string TestDownloadHttpResponse::GetDefaultResponseHeaders() {
   std::string headers;
   // Send partial response.
-  if (parameters_.support_partial_response && parameters_.support_byte_ranges &&
+  if (parameters_.support_byte_ranges &&
       request_.headers.find(net::HttpRequestHeaders::kIfRange) !=
           request_.headers.end() &&
       request_.headers.at(net::HttpRequestHeaders::kIfRange) ==
@@ -339,7 +335,7 @@ std::string TestDownloadHttpResponse::GetDefaultResponseHeaders() {
   }
 
   // Send precondition failed for "If-Match" request header.
-  if (parameters_.support_partial_response && parameters_.support_byte_ranges &&
+  if (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,12 +108,6 @@ 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;