[Downloads] Do not store error responses during resumption.

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 072ba7b0b55469efc47e767f0fa3da78e1f5457e..f35e8591a7ffcd177f8e7223f12bdcfa04ea1796 100644
--- a/content/browser/download/download_request_core.cc
+++ b/content/browser/download/download_request_core.cc
@@ -48,7 +48,8 @@ DownloadRequestCore::DownloadRequestCore(
       last_buffer_size_(0),
       bytes_read_(0),
       pause_count_(0),
-      was_deferred_(false) {
+      was_deferred_(false),
+      is_resumption_request_(save_info_->offset > 0) {
   DCHECK(request_);
   DCHECK(save_info_);
   DCHECK(!on_ready_to_read_callback_.is_null());
@@ -69,14 +70,22 @@ DownloadRequestCore::~DownloadRequestCore() {
 }
 
 // Send the download creation information to the download thread.
-void DownloadRequestCore::OnResponseStarted(
-    scoped_ptr<DownloadCreateInfo>* create_info,
-    scoped_ptr<ByteStreamReader>* stream_reader) {
+DownloadInterruptReason DownloadRequestCore::OnResponseStarted(
+    scoped_ptr<DownloadCreateInfo>* create_info_out,
+    scoped_ptr<ByteStreamReader>* stream_reader_out) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(save_info_);
   DVLOG(20) << __FUNCTION__ << "()" << DebugString();
   download_start_time_ = base::TimeTicks::Now();
 
+  DownloadInterruptReason result =
+      request()->response_headers()
+          ? HandleSuccessfulServerResponse(*request()->response_headers(),
+                                           save_info_.get())
+          : DOWNLOAD_INTERRUPT_REASON_NONE;
+  if (result != DOWNLOAD_INTERRUPT_REASON_NONE)
+    return result;
+
   // If it's a download, we don't want to poison the cache with it.
   request()->StopCaching();
 
@@ -92,7 +101,7 @@ void DownloadRequestCore::OnResponseStarted(
                                : 0;
 
   // Deleted in DownloadManager.
-  scoped_ptr<DownloadCreateInfo> info(
+  scoped_ptr<DownloadCreateInfo> create_info(
       new DownloadCreateInfo(base::Time::Now(), content_length,
                              request()->net_log(), std::move(save_info_)));
 
@@ -100,25 +109,25 @@ void DownloadRequestCore::OnResponseStarted(
   CreateByteStream(
       base::ThreadTaskRunnerHandle::Get(),
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE),
-      kDownloadByteStreamSize, &stream_writer_, stream_reader);
+      kDownloadByteStreamSize, &stream_writer_, stream_reader_out);
   stream_writer_->RegisterCallback(
       base::Bind(&DownloadRequestCore::ResumeRequest, AsWeakPtr()));
 
-  info->url_chain = request()->url_chain();
-  info->referrer_url = GURL(request()->referrer());
+  create_info->url_chain = request()->url_chain();
+  create_info->referrer_url = GURL(request()->referrer());
   string mime_type;
   request()->GetMimeType(&mime_type);
-  info->mime_type = mime_type;
-  info->remote_address = request()->GetSocketAddress().host();
+  create_info->mime_type = mime_type;
+  create_info->remote_address = request()->GetSocketAddress().host();
   if (request()->response_headers()) {
     // Grab the first content-disposition header.  There may be more than one,
     // though as of this writing, the network stack ensures if there are, they
     // are all duplicates.
     request()->response_headers()->EnumerateHeader(
-        nullptr, "Content-Disposition", &info->content_disposition);
+        nullptr, "Content-Disposition", &create_info->content_disposition);
   }
-  RecordDownloadMimeType(info->mime_type);
-  RecordDownloadContentDisposition(info->content_disposition);
+  RecordDownloadMimeType(create_info->mime_type);
+  RecordDownloadContentDisposition(create_info->content_disposition);
 
   // Get the last modified time and etag.
   const net::HttpResponseHeaders* headers = request()->response_headers();
@@ -127,33 +136,37 @@ void DownloadRequestCore::OnResponseStarted(
       // If we don't have strong validators as per RFC 2616 section 13.3.3, then
       // we neither store nor use them for range requests.
       if (!headers->EnumerateHeader(nullptr, "Last-Modified",
-                                    &info->last_modified))
-        info->last_modified.clear();
-      if (!headers->EnumerateHeader(nullptr, "ETag", &info->etag))
-        info->etag.clear();
-    }
-
-    int status = headers->response_code();
-    if (2 == status / 100 && status != net::HTTP_PARTIAL_CONTENT) {
-      // Success & not range response; if we asked for a range, we didn't
-      // get it--reset the file pointers to reflect that.
-      info->save_info->offset = 0;
-      info->save_info->hash_state = "";
+                                    &create_info->last_modified))
+        create_info->last_modified.clear();
+      if (!headers->EnumerateHeader(nullptr, "ETag", &create_info->etag))
+        create_info->etag.clear();
     }
 
-    if (!headers->GetMimeType(&info->original_mime_type))
-      info->original_mime_type.clear();
+    if (!headers->GetMimeType(&create_info->original_mime_type))
+      create_info->original_mime_type.clear();
   }
 
   // Blink verifies that the requester of this download is allowed to set a
-  // suggested name for the security origin of the downlaod URL. However, this
+  // suggested name for the security origin of the download URL. However, this
   // assumption doesn't hold if there were cross origin redirects. Therefore,
   // clear the suggested_name for such requests.
-  if (info->url_chain.size() > 1 &&
-      info->url_chain.front().GetOrigin() != info->url_chain.back().GetOrigin())
-    info->save_info->suggested_name.clear();
+  if (create_info->url_chain.size() > 1 &&
+      create_info->url_chain.front().GetOrigin() !=
+          create_info->url_chain.back().GetOrigin())
+    create_info->save_info->suggested_name.clear();
 
-  info.swap(*create_info);
+  create_info.swap(*create_info_out);
+
+  return DOWNLOAD_INTERRUPT_REASON_NONE;
+}
+
+DownloadInterruptReason DownloadRequestCore::OnRequestRedirected(
+    const net::RedirectInfo& redirect_info) {
+  // A redirect while attempting a partial resumption indicates a potential
+  // middle box. Trigger another interruption so that the DownloadItem can
+  // retry.
+  return is_resumption_request_ ? DOWNLOAD_INTERRUPT_REASON_SERVER_UNREACHABLE
+                                : DOWNLOAD_INTERRUPT_REASON_NONE;
 }
 
 // Create a new buffer, which will be handed to the download thread for file
@@ -221,26 +234,7 @@ DownloadInterruptReason DownloadRequestCore::OnResponseCompleted(
             << " status.error() = " << status.error()
             << " response_code = " << response_code;
 
-  net::Error error_code = net::OK;
-  if (status.status() == net::URLRequestStatus::FAILED ||
-      // Note cancels as failures too.
-      status.status() == net::URLRequestStatus::CANCELED) {
-    error_code = static_cast<net::Error>(status.error());  // Normal case.
-    // Make sure that at least the fact of failure comes through.
-    if (error_code == net::OK)
-      error_code = net::ERR_FAILED;
-  }
-
-  // ERR_CONTENT_LENGTH_MISMATCH and ERR_INCOMPLETE_CHUNKED_ENCODING are
-  // allowed since a number of servers in the wild close the connection too
-  // early by mistake. Other browsers - IE9, Firefox 11.0, and Safari 5.1.4 -
-  // treat downloads as complete in both cases, so we follow their lead.
-  if (error_code == net::ERR_CONTENT_LENGTH_MISMATCH ||
-      error_code == net::ERR_INCOMPLETE_CHUNKED_ENCODING) {
-    error_code = net::OK;
-  }
-  DownloadInterruptReason reason = ConvertNetErrorToInterruptReason(
-      error_code, DOWNLOAD_INTERRUPT_FROM_NETWORK);
+  DownloadInterruptReason reason = HandleRequestStatus(status);
 
   if (status.status() == net::URLRequestStatus::CANCELED &&
       status.error() == net::ERR_ABORTED) {
@@ -252,49 +246,12 @@ DownloadInterruptReason DownloadRequestCore::OnResponseCompleted(
     // to a user action.
     // TODO(ahendrickson) -- Find a better set of codes to use here, as
     // CANCELED/ERR_ABORTED can occur for reasons other than user cancel.
-    if (net::IsCertStatusError(request()->ssl_info().cert_status))
-      reason = DOWNLOAD_INTERRUPT_REASON_SERVER_CERT_PROBLEM;
-    else
+    if (net::IsCertStatusError(request()->ssl_info().cert_status)) {
+      reason = is_resumption_request_
+                   ? DOWNLOAD_INTERRUPT_REASON_SERVER_UNREACHABLE
+                   : DOWNLOAD_INTERRUPT_REASON_SERVER_CERT_PROBLEM;
+    } else {
       reason = DOWNLOAD_INTERRUPT_REASON_USER_CANCELED;
-  }
-
-  if (status.is_success() && reason == DOWNLOAD_INTERRUPT_REASON_NONE &&
-      request()->response_headers()) {
-    // Handle server's response codes.
-    switch (response_code) {
-      case -1:  // Non-HTTP request.
-      case net::HTTP_OK:
-      case net::HTTP_CREATED:
-      case net::HTTP_ACCEPTED:
-      case net::HTTP_NON_AUTHORITATIVE_INFORMATION:
-      case net::HTTP_RESET_CONTENT:
-      case net::HTTP_PARTIAL_CONTENT:
-        // Expected successful codes.
-        break;
-      case net::HTTP_NO_CONTENT:
-      case net::HTTP_NOT_FOUND:
-        reason = DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
-        break;
-      case net::HTTP_REQUESTED_RANGE_NOT_SATISFIABLE:
-        // Retry by downloading from the start automatically:
-        // If we haven't received data when we get this error, we won't.
-        reason = DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE;
-        break;
-      case net::HTTP_UNAUTHORIZED:
-        // Server didn't authorize this request.
-        reason = DOWNLOAD_INTERRUPT_REASON_SERVER_UNAUTHORIZED;
-        break;
-      case net::HTTP_FORBIDDEN:
-        // Server forbids access to this resource.
-        reason = DOWNLOAD_INTERRUPT_REASON_SERVER_FORBIDDEN;
-        break;
-      default:  // All other errors.
-        // Redirection and informational codes should have been handled earlier
-        // in the stack.
-        DCHECK_NE(3, response_code / 100);
-        DCHECK_NE(1, response_code / 100);
-        reason = DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
-        break;
     }
   }
 
@@ -363,4 +320,121 @@ std::string DownloadRequestCore::DebugString() const {
       request() ? request()->url().spec().c_str() : "<NULL request>");
 }
 
+// static
+DownloadInterruptReason DownloadRequestCore::HandleRequestStatus(
+    const net::URLRequestStatus& status) {
+  net::Error error_code = net::OK;
+  if (status.status() == net::URLRequestStatus::FAILED ||
+      // Note cancels as failures too.
+      status.status() == net::URLRequestStatus::CANCELED) {
+    error_code = static_cast<net::Error>(status.error());  // Normal case.
+    // Make sure that at least the fact of failure comes through.
+    if (error_code == net::OK)
+      error_code = net::ERR_FAILED;
+  }
+
+  // ERR_CONTENT_LENGTH_MISMATCH and ERR_INCOMPLETE_CHUNKED_ENCODING are
+  // allowed since a number of servers in the wild close the connection too
+  // early by mistake. Other browsers - IE9, Firefox 11.0, and Safari 5.1.4 -
+  // treat downloads as complete in both cases, so we follow their lead.
+  if (error_code == net::ERR_CONTENT_LENGTH_MISMATCH ||
+      error_code == net::ERR_INCOMPLETE_CHUNKED_ENCODING) {
+    error_code = net::OK;
+  }
+  DownloadInterruptReason reason = ConvertNetErrorToInterruptReason(
+      error_code, DOWNLOAD_INTERRUPT_FROM_NETWORK);
+
+  return reason;
+}
+
+// static
+DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
+    const net::HttpResponseHeaders& http_headers,
+    DownloadSaveInfo* save_info) {
+  switch (http_headers.response_code()) {
+    case -1:  // Non-HTTP request.
+    case net::HTTP_OK:
+    case net::HTTP_NON_AUTHORITATIVE_INFORMATION:
+    case net::HTTP_PARTIAL_CONTENT:
+      // Expected successful codes.
+      break;
+
+    case net::HTTP_CREATED:
+    case net::HTTP_ACCEPTED:
+      // Per RFC 2616 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
+      // handling for these response and they are downloaded the same as a
+      // regular response.
+      break;
+
+    case net::HTTP_NO_CONTENT:
+    case net::HTTP_RESET_CONTENT:
+    // These two status codes don't have an entity (or rather RFC 2616
+    // requires that there be no entity). They are treated the same as the
+    // resource not being found since there is no entity to download.
+
+    case net::HTTP_NOT_FOUND:
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+      break;
+
+    case net::HTTP_REQUESTED_RANGE_NOT_SATISFIABLE:
+      // Retry by downloading from the start automatically:
+      // If we haven't received data when we get this error, we won't.
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE;
+      break;
+    case net::HTTP_UNAUTHORIZED:
+    case net::HTTP_PROXY_AUTHENTICATION_REQUIRED:
+      // Server didn't authorize this request.
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_UNAUTHORIZED;
+      break;
+    case net::HTTP_FORBIDDEN:
+      // Server forbids access to this resource.
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_FORBIDDEN;
+      break;
+    default:  // All other errors.
+      // Redirection and informational codes should have been handled earlier
+      // in the stack.
+      DCHECK_NE(3, http_headers.response_code() / 100);
+      DCHECK_NE(1, http_headers.response_code() / 100);
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
+  }
+
+  if (save_info->offset > 0) {
+    // The caller is expecting a partial response.
+
+    if (http_headers.response_code() != net::HTTP_PARTIAL_CONTENT) {
+      // Requested a partial range, but received the entire response.
+      save_info->offset = 0;
+      save_info->hash_state.clear();
+      return DOWNLOAD_INTERRUPT_REASON_NONE;
+    }
+
+    int64_t first_byte = -1;
+    int64_t last_byte = -1;
+    int64_t length = -1;
+    if (!http_headers.GetContentRange(&first_byte, &last_byte, &length))
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+    DCHECK_GE(first_byte, 0);
+
+    if (first_byte != save_info->offset) {
+      // The server returned a different range than the one we requested. Assume
+      // the response is bad.
+      //
+      // In the future we should consider allowing offsets that are less than
+      // the offset we've requested, since in theory we can truncate the partial
+      // file at the offset and continue.
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+    }
+
+    return DOWNLOAD_INTERRUPT_REASON_NONE;
+  }
+
+  if (http_headers.response_code() == net::HTTP_PARTIAL_CONTENT)
+    return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+
+  return DOWNLOAD_INTERRUPT_REASON_NONE;
+}
+
 }  // namespace content