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

content/browser/download/download_item_impl.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.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 174 matching lines @@
       has_user_gesture_(info.has_user_gesture),
       content_disposition_(info.content_disposition),
       mime_type_(info.mime_type),
       original_mime_type_(info.original_mime_type),
       remote_address_(info.remote_address),
       total_bytes_(info.total_bytes),
       received_bytes_(0),
       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_(INITIAL_INTERNAL),
       danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       delegate_(delegate),
       is_paused_(false),
       auto_resume_count_(0),
       open_when_complete_(false),
       file_externally_removed_(false),
       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);
 
   // Link the event sources.
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_URL_REQUEST,
@@ skipping 69 matching lines @@
 }
 
 void DownloadItemImpl::RemoveObserver(Observer* observer) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   observers_.RemoveObserver(observer);
 }
 
 void DownloadItemImpl::UpdateObservers() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "()";
 
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this));
 }
 
 void DownloadItemImpl::ValidateDangerousDownload() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!IsDone());
   DCHECK(IsDangerous());
 
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
 
   if (IsDone() || !IsDangerous())
     return;
 
   RecordDangerousDownloadAccept(GetDangerType(),
                                 GetTargetFilePath());
 
   danger_type_ = DOWNLOAD_DANGER_TYPE_USER_VALIDATED;
 
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED,
       base::Bind(&ItemCheckedNetLogCallback, GetDangerType()));
 
-  UpdateObservers();
+  UpdateObservers();  // TODO(asanka): This is potentially unsafe. The download
+                      // may not be in a consistent state or around at all after
+                      // invoking observers.

[Randy Smith (Not in Mondays), 2016/02/12 19:10:28]

I presume there are no observers that do anything like this currently?  Are the
observers that will be seriously confused by moving this after
MaybeCompleteDownload()?  Or does that mess up dangerous download processing?

(This seems like a big enough hole to be worth a bug and a fix sometime soon,
though not introduced in this CL, so not blocking.)

[asanka, 2016/02/12 20:52:28]

Yeah. None of the observers do anything crazy, but as I mentioned over IM,
requiring that isn't a great API since callers can't readily understand which
operations are safe and which ones are not.

I filed https://code.google.com/p/chromium/issues/detail?id=586610 to track this
and referenced it from the TODOs.

 
   MaybeCompleteDownload();
 }
 
 void DownloadItemImpl::StealDangerousDownload(
     const AcquireFileCallback& callback) {
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(IsDangerous());
 
@@ skipping 22 matching lines @@
     case INITIAL_INTERNAL:
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
     case INTERRUPTED_INTERNAL:
     case CANCELLED_INTERNAL:
     case RESUMING_INTERNAL:
       // No active request.
       return;
 
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/12 21:57:19]

Hasn't the request already failed, or doesn't exist, in this state?

[asanka, 2016/02/12 23:34:47]

True. Moved to the no-op block above.

     case IN_PROGRESS_INTERNAL:
       request_handle_->PauseRequest();
       is_paused_ = true;
       UpdateObservers();
       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
     case TARGET_RESOLVED_INTERNAL:
       NOTREACHED();
   }
 }
 
 void DownloadItemImpl::Resume() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   switch (state_) {
     case INITIAL_INTERNAL:
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
-    case CANCELLED_INTERNAL:
-    // Nothing to resume.
-    case RESUMING_INTERNAL:
-      // Resumption in progress.
-      DCHECK(!is_paused_);
+    case CANCELLED_INTERNAL:  // Nothing to resume.
+    case RESUMING_INTERNAL:   // Resumption in progress.
       return;
 
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/12 21:57:19]

Same as above.

[asanka, 2016/02/12 23:34:47]

Same.

     case IN_PROGRESS_INTERNAL:
       if (!is_paused_)
         return;
       request_handle_->ResumeRequest();
       is_paused_ = false;
       UpdateObservers();
       return;
 
     case INTERRUPTED_INTERNAL:
       auto_resume_count_ = 0;  // User input resets the counter.
       ResumeInterruptedDownload();
+      UpdateObservers();
       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
     case TARGET_RESOLVED_INTERNAL:
       NOTREACHED();
   }
 }
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   Interrupt(user_cancel ? DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
                         : DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::Remove() {
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   delegate_->AssertStateConsistent(this);
   Interrupt(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED);
+  UpdateObservers();
   delegate_->AssertStateConsistent(this);
 
   NotifyRemoved();
   delegate_->DownloadRemoved(this);
   // We have now been deleted.
 }
 
 void DownloadItemImpl::OpenDownload() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ skipping 46 matching lines @@
 }
 
 bool DownloadItemImpl::CanResume() const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   switch (state_) {
     case INITIAL_INTERNAL:
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
     case CANCELLED_INTERNAL:
     case RESUMING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
       return false;
 
     case TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       return is_paused_;
 
     case INTERRUPTED_INTERNAL: {
       ResumeMode resume_mode = GetResumeMode();
       // Only allow Resume() calls if the resumption mode requires a user
       // action.
       return IsDownloadResumptionEnabled() &&
              (resume_mode == RESUME_MODE_USER_RESTART ||
               resume_mode == RESUME_MODE_USER_CONTINUE);
     }
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       NOTREACHED();
   }
   return false;
 }
 
 bool DownloadItemImpl::IsDone() const {
   switch (state_) {
     case INITIAL_INTERNAL:
     case COMPLETING_INTERNAL:
     case RESUMING_INTERNAL:
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       return false;
 
     case COMPLETE_INTERNAL:
     case CANCELLED_INTERNAL:
       return true;
 
     case INTERRUPTED_INTERNAL:
       return !CanResume();
@@ skipping 404 matching lines @@
     case DOWNLOAD_INTERRUPT_REASON_SERVER_UNAUTHORIZED:
     case DOWNLOAD_INTERRUPT_REASON_SERVER_CERT_PROBLEM:
     case DOWNLOAD_INTERRUPT_REASON_SERVER_FORBIDDEN:
       mode = RESUME_MODE_INVALID;
       break;
   }
 
   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
@@ skipping 64 matching lines @@
   hash_state_ = "";
 
   UpdateObservers();
 }
 
 void DownloadItemImpl::MarkAsComplete() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS);
+  TransitionTo(COMPLETE_INTERNAL);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationUpdate(int64_t bytes_so_far,
                                          int64_t bytes_per_sec,
                                          const std::string& hash_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // If the download is in any other state we don't expect any
   // DownloadDestinationObserver callbacks. An interruption or a cancellation
   // results in a call to ReleaseDownloadFile which invalidates the weak
   // reference held by the DownloadFile and hence cuts off any pending
@@ skipping 28 matching lines @@
   // results in a call to ReleaseDownloadFile which invalidates the weak
   // reference held by the DownloadFile and hence cuts off any pending
   // callbacks.
   DCHECK(state_ == TARGET_PENDING_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
   DVLOG(20) << __FUNCTION__
             << "() reason:" << DownloadInterruptReasonToString(reason);
 
   // Postpone recognition of this error until after file name determination
   // has completed and the intermediate file has been renamed to simplify
   // resumption conditions.
-  if (state_ != IN_PROGRESS_INTERNAL)
+  if (state_ == TARGET_PENDING_INTERNAL) {
     destination_error_ = reason;
-  else
-    Interrupt(reason);
+    return;
+  }
+  Interrupt(reason);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // If the download is in any other state we don't expect any
   // DownloadDestinationObserver callbacks. An interruption or a cancellation
   // results in a call to ReleaseDownloadFile which invalidates the weak
   // reference held by the DownloadFile and hence cuts off any pending
   // callbacks.
   DCHECK(state_ == TARGET_PENDING_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
@@ skipping 36 matching lines @@
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, active_data);
   }
 
   DVLOG(20) << __FUNCTION__ << "() " << DebugString(true);
 }
 
 // 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());
   DVLOG(20) << __FUNCTION__ << "() this=" << DebugString(true);
 
   download_file_ = std::move(file);
   request_handle_ = std::move(req_handle);
 
   if (state_ == CANCELLED_INTERNAL) {
     // The download was in the process of resuming when it was cancelled. Don't
     // proceed.
     ReleaseDownloadFile(true);
-    request_handle_->CancelRequest();
+    if (request_handle_)
+      request_handle_->CancelRequest();
     return;
   }
 
-  TransitionTo(TARGET_PENDING_INTERNAL, UPDATE_OBSERVERS);
+  // The state could be one of the following:
+  //
+  // INITIAL_INTERNAL: A normal download attempt.
+  //
+  // RESUMING_INTERNAL: A resumption attempt. May or may not have been
+  //     successful.
+  DCHECK(state_ == INITIAL_INTERNAL || state_ == RESUMING_INTERNAL);
+
+  // If a resumption attempted failed, or if the download was DOA, then the
+  // download should go back to being interrupted.
+  if (new_create_info.result != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    DCHECK(!download_file_.get());
+
+    // For downloads that are interrupted on initiation, or which don't have a
+    // target, invoke target determination so that we'll end up with a usable
+    // filename.

[Randy Smith (Not in Mondays), 2016/02/12 21:57:19]

nit, suggestion: I had some trouble matching this comment to the test below; I
think my confusion was "interrupted on initiation" not including "interrupted on
resumption".  Maybe add "... initiation, or interrupted on resumption and don't
have a target ..." to make it clear that interrupted on initiation refers to the
global initiation, not this particular request initiation?

[asanka, 2016/02/12 23:34:47]

Ah. I simplified the condition and the comment to just use target_path_.empty().
If the download state is INITIAL_INTERNAL, then target_path_ will be empty.

+    if (state_ == INITIAL_INTERNAL || target_path_.empty()) {
+      TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+      destination_error_ = new_create_info.result;
+      DetermineDownloadTarget();
+      return;
+    }
+
+    // Otherwise, this was a resumption attempt which ended with an
+    // interruption. Continue with current target path.
+    TransitionTo(TARGET_RESOLVED_INTERNAL);
+    Interrupt(new_create_info.result);
+    UpdateObservers();
+    return;
+  }
+
+  // Successful download start.
+  DCHECK(download_file_.get());
+  DCHECK(request_handle_.get());
+
+  if (state_ == RESUMING_INTERNAL)
+    UpdateValidatorsOnResumption(new_create_info);
+
+  TransitionTo(TARGET_PENDING_INTERNAL);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFile::Initialize,
                  // Safe because we control download file lifetime.
                  base::Unretained(download_file_.get()),
                  base::Bind(&DownloadItemImpl::OnDownloadFileInitialized,
                             weak_ptr_factory_.GetWeakPtr())));
 }
 
 void DownloadItemImpl::OnDownloadFileInitialized(
     DownloadInterruptReason result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
   DVLOG(20) << __FUNCTION__
             << "() result:" << DownloadInterruptReasonToString(result);
   if (result != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    // Transition out to TARGET_RESOLVED_INTERNAL since this DownloadItem is
-    // skipping the download target determination process.
-    TransitionTo(TARGET_RESOLVED_INTERNAL, DONT_UPDATE_OBSERVERS);
-    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;
+    // Whoops. That didn't work. Proceed as an interrupted download.
+    destination_error_ = result;
+    TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
   }
 
+  DetermineDownloadTarget();
+}
+
+void DownloadItemImpl::DetermineDownloadTarget() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "() " << DebugString(true);
+
   delegate_->DetermineDownloadTarget(
       this, base::Bind(&DownloadItemImpl::OnDownloadTargetDetermined,
                        weak_ptr_factory_.GetWeakPtr()));
 }
 
-// Called by delegate_ when the download target path has been
-// determined.
+// Called by delegate_ when the download target path has been determined.
 void DownloadItemImpl::OnDownloadTargetDetermined(
     const base::FilePath& target_path,
     TargetDisposition disposition,
     DownloadDangerType danger_type,
     const base::FilePath& intermediate_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
+  DCHECK(state_ == TARGET_PENDING_INTERNAL ||
+         state_ == INTERRUPTED_TARGET_PENDING_INTERNAL);
 
   // If the |target_path| is empty, then we consider this download to be
   // canceled.
   if (target_path.empty()) {
     Cancel(true);
     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);
+  DVLOG(20) << __FUNCTION__ << "() target_path:" << target_path.value()
+            << " disposition:" << disposition << " danger_type:" << danger_type
+            << " this:" << DebugString(true);
 
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
 
+  // This was an interrupted download that was looking for a filename. Now that
+  // it has one, transition to interrupted.
+  if (state_ == INTERRUPTED_TARGET_PENDING_INTERNAL) {
+    Interrupt(destination_error_);
+    destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
+    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
   // space/permission/availability constraints.
   DCHECK(intermediate_path.DirName() == target_path.DirName());
 
   // During resumption, we may choose to proceed with the same intermediate
   // file. No rename is necessary if our intermediate file already has the
   // correct name.
   //
   // The intermediate name may change from its original value during filename
@@ skipping 22 matching lines @@
                  base::Unretained(download_file_.get()),
                  intermediate_path, callback));
 }
 
 void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     DownloadInterruptReason reason,
     const base::FilePath& full_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
-  TransitionTo(TARGET_RESOLVED_INTERNAL, DONT_UPDATE_OBSERVERS);
+  TransitionTo(TARGET_RESOLVED_INTERNAL);
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != destination_error_) {
     // Process destination error.  If both |reason| and |destination_error_|
     // refer to actual errors, we want to use the |destination_error_| as the
     // argument to the Interrupt() routine, as it happened first.
     if (reason == DOWNLOAD_INTERRUPT_REASON_NONE)
       SetFullPath(full_path);
     Interrupt(destination_error_);
     destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
+    UpdateObservers();
   } else if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
     // 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.
     DCHECK(current_path_.empty());
+    UpdateObservers();
   } else {
     SetFullPath(full_path);
-    TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
+    TransitionTo(IN_PROGRESS_INTERNAL);
+    UpdateObservers();  // TODO(asanka): This is not safe. The download could be
+                        // in an underminate state after invoking observers.

[Randy Smith (Not in Mondays), 2016/02/12 19:10:28]

Unlike the other place where you have the same comment, this one concerns me for
this review, because this is a semantic change for this CL that increases
danger.  I'd like to have some assurance that this problem isn't an actual
problem before stamp.

[asanka, 2016/02/12 19:30:35]

Semantics are the same? I.e. both before and after the change, DII ends up
calling UpdateObserver() prior to MaybeCompleteDownload().

     MaybeCompleteDownload();
   }
 }
 
 // When SavePackage downloads MHTML to GData (see
 // SavePackageFilePickerChromeOS), GData calls MaybeCompleteDownload() like it
 // does for non-SavePackage downloads, but SavePackage downloads never satisfy
 // IsDownloadReadyForCompletion(). GDataDownloadObserver manually calls
 // DownloadItem::UpdateObservers() when the upload completes so that SavePackage
 // notices that the upload has completed and runs its normal Finish() pathway.
@@ skipping 27 matching lines @@
     return;
 
   DVLOG(20) << __FUNCTION__ << "()"
             << " " << DebugString(true);
   DCHECK(!GetTargetFilePath().empty());
   DCHECK(!IsDangerous());
 
   // TODO(rdsmith/benjhayden): Remove as part of SavePackage integration.
   if (is_save_package_download_) {
     // Avoid doing anything on the file thread; there's nothing we control
-    // there.
-    // Strictly speaking, this skips giving the embedder a chance to open
-    // the download.  But on a save package download, there's no real
+    // there.  Strictly speaking, this skips giving the embedder a chance to
+    // open the download.  But on a save package download, there's no real
     // concept of opening.
     Completed();
     return;
   }
 
   DCHECK(download_file_.get());
   // Unilaterally rename; even if it already has the right name,
   // we need theannotation.
   DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
@@ skipping 20 matching lines @@
   DVLOG(20) << __FUNCTION__ << "()"
             << " full_path = \"" << full_path.value() << "\""
             << " " << DebugString(false);
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
 
     // All file errors should have resulted in in file deletion above. On
     // resumption we will need to re-do filename determination.
     DCHECK(current_path_.empty());
+    UpdateObservers();
     return;
   }
 
   DCHECK(target_path_ == full_path);
 
   if (full_path != current_path_) {
     // full_path is now the current and target file path.
     DCHECK(!full_path.empty());
     SetFullPath(full_path);
   }
 
   // Complete the download and release the DownloadFile.
   DCHECK(download_file_);
   ReleaseDownloadFile(false);
 
   // We're not completely done with the download item yet, but at this
   // point we're committed to complete the download.  Cancels (or Interrupts,
   // though it's not clear how they could happen) after this point will be
   // ignored.
-  TransitionTo(COMPLETING_INTERNAL, DONT_UPDATE_OBSERVERS);
+  TransitionTo(COMPLETING_INTERNAL);
 
   if (delegate_->ShouldOpenDownload(
           this, base::Bind(&DownloadItemImpl::DelayedDownloadOpened,
                            weak_ptr_factory_.GetWeakPtr()))) {
     Completed();
   } else {
     delegate_delayed_complete_ = true;
     UpdateObservers();
   }
 }
 
 void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   auto_opened_ = auto_opened;
   Completed();
 }
 
 void DownloadItemImpl::Completed() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   DVLOG(20) << __FUNCTION__ << "() " << DebugString(false);
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS);
+  TransitionTo(COMPLETE_INTERNAL);
   RecordDownloadCompleted(start_tick_, received_bytes_);
 
   if (auto_opened_) {
     // If it was already handled by the delegate, do nothing.
   } else if (GetOpenWhenComplete() ||
              ShouldOpenFileBasedOnExtension() ||
              IsTemporary()) {
     // If the download is temporary, like in drag-and-drop, do not open it but
     // we still need to set it auto-opened so that it can be removed from the
     // download shelf.
     if (!IsTemporary())
       OpenDownload();
 
     auto_opened_ = true;
-    UpdateObservers();
   }
-}
-
-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);
+  UpdateObservers();
 }
 
 // **** End of Download progression cascade
 
 // An error occurred somewhere.
 void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason);
   DVLOG(20) << __FUNCTION__
             << "() reason:" << DownloadInterruptReasonToString(reason)
@@ skipping 14 matching lines @@
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
       // Already complete.
       return;
 
     case INITIAL_INTERNAL:
     case MAX_DOWNLOAD_INTERNAL_STATE:
       NOTREACHED();
       return;
 
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
+    case IN_PROGRESS_INTERNAL:
     case TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/12 21:57:19]

I thought we couldn't interrupt from TARGET_PENDING_INTERNAL?  (If that's right,
sorry I didn't catch it on the other CL.)

[asanka, 2016/02/12 23:34:47]

It's not allowed. But we can transition to CANCELLED from TARGET_PENDING which
comes down the same path. This switch is doing state specific prep before we
attempt the state transition below. TransitionTo will ensure that the only
transition allowed is TARGET_PENDING -> CANCEL.

     case TARGET_RESOLVED_INTERNAL:
-    case IN_PROGRESS_INTERNAL:
       // last_reason_ needs to be set for GetResumeMode() to work.
       last_reason_ = reason;
 
       if (download_file_) {
         ResumeMode resume_mode = GetResumeMode();
         ReleaseDownloadFile(resume_mode != RESUME_MODE_IMMEDIATE_CONTINUE &&
                             resume_mode != RESUME_MODE_USER_CONTINUE);
       }
       break;
 
@@ skipping 37 matching lines @@
       reason == DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN) {
     if (IsDangerous()) {
       RecordDangerousDownloadDiscard(
           reason == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
               ? DOWNLOAD_DISCARD_DUE_TO_USER_ACTION
               : DOWNLOAD_DISCARD_DUE_TO_SHUTDOWN,
           GetDangerType(), GetTargetFilePath());
     }
 
     RecordDownloadCount(CANCELLED_COUNT);
-    TransitionTo(CANCELLED_INTERNAL, DONT_UPDATE_OBSERVERS);
-  } else {
-    RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
-    if (!GetWebContents())
-      RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
-    TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS);
-    AutoResumeIfValid();
+    TransitionTo(CANCELLED_INTERNAL);
+    return;
   }
 
-  UpdateObservers();
+  RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
+  if (!GetWebContents())
+    RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
+
+  TransitionTo(INTERRUPTED_INTERNAL);
+  AutoResumeIfValid();
 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DVLOG(20) << __FUNCTION__ << "() destroy_file:" << destroy_file;
 
   if (destroy_file) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         // Will be deleted at end of task execution.
@@ skipping 25 matching lines @@
   // If we don't have all the data, the download is not ready for
   // completion.
   if (!AllDataSaved())
     return false;
 
   // If the download is dangerous, but not yet validated, it's not ready for
   // completion.
   if (IsDangerous())
     return false;
 
-  // Invariants for the IN_PROGRESS state. DCHECKs here verify that the
-  // invariants are still true.
+  // Check for consistency before invoking delegate. Since there are no pending
+  // target determination calls and the download is in progress, both the target
+  // and current paths should be non-empty and they should point to the same
+  // directory.
   DCHECK(!target_path_.empty());
   DCHECK(!current_path_.empty());
   DCHECK(target_path_.DirName() == current_path_.DirName());
 
   // Give the delegate a chance to hold up a stop sign.  It'll call
   // use back through the passed callback if it does and that state changes.
   if (!delegate_->ShouldCompleteDownload(this, state_change_notification))
     return false;
 
   return true;
 }
 
-void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
-                                    ShouldUpdateObservers notify_action) {
+void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (state_ == new_state)
     return;
 
   DownloadInternalState old_state = state_;
   state_ = new_state;
 
   DCHECK(is_save_package_download_
              ? IsValidSavePackageStateTransition(old_state, new_state)
              : IsValidStateTransition(old_state, new_state))
       << "Invalid state transition from:" << DebugDownloadStateString(old_state)
       << " to:" << DebugDownloadStateString(new_state);
 
   switch (state_) {
     case INITIAL_INTERNAL:
       NOTREACHED();
       break;
 
     case TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
       break;
 
     case IN_PROGRESS_INTERNAL:
       DCHECK(!current_path_.empty()) << "Current output path must be known.";
       DCHECK(!target_path_.empty()) << "Target path must be known.";
       DCHECK(current_path_.DirName() == target_path_.DirName())
           << "Current output directory must match target directory.";
       DCHECK(download_file_) << "Output file must be owned by download item.";
       DCHECK(request_handle_) << "Download source must be active.";
       DCHECK(!is_paused_) << "At the time a download enters IN_PROGRESS state, "
@@ skipping 61 matching lines @@
     bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
 
   // Resumption
   if (was_done && !is_done) {
     std::string file_name(target_path_.BaseName().AsUTF8Unsafe());
     bound_net_log_.BeginEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE,
                               base::Bind(&ItemActivatedNetLogCallback,
                                          this, SRC_ACTIVE_DOWNLOAD,
                                          &file_name));
   }
-
-  if (notify_action == UPDATE_OBSERVERS)
-    UpdateObservers();
 }
 
 void DownloadItemImpl::SetDangerType(DownloadDangerType danger_type) {
   if (danger_type != danger_type_) {
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED,
         base::Bind(&ItemCheckedNetLogCallback, danger_type));
   }
   // Only record the Malicious UMA stat if it's going from {not malicious} ->
   // {malicious}.
@@ skipping 41 matching lines @@
 
 void DownloadItemImpl::ResumeInterruptedDownload() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!IsDownloadResumptionEnabled())
     return;
 
   // If we're not interrupted, ignore the request; our caller is drunk.
   if (state_ != INTERRUPTED_INTERNAL)
     return;
 
+  // We are starting a new request. Shake off all pending operations.
+  DCHECK(!download_file_);
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
   // Reset the appropriate state if restarting.
   ResumeMode mode = GetResumeMode();
   if (mode == RESUME_MODE_IMMEDIATE_RESTART ||
       mode == RESUME_MODE_USER_RESTART) {
     received_bytes_ = 0;
     hash_state_ = "";
     last_modified_time_ = "";
     etag_ = "";
   }
 
-  scoped_ptr<DownloadUrlParameters> download_params;
-  if (GetWebContents()) {
-    download_params =
-        DownloadUrlParameters::FromWebContents(GetWebContents(), GetURL());
-  } else {
-    download_params = make_scoped_ptr(new DownloadUrlParameters(
-        GetURL(), -1, -1, -1, GetBrowserContext()->GetResourceContext()));
-  }
-
+  // Avoid using the WebContents even if it's still around. Resumption requests
+  // are consistently routed through the no-renderer code paths so that the
+  // request will not be dropped if the WebContents (and by extension, the
+  // associated renderer) goes away before a response is received.
+  scoped_ptr<DownloadUrlParameters> download_params(new DownloadUrlParameters(
+      GetURL(), -1, -1, -1, GetBrowserContext()->GetResourceContext()));
   download_params->set_file_path(GetFullPath());
   download_params->set_offset(GetReceivedBytes());
   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()));
 
-  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
+  TransitionTo(RESUMING_INTERNAL);
   delegate_->ResumeInterruptedDownload(std::move(download_params), GetId());
   // Just in case we were interrupted while paused.
   is_paused_ = false;
 }
 
 // static
 DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
     DownloadInternalState internal_state) {
   switch (internal_state) {
     case INITIAL_INTERNAL:
     case TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     // TODO(asanka): Introduce an externally visible state to distinguish
     // between the above states and IN_PROGRESS_INTERNAL. The latter (the
     // state where the download is active and has a known target) is the state
     // that most external users are interested in.
     case IN_PROGRESS_INTERNAL:
       return IN_PROGRESS;
     case COMPLETING_INTERNAL:
       return IN_PROGRESS;
     case COMPLETE_INTERNAL:
       return COMPLETE;
@@ skipping 30 matching lines @@
 }
 
 // static
 bool DownloadItemImpl::IsValidSavePackageStateTransition(
     DownloadInternalState from,
     DownloadInternalState to) {
 #if DCHECK_IS_ON()
   switch (from) {
     case INITIAL_INTERNAL:
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
     case INTERRUPTED_INTERNAL:
     case RESUMING_INTERNAL:
     case CANCELLED_INTERNAL:
       return false;
 
     case IN_PROGRESS_INTERNAL:
       return to == CANCELLED_INTERNAL || to == COMPLETE_INTERNAL;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       NOTREACHED();
   }
   return false;
 #else
   return true;
 #endif
 }
 
 // static
 bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from,
                                               DownloadInternalState to) {
 #if DCHECK_IS_ON()
   switch (from) {
     case INITIAL_INTERNAL:
-      return to == TARGET_PENDING_INTERNAL || to == INTERRUPTED_INTERNAL;
+      return to == TARGET_PENDING_INTERNAL ||
+             to == INTERRUPTED_TARGET_PENDING_INTERNAL;
 
     case TARGET_PENDING_INTERNAL:
-      return to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL;
+      return to == INTERRUPTED_TARGET_PENDING_INTERNAL ||
+             to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL;
+
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
+      return to == INTERRUPTED_INTERNAL || to == CANCELLED_INTERNAL;
 
     case TARGET_RESOLVED_INTERNAL:
       return to == IN_PROGRESS_INTERNAL || to == INTERRUPTED_INTERNAL ||
              to == CANCELLED_INTERNAL;
 
     case IN_PROGRESS_INTERNAL:
       return to == COMPLETING_INTERNAL || to == CANCELLED_INTERNAL ||
              to == INTERRUPTED_INTERNAL;
 
     case COMPLETING_INTERNAL:
       return to == COMPLETE_INTERNAL;
 
     case COMPLETE_INTERNAL:
       return false;
 
     case INTERRUPTED_INTERNAL:
       return to == RESUMING_INTERNAL || to == CANCELLED_INTERNAL;
 
     case RESUMING_INTERNAL:
-      return to == TARGET_PENDING_INTERNAL || to == CANCELLED_INTERNAL;
+      return to == TARGET_PENDING_INTERNAL ||
+             to == INTERRUPTED_TARGET_PENDING_INTERNAL ||
+             to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL;
 
     case CANCELLED_INTERNAL:
       return false;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       NOTREACHED();
   }
   return false;
 #else
   return true;
 #endif  // DCHECK_IS_ON()
 }
 
 const char* DownloadItemImpl::DebugDownloadStateString(
     DownloadInternalState state) {
   switch (state) {
     case INITIAL_INTERNAL:
       return "INITIAL";
     case TARGET_PENDING_INTERNAL:
       return "TARGET_PENDING";
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
+      return "INTERRUPTED_TARGET_PENDING";
     case TARGET_RESOLVED_INTERNAL:
       return "TARGET_RESOLVED";
     case IN_PROGRESS_INTERNAL:
       return "IN_PROGRESS";
     case COMPLETING_INTERNAL:
       return "COMPLETING";
     case COMPLETE_INTERNAL:
       return "COMPLETE";
     case CANCELLED_INTERNAL:
       return "CANCELLED";
@@ skipping 19 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content