[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:
     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();
       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:
     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 28 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 IN_PROGRESS 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);
 
   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();
     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);

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

Needs an UpdateObservers() call.

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

This looks like a note you made to yourself that you didn't execute on; status?

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

Stale. We've relieved Interrupt() of the responsibility of updating observers.
Cancel() has an UpdateObservers() call that does it for us.

+    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();

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

Perhaps we should do this when the previous request / download session ended.

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

I think we do (given that Interrupt() does a ReleaseDownloadFile() if we have a
download_file_ pointer and the only way we can lose a download_file_ pointer is
through ReleaseDownloadFile()).  Are you seeing some way to have an outstanding
valid weak pointer at this point in the code?

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

This was a note to self to check. It is possible to have a pending weak
reference from a DeleteDownloadedFile call. That would happen if there was an
intermediate file after an interruption and Cancel() was called. However, such a
download wouldn't be resumable and hence we wouldn't get here.

+
   // 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:
     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