Propagate server response error and interrupt the download.

content/browser/download/download_worker.cc

 // Copyright 2017 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_worker.h"
 
 #include "content/browser/download/download_create_info.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/web_contents.h"
 
@@ skipping 14 matching lines @@
                                                    params.get());
 
   return std::unique_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread>(
       UrlDownloader::BeginDownload(delegate, std::move(url_request),
                                    params->referrer())
           .release());
 }
 
 }  // namespace
 
-DownloadWorker::DownloadWorker() : weak_factory_(this) {}
+DownloadWorker::DownloadWorker(DownloadWorker::Delegate* delegate,
+                               int64_t offset,
+                               int64_t length)
+    : delegate_(delegate),
+      offset_(offset),
+      length_(length),
+      weak_factory_(this) {
+  DCHECK(delegate_);
+}
 
 DownloadWorker::~DownloadWorker() = default;
 
 void DownloadWorker::SendRequest(
     std::unique_ptr<DownloadUrlParameters> params) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   BrowserThread::PostTaskAndReplyWithResult(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&CreateUrlDownloader, base::Passed(&params),
                  weak_factory_.GetWeakPtr()),
       base::Bind(&DownloadWorker::AddUrlDownloader,
                  weak_factory_.GetWeakPtr()));
 }
 
 void DownloadWorker::Pause() {
-  request_handle_->PauseRequest();
+  if (request_handle_)
+    request_handle_->PauseRequest();
 }
 
 void DownloadWorker::Resume() {
-  request_handle_->ResumeRequest();
+  if (request_handle_)
+    request_handle_->ResumeRequest();
 }
 
 void DownloadWorker::Cancel() {
-  request_handle_->CancelRequest();
+  if (request_handle_)
+    request_handle_->CancelRequest();
 }
 
 void DownloadWorker::OnUrlDownloaderStarted(
     std::unique_ptr<DownloadCreateInfo> create_info,
     std::unique_ptr<ByteStreamReader> stream_reader,
     const DownloadUrlParameters::OnStartedCallback& callback) {
   // |callback| is not used in subsequent requests.
   DCHECK(callback.is_null());
 
-  // TODO(xingliu): Pass the |stream_reader| to parallel job and handle failed
-  // request.
+  // If the server gave us a different part of the content, due to "If-Match" or
+  // "If-Unmodified-Since" headers, reports an error, which results in the
+  // download to be restarted.
+  if (create_info->save_info->offset != offset() ||
+      create_info->save_info->length != length()) {

[asanka, 2017/03/17 15:54:22]

This condition will trip for any half-open slice. But that's moot given the
following.

OnUrlDownloaderStarted() shouldn't be getting invoked with a mismatched slice.
If the server didn't respond with the correct slice, that should be handled by
DownloadRequestCore.

Architecturally speaking, it would be brittle if we need to handle mismatched
slices at this layer. The result we should expect at this point should be either
an error, or a correct slice. Otherwise the all consumers downstream from
DownloadRequestCore would need to handle a much larger set of possible outcomes
than is practical.

There's an additional complication where DownloadRequestCore doesn't have all
the bits it needs to validate a server response. For example, the code there
currently relies on heuristics to determine whether it's acceptable to receive
the entire response when the request was for a slice. This is currently
acceptable only for the primary resumption request. Unfortunately it's not
always possible for DRC to tell the difference between the primary resumption
request, and the last slice. Both of those are half-open and DRC accepts HTTP
200 responses for both.


I'd suggest:

* Explicitly indicate to DRC whether a full entity response is acceptable.
Missing such an indication, DRC should strictly validate the response slice vs.
the request rage.

* Downstream from DRC, always assume that the response is valid. If the consumer
explicitly indicated support for full entity responses, then they should check
for that condition. Other than those, any deviation should have resulted in an
error.

[xingliu, 2017/03/17 21:14:36]

Good point, It will be cleaner to let DownloadRequestCore handle this.
added a TODO here.
 
I'll do this in a separate CL. We also need to pull the http version from there.

[asanka, 2017/03/20 20:50:20]

Don't forget the part about explicitly indicating whether receiving the entire
entity body is acceptable.

[xingliu, 2017/03/21 17:32:15]

Yeah, that's on my list.

+    // TODO(xingliu): Add the interrupt reason and metric data for precondition
+    // failure.
+    VLOG(kVerboseLevel) << "Server response does not match the offset or "
+                           "length in the request.";
+    create_info->result = DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE;
+  }
+
   if (create_info->result !=
       DownloadInterruptReason::DOWNLOAD_INTERRUPT_REASON_NONE) {
-    VLOG(kVerboseLevel) << "Parallel download sub request failed. reason = "
+    VLOG(kVerboseLevel) << "Parallel download sub-request failed. reason = "
                         << create_info->result;
-    NOTIMPLEMENTED();
+    delegate_->OnServerResponseError(this, create_info->result);
     return;
   }
 
   request_handle_ = std::move(create_info->request_handle);
 }
 
 void DownloadWorker::OnUrlDownloaderStopped(UrlDownloader* downloader) {
   // Release the |url_downloader_|, the object will be deleted on IO thread.
   url_downloader_.reset();
 }
 
 void DownloadWorker::AddUrlDownloader(
     std::unique_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread>
         downloader) {
   url_downloader_ = std::move(downloader);
 }
 
 }  // namespace content