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

content/browser/download/download_resource_handler.cc

Index: content/browser/download/download_resource_handler.cc
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index ab9e19ef3df08ba3c7f79af4dce81caa8c5143c1..829884a567d060e1d6c0c0d506a767373ef9435d 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -34,20 +34,9 @@
 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) {
@@ -69,6 +58,37 @@ static void StartOnUIThread(
   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;
@@ -86,7 +106,7 @@ DownloadResourceHandler::DownloadResourceHandler(
       bytes_read_(0),
       pause_count_(0),
       was_deferred_(false),
-      on_response_started_called_(false) {
+      download_manager_notified_(false) {
   RecordDownloadCount(UNTHROTTLED_COUNT);
 }
 
@@ -113,10 +133,6 @@ bool DownloadResourceHandler::OnResponseStarted(
     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();
@@ -128,15 +144,35 @@ bool DownloadResourceHandler::OnResponseStarted(
   // 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,
@@ -145,32 +181,6 @@ bool DownloadResourceHandler::OnResponseStarted(
                              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
@@ -194,6 +204,33 @@ bool DownloadResourceHandler::OnResponseStarted(
       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,
@@ -205,21 +242,42 @@ bool DownloadResourceHandler::OnResponseStarted(
                  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();
 }
 
@@ -277,6 +335,12 @@ bool DownloadResourceHandler::OnReadCompleted(int request_id, int bytes_read,
   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)) {
@@ -341,44 +405,6 @@ void DownloadResourceHandler::OnResponseCompleted(
     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()) {
@@ -391,12 +417,19 @@ void DownloadResourceHandler::OnResponseCompleted(
   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.
@@ -406,7 +439,6 @@ void DownloadResourceHandler::OnResponseCompleted(
                                      net::GetAllErrorCodesForUma());
   }
 
-  stream_writer_.reset();  // We no longer need the stream.
   read_buffer_ = NULL;
 }
 
@@ -473,10 +505,22 @@ std::string DownloadResourceHandler::DebugString() const {
 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_)