Range request support for parallel download in DownloadRequestCore.

content/browser/download/download_request_core.cc

Index: content/browser/download/download_request_core.cc
diff --git a/content/browser/download/download_request_core.cc b/content/browser/download/download_request_core.cc
index 0fec3f3b8e5730fd3f2b16884f2ca87a9d4e819c..f8569258ea246b3249bfe26bb0ed620b0d050f82 100644
--- a/content/browser/download/download_request_core.cc
+++ b/content/browser/download/download_request_core.cc
@@ -158,19 +158,25 @@ std::unique_ptr<net::URLRequest> DownloadRequestCore::CreateRequestOnIOThread(
   bool has_last_modified = !params->last_modified().empty();
   bool has_etag = !params->etag().empty();
 
-  // If we've asked for a range, we want to make sure that we only get that
-  // range if our current copy of the information is good.  We shouldn't be
-  // asked to continue if we don't have a verifier.
-  DCHECK(params->offset() == 0 || has_etag || has_last_modified);
-
-  // If we're not at the beginning of the file, retrieve only the remaining
-  // portion.
-  if (params->offset() > 0 && (has_etag || has_last_modified)) {
-    request->SetExtraRequestHeaderByName(
-        "Range", base::StringPrintf("bytes=%" PRId64 "-", params->offset()),
-        true);
-
-    // In accordance with RFC 2616 Section 14.27, use If-Range to specify that
+  // Strong validator(i.e. etag or last modified) is required in range requests
+  // for download resumption and parallel download.
+  DCHECK((params->offset() == 0 &&
+          params->length() == DownloadSaveInfo::kLengthFullContent) ||
+         has_etag || has_last_modified);
+
+  // Add "Range" and "If-Range" request header fields if we ask for a range.
+  if ((params->offset() > 0 ||
+       params->length() > DownloadSaveInfo::kLengthFullContent) &&

[asanka, 2017/02/01 22:51:17]

This expression requires that kLengthFullContent be 0. Any other value would be
nonsensical.

Perhaps just check that params->length() != kLengthFullContent ?

[xingliu, 2017/02/02 05:46:36]

Make sense, changed to != check in this file.

+      (has_etag || has_last_modified)) {
+    std::string range_header =
+        params->length() > DownloadSaveInfo::kLengthFullContent
+            ? base::StringPrintf("bytes=%" PRId64 "-%" PRId64, params->offset(),
+                                 params->offset() + params->length() - 1)
+            : base::StringPrintf("bytes=%" PRId64 "-", params->offset());
+
+    request->SetExtraRequestHeaderByName("Range", range_header, true);
+
+    // In accordance with RFC 7233 Section 3.2, use If-Range to specify that
     // the server return the entire entity if the validator doesn't match.
     // Last-Modified can be used in the absence of ETag as a validator if the
     // response headers satisfied the HttpUtil::HasStrongValidators() predicate.
@@ -304,7 +310,7 @@ bool DownloadRequestCore::OnResponseStarted(
   const net::HttpResponseHeaders* headers = request()->response_headers();
   if (headers) {
     if (headers->HasStrongValidators()) {
-      // If we don't have strong validators as per RFC 2616 section 13.3.3, then
+      // If we don't have strong validators as per RFC 7232 section 2, then
       // we neither store nor use them for range requests.
       if (!headers->EnumerateHeader(nullptr, "Last-Modified",
                                     &create_info->last_modified))
@@ -546,7 +552,7 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
 
     case net::HTTP_CREATED:
     case net::HTTP_ACCEPTED:
-      // Per RFC 2616 the entity being transferred is metadata about the
+      // Per RFC 7231 the entity being transferred is metadata about the
       // resource at the target URL and not the resource at that URL (or the
       // resource that would be at the URL once processing is completed in the
       // case of HTTP_ACCEPTED). However, we currently don't have special
@@ -556,7 +562,7 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
 
     case net::HTTP_NO_CONTENT:
     case net::HTTP_RESET_CONTENT:
-    // These two status codes don't have an entity (or rather RFC 2616
+    // These two status codes don't have an entity (or rather RFC 7231
     // requires that there be no entity). They are treated the same as the
     // resource not being found since there is no entity to download.
 
@@ -586,11 +592,16 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
       return DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
   }
 
-  if (save_info && save_info->offset > 0) {
-    // The caller is expecting a partial response.
-
+  // The caller is expecting a partial response.
+  if (save_info && (save_info->offset > 0 || save_info->length > 0)) {
     if (http_headers.response_code() != net::HTTP_PARTIAL_CONTENT) {
-      // Requested a partial range, but received the entire response.
+      // Server should send partial content when we ask for a range with last
+      // byte position specified. e.g. "Range:bytes=50-99".

[asanka, 2017/02/01 22:51:17]

Since we specified an If-Range header, we specifically asked the server to send
the entire entity if the validators didn't match.

We are trying to do two things here:

  * Assuming we have a prefix of the entity, we can send a request for the
remainder. This is Range: bytes=123- . If there's a mismatch in our validators,
then the server send the entire resource and we discard the prefix and store the
entity.

  * With-or-without a prefix, we want to fetch some random part of the entity.
This is 'Range: bytes=123-345'. If validation fails, then we don't necessarily
want to revert to fetching the entire entity. Instead we fail and allow the
caller to figure out how they want to handle this.

In the case of multiple donwload jobs, you'd now be able to ensure that only the
primary job will fetch the entire resource. We really don't want each job to
fetch full separate copies :-)

So, with apologies, the case of the length-capped fetch should be handled as:

   Range: bytes=123-235
   If-Match: etag

  or

   Range: bytes=123-234
   If-Unmodified-Since: last-modified

This way, the server will fail the request with a HTTP 412 rather than send the
entire response body.

[xingliu, 2017/02/02 05:46:36]

Thanks for looking into RFC details here.

Yeah, I'm thinking of If-Range is a bit weird for a slice. It can work but need
to drop the request somewhere when the server gives 200 response for a slice
like "Range:bytes=x-y".

Changed to send If-Match and If-Unmodified-Since together. In RFC, these two
fields can stack together.

+      if (save_info->length > DownloadSaveInfo::kLengthFullContent)
+        return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+
+      // Requested a partial range, but received the entire response, when
+      // the range header is "Range:bytes={offset}-".
       save_info->offset = 0;
       save_info->hash_of_partial_file.clear();
       save_info->hash_state.reset();
@@ -604,7 +615,9 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
       return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
     DCHECK_GE(first_byte, 0);
 
-    if (first_byte != save_info->offset) {
+    if (first_byte != save_info->offset ||
+        (save_info->length > 0 &&
+         last_byte != save_info->offset + save_info->length - 1)) {
       // The server returned a different range than the one we requested. Assume
       // the response is bad.
       //