Propagate server response error and interrupt the download.

content/browser/download/download_worker.cc

Index: content/browser/download/download_worker.cc
diff --git a/content/browser/download/download_worker.cc b/content/browser/download/download_worker.cc
index d537b1d9951e818909a3ebf3d8957f794142ba1e..2992a4dc3fa7a2d8ad16e4d48d869d29b1295f4c 100644
--- a/content/browser/download/download_worker.cc
+++ b/content/browser/download/download_worker.cc
@@ -32,7 +32,15 @@ CreateUrlDownloader(std::unique_ptr<DownloadUrlParameters> params,
 
 }  // 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;
 
@@ -48,15 +56,18 @@ void DownloadWorker::SendRequest(
 }
 
 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(
@@ -66,13 +77,23 @@ void DownloadWorker::OnUrlDownloaderStarted(
   // |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;
   }