[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 135 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:
     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       request_handle_->PauseRequest();
       is_paused_ = true;
       UpdateObservers();
       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       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.
       return;
 
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

This also seems incompatible with mapping to an external INTERRUPTED state; the
external expectation is almost certainly that a Resume() call on the object will
actually result in an resumption.

[asanka, 2016/02/11 23:57:25]

It's worse than that, we can't trigger a resumption while a target determination
is pending. I've updated CanResume() to reflect this, but we should allow
resumption from this state, at least by resuming when target determination is
complete. But not for this CL since the state machine changes are complicated
enough as it is.

[asanka, 2016/02/12 16:06:49]

Fine! We'll go with mapping this to IN_PROGRESS :P

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

:-}.

     case TARGET_RESOLVED_INTERNAL:
     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();

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

See comment elsewhere about registered observers vs. polling consumers (though
I'm not certain what the right choice for this CL is, just calling out that it's
an issue I'm curious as to your thoughts about WRT this change).

[asanka, 2016/02/11 23:57:25]

Responded there. Basically, UpdateObservers is best called just before we exit
the state machine and yield to external signals. We need to be careful about the
state we leave the DII when we make async calls since then someone can examine
the DII. Even then, as long as the exposed state is consistent, I'm not
concerned.

       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       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);
   delegate_->AssertStateConsistent(this);
 
@@ skipping 57 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   switch (state_) {
     case INITIAL_INTERNAL:
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
     case CANCELLED_INTERNAL:
     case RESUMING_INTERNAL:
       return false;
 
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_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:

[Randy Smith (Not in Mondays), 2016/02/11 22:08:09]

I'm beginning to think that the mapping of this to INTERRUPTED rather than
IN_PROGRESS in InternalToExternalState was a typo .... :-}.

[asanka, 2016/02/11 23:57:25]

?

[asanka, 2016/02/12 16:06:49]

NM

     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 66 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);
   DVLOG(20) << __FUNCTION__ << " so_far=" << bytes_so_far
             << " per_sec=" << bytes_per_sec << " download="
             << DebugString(true);
 
@@ skipping 29 matching lines @@
 
 void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
   DCHECK(state_ == TARGET_PENDING_INTERNAL ||
          state_ == TARGET_RESOLVED_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_ != IN_PROGRESS_INTERNAL) {
     destination_error_ = reason;
-  else
-    Interrupt(reason);
+    return;
+  }
+  Interrupt(reason);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
   OnAllDataSaved(final_hash);
   MaybeCompleteDownload();
 }
 
 // **** Download progression cascade
 
@@ skipping 28 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, invoke target
+    // determination so that we'll end up with a usable filename.
+    if (state_ == INITIAL_INTERNAL) {
+      TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+      destination_error_ = new_create_info.result;
+      DetermineDownloadTarget();
+      return;
+    }
+
+    // Otherwise, this was a resumption attempt which ended with an
+    // interruption.
+    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);
+  UpdateObservers();
 
   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. Update the
+    // observers since this is an externally visible transition.
+    destination_error_ = result;
+    TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+    UpdateObservers();
   }
 
+  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);
 
   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_);
+    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);

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

It looks to me as if this state isn't needed; the download item is never in it
when any other code can run on the UI thread.  Am I wrong?

[asanka, 2016/02/11 23:57:25]

I've tried to justify the existence of this state in download_item_impl.h. It's
needed since in its absence we'd be transitioning from TARGET_PENDING_INTERNAL
directly to INTERRUPTED. If that transition is allowed, then we lose the
advantage of having the PENDING state.

 
   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;
   } 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());
   } else {
     SetFullPath(full_path);
-    TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
+    TransitionTo(IN_PROGRESS_INTERNAL);
     MaybeCompleteDownload();
   }
+  UpdateObservers();
 }
 
 // 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.
 // MaybeCompleteDownload() is never the mechanism by which SavePackage completes
 // downloads. SavePackage always uses its own Finish() to mark downloads
@@ skipping 25 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();

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

nit, suggestion: I'd rather have these right next to the state transition
functions (Interrupt in this case) so it's clear what's being broadcast to the
observers.

[asanka, 2016/02/11 23:57:25]

Iv'e tried to move the UpdateObservers() calls as close to an exit as possible
because the state of the DII after yielding to observers is undefined.

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

SGTM.

     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);
+  UpdateObservers();
   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);
-}
-
 // **** 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)
             << " this=" << DebugString(true);
 
@@ skipping 13 matching lines @@
     case COMPLETE_INTERNAL:
       // Already complete.
       return;
 
     case TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       break;
 
     case RESUMING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     case INTERRUPTED_INTERNAL:
       // The first non-cancel interrupt reason wins in cases where multiple
       // things go wrong.
       if (reason != DOWNLOAD_INTERRUPT_REASON_USER_CANCELED &&
           reason != DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN)
         return;
 
       if (!current_path_.empty()) {
         BrowserThread::PostTask(
             BrowserThread::FILE, FROM_HERE,
@@ skipping 34 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 62 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();

[Randy Smith (Not in Mondays), 2016/02/11 22:08:09]

Why doesn't this solve the race that the earlier CL was trying to fix? 

[asanka, 2016/02/11 23:57:25]

The race was that a DestinationError() or DestinationCompleted() call could come
in after we start target determination. The destination observer and the target
determination can both be using weak pointers from the same session. However,
when the call comes in, we don't have a way to tell that there's a target
determination pending. The former logic was to examine the target path, but this
isn't reliable for a resumption.

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

Right, sorry, still confused.  My image was that the other, smaller CL took care
of the race by making states finer grained and examining them.  But it looks to
me like this would also fix the race.  The DE() or DC() that comes in after
starting target determination would need to have been from a previous pre-resume
portion of the DIIs lifetime, wouldn't it?

[asanka, 2016/02/12 18:31:47]

It could be the same. The workflow is:

  - initialize download file.                   ----- [1]
  - initialize download file completes.

      [ download observer stuff happens ]

  - delegate starts download target determination

      [ download observer stuff happens ]

  - delegate determines download target
  - start intermediate rename

      [ download observer stuff happens ]

  - end intermediate rename                     ----- [2]

The destination observation calls can start coming through after [1]. [2] is
when we officially conclude target determination, and the first point where
successful new downloads have both a target and a current path.

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

Ah, got it.  Thanks!

+
   // 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()));
   }
 
   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:
     // 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;
     case CANCELLED_INTERNAL:
       return CANCELLED;
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/11 22:08:09]

Doesn't mapping this to interrupted leave you open to the consumer calling
ResumeInterruptedDownload() and being ignored?  That the behavior you want?

[asanka, 2016/02/11 23:57:25]

Resume() already doesn't work on an interrupted download that return false for
CanResume(). E.g. not HTTP, and in the future, not a supported HTTP verb. The
subtlety I'm introducing is that a download could sometimes change its
CanResume() value after entering an INTERRUPTED state. Currently this is
correctly handled by the UI because we update the observers when this happens
and the UI only looks at the CanResume() flag.

[asanka, 2016/02/12 16:06:49]

Retracted.

     case INTERRUPTED_INTERNAL:
       return INTERRUPTED;
     case RESUMING_INTERNAL:
       return IN_PROGRESS;
     case MAX_DOWNLOAD_INTERNAL_STATE:
       break;
   }
   NOTREACHED();
   return MAX_DOWNLOAD_STATE;
 }
@@ skipping 18 matching lines @@
 }
 
 #if DCHECK_IS_ON()
 // static
 bool DownloadItemImpl::IsValidSavePackageStateTransition(
     DownloadInternalState from,
     DownloadInternalState to) {
   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 || COMPLETE_INTERNAL;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       NOTREACHED();
   }
   return false;
 }
 
 // static
 bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from,
                                               DownloadInternalState to) {
   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 == CANCELLED_INTERNAL;
 
     case CANCELLED_INTERNAL:
       return false;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       NOTREACHED();
   }
   return false;
 }
 #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