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

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index cef57731792e96e8832ee0c0962b58a5d0f75b8b..cb223de575ed1263ad56150508a502ed2d9433fd 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -191,9 +191,11 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bytes_per_sec_(0),
       last_modified_time_(info.last_modified),
       etag_(info.etag),
-      last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
+      last_reason_(info.result),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS_INTERNAL),
+      state_(info.result == DOWNLOAD_INTERRUPT_REASON_NONE
+                 ? IN_PROGRESS_INTERNAL
+                 : INTERRUPTED_INTERNAL),
       danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       delegate_(delegate),
@@ -204,13 +206,14 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       auto_opened_(false),
       is_temporary_(!info.save_info->file_path.empty()),
       all_data_saved_(false),
-      destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
+      destination_error_(DOWNLOAD_INTERRUPT_REASON_NONE),
       opened_(false),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
       weak_ptr_factory_(this) {
   delegate_->Attach();
-  Init(true /* actively downloading */, SRC_ACTIVE_DOWNLOAD);
+  Init(state_ == IN_PROGRESS_INTERNAL /* actively downloading */,
+       SRC_ACTIVE_DOWNLOAD);
 
   // Link the event sources.
   bound_net_log_.AddEvent(
@@ -355,6 +358,7 @@ void DownloadItemImpl::Pause() {
 
 void DownloadItemImpl::Resume() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   switch (state_) {
     case IN_PROGRESS_INTERNAL:
       if (!is_paused_)
@@ -373,6 +377,7 @@ void DownloadItemImpl::Resume() {
     case INTERRUPTED_INTERNAL:
       auto_resume_count_ = 0;  // User input resets the counter.
       ResumeInterruptedDownload();
+      UpdateObservers();
       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
@@ -925,31 +930,19 @@ DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
   return mode;
 }
 
-void DownloadItemImpl::MergeOriginInfoOnResume(
+void DownloadItemImpl::UpdateValidatorsOnResumption(
     const DownloadCreateInfo& new_create_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(RESUMING_INTERNAL, state_);
-  DCHECK(!new_create_info.url_chain.empty());
-
-  // We are going to tack on any new redirects to our list of redirects.
-  // When a download is resumed, the URL used for the resumption request is the
-  // one at the end of the previous redirect chain. Tacking additional redirects
-  // to the end of this chain ensures that:
-  // - If the download needs to be resumed again, the ETag/Last-Modified headers
-  //   will be used with the last server that sent them to us.
-  // - The redirect chain contains all the servers that were involved in this
-  //   download since the initial request, in order.
-  std::vector<GURL>::const_iterator chain_iter =
-      new_create_info.url_chain.begin();
-  if (*chain_iter == url_chain_.back())
-    ++chain_iter;
+
+  // Redirects are disallowed for a resumption request.
+  DCHECK_EQ(1u, new_create_info.url_chain.size());
+  DCHECK_EQ(url_chain_.back(), new_create_info.url_chain.back());

[Randy Smith (Not in Mondays), 2016/02/10 21:48:45]

So I could be misunderstand the code, but it seems like this assumption is
dependent on proper behavior of web servers, which I sorta think can't be a good
idea.  Specifically, IIUC, this invariant is believed true because we stored the
final URL in the histogram/DownloadItemImpl on the first attempt, and thus we
shouldn't get any redirects on the second attempt.  But I don't think there's
anything to stop the web server from sticking an extra redirect in between
attempt 1 & 2?  I could also imagine that we had, e.g., a network failure while
we were following the redirect chain, and thus only stored an intermediate URL. 
This might not be true now (i.e. it might be true that on an initial download
attempt any breakage during redirects won't result in a DownlodItemImpl) but if
we're aiming to eventually always create a DII for any download attempt, that
won't be something we can assume in the future.  Thoughts?

[asanka, 2016/02/11 03:43:07]

This is asserted by DownloadRequestCore which interrupts with .. oh wait, that's
https://codereview.chromium.org/1544603003/.

In reality, any validators we've presented only work for one hop since different
servers (or different URLs) don't necessarily honor the same validators. Also,
an unexpected redirect is very likely the result of a meddling middlebox rather
than the target server deciding not to honor the request this time around.

Rather than speculate, the CL mentioned above treats it as an error. But for
this round, I'm restoring the redirect handling since it should be dealt with on
the other CL.

 
   // Record some stats. If the precondition failed (the server returned
   // HTTP_PRECONDITION_FAILED), then the download will automatically retried as
   // a full request rather than a partial. Full restarts clobber validators.
   int origin_state = 0;
-  if (chain_iter != new_create_info.url_chain.end())
-    origin_state |= ORIGIN_STATE_ON_RESUMPTION_ADDITIONAL_REDIRECTS;
   if (etag_ != new_create_info.etag ||
       last_modified_time_ != new_create_info.last_modified)
     origin_state |= ORIGIN_STATE_ON_RESUMPTION_VALIDATORS_CHANGED;
@@ -958,8 +951,6 @@ void DownloadItemImpl::MergeOriginInfoOnResume(
   RecordOriginStateOnResumption(new_create_info.save_info->offset != 0,
                                 origin_state);
 
-  url_chain_.insert(
-      url_chain_.end(), chain_iter, new_create_info.url_chain.end());
   etag_ = new_create_info.etag;
   last_modified_time_ = new_create_info.last_modified;
   content_disposition_ = new_create_info.content_disposition;
@@ -1060,7 +1051,7 @@ void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
   if (current_path_.empty() || target_path_.empty())
     destination_error_ = reason;
   else
-    Interrupt(reason);
+    Interrupt(reason, UPDATE_OBSERVERS);
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
@@ -1111,11 +1102,10 @@ void DownloadItemImpl::Init(bool active,
 // We're starting the download.
 void DownloadItemImpl::Start(
     scoped_ptr<DownloadFile> file,
-    scoped_ptr<DownloadRequestHandleInterface> req_handle) {
+    scoped_ptr<DownloadRequestHandleInterface> req_handle,
+    const DownloadCreateInfo& new_create_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!download_file_.get());
-  DCHECK(file.get());
-  DCHECK(req_handle.get());
 
   download_file_ = std::move(file);
   request_handle_ = std::move(req_handle);
@@ -1128,6 +1118,53 @@ void DownloadItemImpl::Start(
     return;
   }
 
+  // The state could be one of the following:
+  //
+  // IN_PROGRESS_INTERNAL: Download started normally.
+  //
+  // RESUMING_INTERNAL: A resumption attempt. May or may not have been
+  //     successful.
+  //
+  // INTERRUPTED_INTERNAL: Download was DOA. (E.g. a programmatic download that
+  //     encountered a network error and didn't progress to the point where a
+  //     server response was available, or the server responded with an error).
+  DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == RESUMING_INTERNAL ||
+         state_ == INTERRUPTED_INTERNAL);
+
+  // If a resumption attempted failed, then the download goes back to being
+  // interrupted.
+  if (state_ == RESUMING_INTERNAL &&
+      new_create_info.result != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    DCHECK(!download_file_.get());
+
+    Interrupt(new_create_info.result, DONT_UPDATE_OBSERVERS);
+
+    // Transitioning to interrupted may trigger another resumption attempt. If
+    // so, Start() will be called again once a response is available for that
+    // request. Nothing more needs to be done now. Note that Start() is never
+    // called synchronously.
+    if (state_ == RESUMING_INTERNAL)
+      return;
+
+    // Otherwise, the download should now be interrupted.
+    DCHECK_EQ(INTERRUPTED_INTERNAL, state_);
+  }
+
+  if (state_ == INTERRUPTED_INTERNAL) {
+    DCHECK(!download_file_.get());
+    // If the download failed, then skip the download_file_ initialization. It
+    // isn't going to be used anyway.
+    DetermineDownloadTarget();
+    return;
+  }
+
+  // Successful download start.
+  DCHECK(download_file_.get());
+  DCHECK(request_handle_.get());
+
+  if (state_ == RESUMING_INTERNAL)
+    UpdateValidatorsOnResumption(new_create_info);
+
   TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
 
   BrowserThread::PostTask(
@@ -1143,18 +1180,22 @@ void DownloadItemImpl::OnDownloadFileInitialized(
     DownloadInterruptReason result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (result != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    Interrupt(result);
-    // TODO(rdsmith/asanka): Arguably we should show this in the UI, but
-    // it's not at all clear what to show--we haven't done filename
-    // determination, so we don't know what name to display.  OTOH,
-    // the failure mode of not showing the DI if the file initialization
-    // fails isn't a good one.  Can we hack up a name based on the
-    // URLRequest?  We'll need to make sure that initialization happens
-    // properly.  Possibly the right thing is to have the UI handle
-    // this case specially.
-    return;
+    // The download file failed to initialize.
+    Interrupt(result, DONT_UPDATE_OBSERVERS);
+
+    // The download may transition to RESUMING_INTERNAL as a result of the
+    // Interrupt() call above if it was resumed automatically. If so, then
+    // Start() is expected to be called again and the download progression
+    // cascade will be redone. Therefore we are done for now. Start() is never
+    // called synchronously.
+    if (state_ == RESUMING_INTERNAL)
+      return;
   }
 
+  DetermineDownloadTarget();

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

Random thought: Reading through this code, I'm struck by its similarity to the
type of problem DoLoop was designed to solve (basically single threaded control
flow but with places where writing the code as a single function would require
sleeping waiting for async results).  I wonder if we should consider rewriting
this in the DoLoop() idiom.  Low priority, obviously; I'm not really sure it
would aid comprehension.  But there's a certain value to solving the same
problem the same way.

[asanka, 2016/02/11 03:43:07]

This is true. I'm also tempted to pull out the download job handling part out of
DownloadItem so that only active downloads have all the guts needed to drive a
download request. But yeah, that's a CL for another day.

+}
+
+void DownloadItemImpl::DetermineDownloadTarget() {
   delegate_->DetermineDownloadTarget(
       this, base::Bind(&DownloadItemImpl::OnDownloadTargetDetermined,
                        weak_ptr_factory_.GetWeakPtr()));
@@ -1176,20 +1217,21 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
     return;
   }
 
-  // TODO(rdsmith,asanka): We are ignoring the possibility that the download
-  // has been interrupted at this point until we finish the intermediate
-  // rename and set the full path.  That's dangerous, because we might race
-  // with resumption, either manual (because the interrupt is visible to the
-  // UI) or automatic.  If we keep the "ignore an error on download until file
-  // name determination complete" semantics, we need to make sure that the
-  // error is kept completely invisible until that point.
-
   DVLOG(20) << __FUNCTION__ << " " << target_path.value() << " " << disposition
             << " " << danger_type << " " << DebugString(true);
 
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
+  if (state_ != IN_PROGRESS_INTERNAL) {
+    DCHECK(!download_file_);
+    // Already interrupted or cancelled. Since there's now a filename for
+    // display purposes, observers can be safely notified of the new [failed]
+    // download. Nothing more needs to be done since there's no active request
+    // or in-use file.

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

Possible alternative implementation: Create a new _INTERNAL state which maps to
IN_PROGRESS but the internal code knows means "fail when name is determined".

[asanka, 2016/02/11 03:43:07]

Yup. That's what I ended up doing, partly to deal with the race condition I
mentioned.

+    UpdateObservers();
+    return;
+  }
 
   // We want the intermediate and target paths to refer to the same directory so
   // that they are both on the same device and subject to same
@@ -1239,10 +1281,10 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     // argument to the Interrupt() routine, as it happened first.
     if (reason == DOWNLOAD_INTERRUPT_REASON_NONE)
       SetFullPath(full_path);
-    Interrupt(destination_error_);
+    Interrupt(destination_error_, UPDATE_OBSERVERS);
     destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
   } else if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
-    Interrupt(reason);
+    Interrupt(reason, UPDATE_OBSERVERS);
     // All file errors result in file deletion above; no need to cleanup.  The
     // current_path_ should be empty. Resuming this download will force a
     // restart and a re-doing of filename determination.
@@ -1339,7 +1381,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
             << " " << DebugString(false);
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
-    Interrupt(reason);
+    Interrupt(reason, UPDATE_OBSERVERS);
 
     // All file errors should have resulted in in file deletion above. On
     // resumption we will need to re-do filename determination.
@@ -1408,28 +1450,16 @@ void DownloadItemImpl::Completed() {
   }
 }
 
-void DownloadItemImpl::OnResumeRequestStarted(
-    DownloadItem* item,
-    DownloadInterruptReason interrupt_reason) {
-  // If |item| is not NULL, then Start() has been called already, and nothing
-  // more needs to be done here.
-  if (item) {
-    DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
-    DCHECK_EQ(static_cast<DownloadItem*>(this), item);
-    return;
-  }
-  // Otherwise, the request failed without passing through
-  // DownloadResourceHandler::OnResponseStarted.
-  DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
-  Interrupt(interrupt_reason);
-}
-
 // **** End of Download progression cascade
 
 // An error occurred somewhere.
-void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
+void DownloadItemImpl::Interrupt(
+    DownloadInterruptReason reason,
+    ShouldUpdateObservers should_update_observers) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason);
+  DVLOG(20) << __FUNCTION__
+            << "() reason=" << DownloadInterruptReasonToString(reason);
 
   // Somewhat counter-intuitively, it is possible for us to receive an
   // interrupt after we've already been interrupted.  The generation of
@@ -1484,7 +1514,8 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
     RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
 
   AutoResumeIfValid();
-  UpdateObservers();
+  if (should_update_observers == UPDATE_OBSERVERS)
+    UpdateObservers();
 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
@@ -1676,6 +1707,25 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
   if (state_ != INTERRUPTED_INTERNAL)
     return;
 
+  // We are starting a new request. Shake off all pending operations.
+  DCHECK(!download_file_);
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
+  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DownloadItemImpl::InvokeDelegateForResumption,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DownloadItemImpl::InvokeDelegateForResumption() {

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

Why replacing the sync invocation with an async invocation?

[asanka, 2016/02/11 03:43:07]

This CL relies on Start() never being called synchronously. I added the extra
async hop so that we can enforce this from within DII rather than relying on
callers doing the right thing.

However, we are no longer relying on that due to the extra internal states that
I added for the new patchset. So I've removed the extra hop.

+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // A state change happend while we were out. This could be because of a
+  // Cancel() call. Since we aren't resuming anymore, there's nothing else to
+  // do.
+  if (state_ != RESUMING_INTERNAL)
+    return;
+
   // Reset the appropriate state if restarting.
   ResumeMode mode = GetResumeMode();
   if (mode == RESUME_MODE_IMMEDIATE_RESTART ||
@@ -1700,15 +1750,10 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
   download_params->set_hash_state(GetHashState());
   download_params->set_last_modified(GetLastModifiedTime());
   download_params->set_etag(GetETag());
-  download_params->set_callback(
-      base::Bind(&DownloadItemImpl::OnResumeRequestStarted,
-                 weak_ptr_factory_.GetWeakPtr()));
 
   delegate_->ResumeInterruptedDownload(std::move(download_params), GetId());
   // Just in case we were interrupted while paused.
   is_paused_ = false;
-
-  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
 }
 
 // static