[Downloads] Always call DM::StartDownload() for explicit downloads.

content/browser/download/download_resource_handler.cc

 // Copyright (c) 2012 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_resource_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 16 matching lines @@
 #include "content/public/common/resource_response.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_request_context.h"
 
 namespace content {
 namespace {
 
-void CallStartedCBOnUIThread(
-    const DownloadUrlParameters::OnStartedCallback& started_cb,
-    DownloadItem* item,
-    DownloadInterruptReason interrupt_reason) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (started_cb.is_null())
-    return;
-  started_cb.Run(item, interrupt_reason);
-}
-
 // Static function in order to prevent any accidental accesses to
 // DownloadResourceHandler members from the UI thread.

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

nit: This comment seems outdated; it implies that this is a class method.

[asanka, 2014/03/19 20:37:06]

Removed comment.

-static void StartOnUIThread(
+void StartOnUIThread(
     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<ByteStreamReader> stream,
     const DownloadUrlParameters::OnStartedCallback& started_cb) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadManager* download_manager = info->request_handle.GetDownloadManager();
   if (!download_manager) {
     // NULL in unittests or if the page closed right after starting the
     // download.
     if (!started_cb.is_null())
       started_cb.Run(NULL, DOWNLOAD_INTERRUPT_REASON_USER_CANCELED);
 
     // |stream| gets deleted on non-FILE thread, but it's ok since
     // we're not using stream_writer_ yet.
 
     return;
   }
 
   download_manager->StartDownload(info.Pass(), stream.Pass(), started_cb);
 }
 
+DownloadInterruptReason HTTPResponseCodeToInterruptReason(int response_code) {
+  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:
+      return DOWNLOAD_INTERRUPT_REASON_NONE;
+    case net::HTTP_NO_CONTENT:
+    case net::HTTP_NOT_FOUND:
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+    case net::HTTP_PRECONDITION_FAILED:
+      // Failed our 'If-Unmodified-Since' or 'If-Match'; see
+      // download_manager_impl.cc BeginDownload()
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION;
+    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;
+    // TODO(asanka): Add handling for 5xx errors with backoff semantics.
+    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);
+      return DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
+  }
+}
+
 }  // namespace
 
 const int DownloadResourceHandler::kDownloadByteStreamSize = 100 * 1024;
 
 DownloadResourceHandler::DownloadResourceHandler(
     uint32 id,
     net::URLRequest* request,
     const DownloadUrlParameters::OnStartedCallback& started_cb,
     scoped_ptr<DownloadSaveInfo> save_info)
     : ResourceHandler(request),
       download_id_(id),
       started_cb_(started_cb),
       save_info_(save_info.Pass()),
       last_buffer_size_(0),
       bytes_read_(0),
       pause_count_(0),
       was_deferred_(false),
-      on_response_started_called_(false) {
+      download_manager_notified_(false) {
   RecordDownloadCount(UNTHROTTLED_COUNT);
 }
 
 bool DownloadResourceHandler::OnUploadProgress(int request_id,
                                                uint64 position,
                                                uint64 size) {
   return true;
 }
 
 bool DownloadResourceHandler::OnRequestRedirected(
     int request_id,
     const GURL& url,
     ResourceResponse* response,
     bool* defer) {
   // We treat a download as a main frame load, and thus update the policy URL
   // on redirects.
   request()->set_first_party_for_cookies(url);
   return true;
 }
 
 // Send the download creation information to the download thread.
 bool DownloadResourceHandler::OnResponseStarted(
     int request_id,
     ResourceResponse* response,
     bool* defer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  // There can be only one (call)
-  DCHECK(!on_response_started_called_);
-  on_response_started_called_ = true;
-
   VLOG(20) << __FUNCTION__ << "()" << DebugString()
            << " request_id = " << request_id;
   download_start_time_ = base::TimeTicks::Now();
 
   // If it's a download, we don't want to poison the cache with it.
   request()->StopCaching();
 
   // Lower priority as well, so downloads don't contend for resources
   // with main frames.
   request()->SetPriority(net::IDLE);
 
+  const net::HttpResponseHeaders* headers = request()->response_headers();
+  DownloadInterruptReason interrupt_reason =
+      (headers ? HTTPResponseCodeToInterruptReason(headers->response_code())
+               : DOWNLOAD_INTERRUPT_REASON_NONE);
+
+  if (interrupt_reason != DOWNLOAD_INTERRUPT_REASON_NONE)
+    NotifyDownloadManagerOfFailedRequest(interrupt_reason);
+  else
+    NotifyDownloadManagerOfSuccessfulResponseStart(response);
+
+  return true;
+}
+
+void DownloadResourceHandler::NotifyDownloadManagerOfSuccessfulResponseStart(
+    ResourceResponse* response) {
+  // NotifyDownloadManagerOfFailedRequest or
+  // NotifyDownloadManagerOfSuccessfulResponseStart can only be called once.
+  DCHECK(!download_manager_notified_);
+  download_manager_notified_ = true;
+
+  const ResourceRequestInfoImpl* request_info = GetRequestInfo();
+  const net::HttpResponseHeaders* headers = request()->response_headers();
+
   // If the content-length header is not present (or contains something other
   // than numbers), the incoming content_length is -1 (unknown size).
   // Set the content length to 0 to indicate unknown size to DownloadManager.
   int64 content_length =
       response->head.content_length > 0 ? response->head.content_length : 0;

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

nit, question: It feels a little funny to be both playing around in the
HttpResponseHeaders and reaching into the blink based content structures; is
there a reason to use response-> here instead of getting the same information
from headers?

[asanka, 2014/03/19 20:37:06]

The URLRequestJob provides the expected content length that percolates up to
response->head.content_length. This may or may not correspond to a (possibly
non-existent) Content-Length header. The HttpResponseHeaders should only be used
for HTTP specific things.

 
-  const ResourceRequestInfoImpl* request_info = GetRequestInfo();
-
-  // Deleted in DownloadManager.
   scoped_ptr<DownloadCreateInfo> info(
       new DownloadCreateInfo(base::Time::Now(),
                              content_length,
                              request()->net_log(),
                              request_info->HasUserGesture(),
                              request_info->GetPageTransition(),
                              save_info_.Pass()));
 
-  // Create the ByteStream for sending data to the download sink.
-  scoped_ptr<ByteStreamReader> stream_reader;
-  CreateByteStream(
-      base::MessageLoopProxy::current(),
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE),
-      kDownloadByteStreamSize, &stream_writer_, &stream_reader);
-  stream_writer_->RegisterCallback(
-      base::Bind(&DownloadResourceHandler::ResumeRequest, AsWeakPtr()));
-
-  info->download_id = download_id_;
-  info->url_chain = request()->url_chain();
-  info->referrer_url = GURL(request()->referrer());
-  info->mime_type = response->head.mime_type;
-  info->remote_address = request()->GetSocketAddress().host();
-  request()->GetResponseHeaderByName("content-disposition",
-                                     &info->content_disposition);
-  RecordDownloadMimeType(info->mime_type);
-  RecordDownloadContentDisposition(info->content_disposition);
-
-  info->request_handle =
-      DownloadRequestHandle(AsWeakPtr(), request_info->GetChildID(),
-                            request_info->GetRouteID(),
-                            request_info->GetRequestID());
-
-  // Get the last modified time and etag.
-  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
       // we neither store nor use them for range requests.
       if (!headers->EnumerateHeader(NULL, "Last-Modified",
                                     &info->last_modified))
         info->last_modified.clear();
       if (!headers->EnumerateHeader(NULL, "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 = "";
     }
 
     if (!headers->GetMimeType(&info->original_mime_type))
       info->original_mime_type.clear();
   }
 
+  info->download_id = download_id_;
+  info->url_chain = request()->url_chain();
+  info->referrer_url = GURL(request()->referrer());
+
+  scoped_ptr<ByteStreamReader> stream_reader;
+  // Create the ByteStream for sending data to the download sink.
+  CreateByteStream(
+      base::MessageLoopProxy::current(),
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE),
+      kDownloadByteStreamSize,
+      &stream_writer_,
+      &stream_reader);
+  stream_writer_->RegisterCallback(
+      base::Bind(&DownloadResourceHandler::ResumeRequest, AsWeakPtr()));
+
+  info->mime_type = response->head.mime_type;
+  info->remote_address = request()->GetSocketAddress().host();
+  request()->GetResponseHeaderByName("content-disposition",
+                                     &info->content_disposition);

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

nit, suggestion: It makes some sense to me to switch around the order in which
this stuff is done, but I'm not sure why creation of the ByteStream should be in
the middle of setting various fields on the info; before or after makes more
sense to me?

[asanka, 2014/03/19 20:37:06]

Done.

+  RecordDownloadMimeType(info->mime_type);
+  RecordDownloadContentDisposition(info->content_disposition);
+
+  info->request_handle = DownloadRequestHandle(AsWeakPtr(),
+                                               request_info->GetChildID(),
+                                               request_info->GetRouteID(),
+                                               request_info->GetRequestID());
+
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&StartOnUIThread,
                  base::Passed(&info),
                  base::Passed(&stream_reader),
                  // Pass to StartOnUIThread so that variable
                  // access is always on IO thread but function
                  // is called on UI thread.
                  started_cb_));
   // Guaranteed to be called in StartOnUIThread
   started_cb_.Reset();
-
-  return true;
 }
 
-void DownloadResourceHandler::CallStartedCB(
-    DownloadItem* item,
+void DownloadResourceHandler::NotifyDownloadManagerOfFailedRequest(
     DownloadInterruptReason interrupt_reason) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (started_cb_.is_null())
-    return;
-  BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(
-          &CallStartedCBOnUIThread, started_cb_, item, interrupt_reason));
+  // NotifyDownloadManagerOfFailedRequest or
+  // NotifyDownloadManagerOfSuccessfulResponseStart can only be called once.
+  DCHECK(!download_manager_notified_);
+  DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
+  download_manager_notified_ = true;
+
+  const ResourceRequestInfoImpl* request_info = GetRequestInfo();
+
+  scoped_ptr<DownloadCreateInfo> info(
+      new DownloadCreateInfo(base::Time::Now(),
+                             -1,
+                             request()->net_log(),
+                             false,
+                             request_info->GetPageTransition(),
+                             save_info_.Pass()));
+  info->download_id = download_id_;
+  info->url_chain = request()->url_chain();
+  info->referrer_url = GURL(request()->referrer());
+  info->interrupt_reason = interrupt_reason;
+  info->remote_address = request()->GetSocketAddress().host();
+  info->request_handle = DownloadRequestHandle(AsWeakPtr(),
+                                               request_info->GetChildID(),
+                                               request_info->GetRouteID(),
+                                               request_info->GetRequestID());
+
+  scoped_ptr<ByteStreamReader> stream_reader;
+  BrowserThread::PostTask(BrowserThread::UI,
+                          FROM_HERE,
+                          base::Bind(&StartOnUIThread,
+                                     base::Passed(&info),
+                                     base::Passed(&stream_reader),

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

nit: I think it'd be clearer to cons up a null one on the fly.

[asanka, 2014/03/19 20:37:06]

Done.

+                                     started_cb_));
   started_cb_.Reset();
 }
 
 bool DownloadResourceHandler::OnWillStart(int request_id,
                                           const GURL& url,
                                           bool* defer) {
   return true;
 }
 
 bool DownloadResourceHandler::OnBeforeNetworkStart(int request_id,
@@ skipping 37 matching lines @@
     double potential_bandwidth = last_buffer_size_/seconds_since_last_read;
     RecordBandwidth(actual_bandwidth, potential_bandwidth);
   }
   last_read_time_ = now;
 
   if (!bytes_read)
     return true;
   bytes_read_ += bytes_read;
   DCHECK(read_buffer_.get());
 
+  // If this was an error response, then stream_writer_ would be NULL.
+  if (!stream_writer_) {
+    read_buffer_ = NULL;
+    return true;
+  }
+
   // Take the data ship it down the stream.  If the stream is full, pause the
   // request; the stream callback will resume it.
   if (!stream_writer_->Write(read_buffer_, bytes_read)) {
     PauseRequest();
     *defer = was_deferred_ = true;
     last_stream_pause_time_ = now;
   }
 
   read_buffer_ = NULL;  // Drop our reference.
 
@@ skipping 44 matching lines @@
     // stack cancelled the request.  There aren't that many things that
     // could do this to a download request (whose lifetime is separated from
     // the tab from which it came).  We map this to USER_CANCELLED as the
     // case we know about (system suspend because of laptop close) corresponds
     // 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.
     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_PRECONDITION_FAILED:
-        // Failed our 'If-Unmodified-Since' or 'If-Match'; see
-        // download_manager_impl.cc BeginDownload()
-        reason = DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION;
-        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;
-      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;
-    }
-  }
-
   std::string accept_ranges;
   bool has_strong_validators = false;
   if (request()->response_headers()) {
     request()->response_headers()->EnumerateHeader(
         NULL, "Accept-Ranges", &accept_ranges);
     has_strong_validators =
         request()->response_headers()->HasStrongValidators();
   }
   RecordAcceptsRanges(accept_ranges, bytes_read_, has_strong_validators);
   RecordNetworkBlockage(base::TimeTicks::Now() - download_start_time_,
                         total_pause_time_);
 
-  CallStartedCB(NULL, reason);
-
-  // Send the info down the stream.  Conditional is in case we get
-  // OnResponseCompleted without OnResponseStarted.
-  if (stream_writer_)
+  // If OnResponseCompleted() is called without a corresponding
+  // OnResponseStarted() call, then the DownloadManager wouldn't have been
+  // notified.
+  if (!download_manager_notified_ &&
+      status.status() != net::URLRequestStatus::CANCELED) {
+    DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason);
+    NotifyDownloadManagerOfFailedRequest(reason);
+  } else if (stream_writer_) {
+    // Send the info down the stream.  Conditional is in case we get
+    // OnResponseCompleted without OnResponseStarted.
     stream_writer_->Close(reason);
+    stream_writer_.reset();  // We no longer need the stream.
+  }
 
   // If the error mapped to something unknown, record it so that
   // we can drill down.
   if (reason == DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED) {
     UMA_HISTOGRAM_CUSTOM_ENUMERATION("Download.MapErrorNetworkFailed",
                                      std::abs(status.error()),
                                      net::GetAllErrorCodesForUma());
   }
 
-  stream_writer_.reset();  // We no longer need the stream.
   read_buffer_ = NULL;
 }
 
 void DownloadResourceHandler::OnDataDownloaded(
     int request_id,
     int bytes_downloaded) {
   NOTREACHED();
 }
 
 void DownloadResourceHandler::PauseRequest() {
@@ skipping 46 matching lines @@
                                 request()->url().spec().c_str() :
                                 "<NULL request>",
                             info->GetChildID(),
                             info->GetRequestID(),
                             info->GetRouteID());
 }
 
 DownloadResourceHandler::~DownloadResourceHandler() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  // This won't do anything if the callback was called before.
-  // If it goes through, it will likely be because OnWillStart() returned
-  // false somewhere in the chain of resource handlers.
-  CallStartedCB(NULL, DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED);
+  // DownloadManager would not be notified if:
+  //
+  // - The Content-Type or Content-Disposition indicated that the entity should
+  //   be downloaded, but the response code was not 2xx. BufferedResourceHandler
+  //   will create a DownloadResourceHandler, but won't invoke it.
+  //   TODO(asanka): Fix this so that ResourceHandlers are only created if they
+  //   are going to be used.
+  //
+  // - OnWillStart() returned false somewhere in the chain of resource handlers.
+  if (!download_manager_notified_ && !started_cb_.is_null())

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

DCHECK opportunity?  Shouldn't these two conditionals be in sync always?

[asanka, 2014/03/19 20:37:06]

After verifying the OnWillStart() pathway, I decided to DCHECK on started_cb_
being null instead of handling this case. The justification is in the new
comment.

+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(started_cb_,

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

Is the eventual intent to get rid of started_cb?  (Mostly asking for curiosity
and because I don't like multiple ways of exposing errors, but if it is, a
TODO() might be useful doc of the intent.)

[asanka, 2014/03/19 20:37:06]

Yeah. I'd like to get rid of started_cb_. I added a TODO for this.

+                   static_cast<DownloadItem*>(NULL),
+                   DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED));
 
   // Remove output stream callback if a stream exists.
   if (stream_writer_)
     stream_writer_->RegisterCallback(base::Closure());
 
   UMA_HISTOGRAM_TIMES("SB2.DownloadDuration",
                       base::TimeTicks::Now() - download_start_time_);
 }
 
 }  // namespace content