[Downloads] Enforce state transition integrity and state invariants.

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index cef57731792e96e8832ee0c0962b58a5d0f75b8b..d1482bc95fd9dbf182413da766c570e934b32dd3 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -160,7 +160,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bound_net_log_(bound_net_log),
       weak_ptr_factory_(this) {
   delegate_->Attach();
-  DCHECK_NE(IN_PROGRESS_INTERNAL, state_);
+  DCHECK(state_ == COMPLETE_INTERNAL || state_ == INTERRUPTED_INTERNAL ||
+         state_ == CANCELLED_INTERNAL);
   Init(false /* not actively downloading */, SRC_HISTORY_IMPORT);
 }
 
@@ -193,7 +194,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       etag_(info.etag),
       last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS_INTERNAL),
+      state_(INITIAL_INTERNAL),
       danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       delegate_(delegate),
@@ -327,6 +328,7 @@ void DownloadItemImpl::StealDangerousDownload(
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(IsDangerous());
+
   if (download_file_) {
     BrowserThread::PostTaskAndReplyWithResult(
         BrowserThread::FILE,
@@ -345,17 +347,46 @@ void DownloadItemImpl::Pause() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // Ignore irrelevant states.
-  if (state_ != IN_PROGRESS_INTERNAL || is_paused_)
+  if (is_paused_)
     return;
 
-  request_handle_->PauseRequest();
-  is_paused_ = true;
-  UpdateObservers();
+  switch (state_) {
+    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 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);
   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 TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       if (!is_paused_)
         return;
@@ -364,12 +395,6 @@ void DownloadItemImpl::Resume() {
       UpdateObservers();
       return;
 
-    case COMPLETING_INTERNAL:
-    case COMPLETE_INTERNAL:
-    case CANCELLED_INTERNAL:
-    case RESUMING_INTERNAL:
-      return;
-
     case INTERRUPTED_INTERNAL:
       auto_resume_count_ = 0;  // User input resets the counter.
       ResumeInterruptedDownload();
@@ -382,52 +407,9 @@ void DownloadItemImpl::Resume() {
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  if (state_ != IN_PROGRESS_INTERNAL &&
-      state_ != INTERRUPTED_INTERNAL &&
-      state_ != RESUMING_INTERNAL) {
-    // Small downloads might be complete before this method has a chance to run.
-    return;
-  }
-
-  if (IsDangerous()) {
-    RecordDangerousDownloadDiscard(
-        user_cancel ? DOWNLOAD_DISCARD_DUE_TO_USER_ACTION
-                    : DOWNLOAD_DISCARD_DUE_TO_SHUTDOWN,
-        GetDangerType(),
-        GetTargetFilePath());
-  }
-
-  last_reason_ = user_cancel ? DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
-                             : DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
-
-  RecordDownloadCount(CANCELLED_COUNT);
-
-  // TODO(rdsmith/benjhayden): Remove condition as part of
-  // |SavePackage| integration.
-  // |download_file_| can be NULL if Interrupt() is called after the
-  // download file has been released.
-  if (!is_save_package_download_ && download_file_)
-    ReleaseDownloadFile(true);
-
-  if (state_ == IN_PROGRESS_INTERNAL) {
-    // Cancel the originating URL request unless it's already been cancelled
-    // by interrupt.
-    request_handle_->CancelRequest();
-  }
-
-  // Remove the intermediate file if we are cancelling an interrupted download.
-  // Continuable interruptions leave the intermediate file around.
-  if ((state_ == INTERRUPTED_INTERNAL || state_ == RESUMING_INTERNAL) &&
-      !current_path_.empty()) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(base::IgnoreResult(&DeleteDownloadedFile), current_path_));
-    current_path_.clear();
-  }
-
-  TransitionTo(CANCELLED_INTERNAL, UPDATE_OBSERVERS);
+  Interrupt(user_cancel ? DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
+                        : DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN);
 }
 
 void DownloadItemImpl::Remove() {
@@ -435,7 +417,7 @@ void DownloadItemImpl::Remove() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   delegate_->AssertStateConsistent(this);
-  Cancel(true);
+  Interrupt(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED);
   delegate_->AssertStateConsistent(this);
 
   NotifyRemoved();
@@ -495,26 +477,43 @@ bool DownloadItemImpl::IsTemporary() const {
 }
 
 bool DownloadItemImpl::CanResume() const {
-  if ((GetState() == IN_PROGRESS) && IsPaused())
-    return true;
-
-  if (state_ != INTERRUPTED_INTERNAL)
-    return false;
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  switch (state_) {
+    case INITIAL_INTERNAL:
+    case COMPLETING_INTERNAL:
+    case COMPLETE_INTERNAL:
+    case CANCELLED_INTERNAL:
+    case RESUMING_INTERNAL:
+      return false;
 
-  // We currently only support HTTP(S) requests for download resumption.
-  if (!GetURL().SchemeIsHTTPOrHTTPS())
-    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);
+    }
 
-  ResumeMode resume_mode = GetResumeMode();
-  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 IN_PROGRESS_INTERNAL:
+    case INITIAL_INTERNAL:
     case COMPLETING_INTERNAL:
+    case RESUMING_INTERNAL:
+    case TARGET_PENDING_INTERNAL:
+    case TARGET_RESOLVED_INTERNAL:
+    case IN_PROGRESS_INTERNAL:
       return false;
 
     case COMPLETE_INTERNAL:
@@ -524,14 +523,10 @@ bool DownloadItemImpl::IsDone() const {
     case INTERRUPTED_INTERNAL:
       return !CanResume();
 
-    case RESUMING_INTERNAL:
-      return false;
-
     case MAX_DOWNLOAD_INTERNAL_STATE:
-      break;
+      NOTREACHED();
   }
-  NOTREACHED();
-  return true;
+  return false;
 }
 
 const GURL& DownloadItemImpl::GetURL() const {
@@ -770,6 +765,14 @@ WebContents* DownloadItemImpl::GetWebContents() const {
 void DownloadItemImpl::OnContentCheckCompleted(DownloadDangerType danger_type) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(AllDataSaved());
+
+  // Danger type is only allowed to be set on an active download after all data
+  // has been saved. This excludes all other states. In particular,
+  // OnContentCheckCompleted() isn't allowed on an INTERRUPTED download since
+  // such an interruption would need to happen between OnAllDataSaved() and
+  // OnContentCheckCompleted() during which no disk or network activity
+  // should've taken place.
+  DCHECK_EQ(state_, IN_PROGRESS_INTERNAL);
   DVLOG(20) << __FUNCTION__ << " danger_type=" << danger_type
             << " download=" << DebugString(true);
   SetDangerType(danger_type);
@@ -854,6 +857,14 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
 
 DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (!IsDownloadResumptionEnabled())
+    return RESUME_MODE_INVALID;
+
+  // Cnly support resumption for HTTP(S).
+  if (!GetURL().SchemeIsHTTPOrHTTPS())
+    return RESUME_MODE_INVALID;
+
   // We can't continue without a handle on the intermediate file.
   // We also can't continue if we don't have some verifier to make sure
   // we're getting the same file.
@@ -994,8 +1005,8 @@ void DownloadItemImpl::SetTotalBytes(int64_t total_bytes) {
 
 void DownloadItemImpl::OnAllDataSaved(const std::string& final_hash) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
+  DCHECK(state_ == TARGET_PENDING_INTERNAL ||
+         state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
@@ -1027,12 +1038,11 @@ void DownloadItemImpl::DestinationUpdate(int64_t bytes_so_far,
     // Ignore if we're no longer in-progress.  This can happen if we race a
     // Cancel on the UI thread with an update on the FILE thread.
     //
-    // TODO(rdsmith): Arguably we should let this go through, as this means
-    // the download really did get further than we know before it was
-    // cancelled.  But the gain isn't very large, and the code is more
-    // fragile if it has to support in progress updates in a non-in-progress
-    // state.  This issue should be readdressed when we revamp performance
-    // reporting.
+    // Arguably we should let this go through, as this means the download really
+    // did get further than we know before it was cancelled.  But the gain isn't
+    // very large, and the code is more fragile if it has to support in progress
+    // updates in a non-in-progress state.  This issue should be readdressed

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

The existence of a phrase "This issue should be readdressed" makes me think that
there should be a TODO somewhere.

[asanka, 2016/02/11 21:12:05]

Done.

+    // when we revamp performance reporting.
     return;
   }
   bytes_per_sec_ = bytes_per_sec;
@@ -1054,10 +1064,15 @@ void DownloadItemImpl::DestinationUpdate(int64_t bytes_so_far,
 }
 
 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 (current_path_.empty() || target_path_.empty())
+  if (state_ != IN_PROGRESS_INTERNAL)
     destination_error_ = reason;
   else
     Interrupt(reason);
@@ -1065,8 +1080,6 @@ void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
-  if (GetState() != IN_PROGRESS)
-    return;
   OnAllDataSaved(final_hash);
   MaybeCompleteDownload();
 }
@@ -1116,11 +1129,12 @@ void DownloadItemImpl::Start(
   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 (GetState() == CANCELLED) {
+  if (state_ == CANCELLED_INTERNAL) {
     // The download was in the process of resuming when it was cancelled. Don't
     // proceed.
     ReleaseDownloadFile(true);
@@ -1128,7 +1142,7 @@ void DownloadItemImpl::Start(
     return;
   }
 
-  TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
+  TransitionTo(TARGET_PENDING_INTERNAL, UPDATE_OBSERVERS);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ -1142,7 +1156,13 @@ void DownloadItemImpl::Start(
 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.

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

I don't follow "Transition out to IN_PROGRESS"; this os going TARGET_RESOLVED ->
INTERRUPTED, right?

[asanka, 2016/02/11 21:12:05]

Yup :) Done

+    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
@@ -1168,6 +1188,7 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
     DownloadDangerType danger_type,
     const base::FilePath& intermediate_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
 
   // If the |target_path| is empty, then we consider this download to be
   // canceled.
@@ -1231,7 +1252,9 @@ 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);
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != destination_error_) {
     // Process destination error.  If both |reason| and |destination_error_|
@@ -1249,7 +1272,7 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     DCHECK(current_path_.empty());
   } else {
     SetFullPath(full_path);
-    UpdateObservers();
+    TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
     MaybeCompleteDownload();
   }
 }
@@ -1272,12 +1295,8 @@ void DownloadItemImpl::MaybeCompleteDownload() {
                      weak_ptr_factory_.GetWeakPtr())))
     return;
 
-  // TODO(rdsmith): DCHECK that we only pass through this point
-  // once per download.  The natural way to do this is by a state
-  // transition on the DownloadItem.
-
-  // Confirm we're in the proper set of states to be here;
-  // have all data, have a history handle, (validated or safe).
+  // Confirm we're in the proper set of states to be here; have all data, have a
+  // history handle, (validated or safe).
   DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
   DCHECK(!IsDangerous());
   DCHECK(all_data_saved_);
@@ -1356,7 +1375,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   }
 
   // Complete the download and release the DownloadFile.
-  DCHECK(download_file_.get());
+  DCHECK(download_file_);
   ReleaseDownloadFile(false);
 
   // We're not completely done with the download item yet, but at this
@@ -1430,6 +1449,9 @@ void DownloadItemImpl::OnResumeRequestStarted(
 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);
 
   // Somewhat counter-intuitively, it is possible for us to receive an
   // interrupt after we've already been interrupted.  The generation of
@@ -1439,56 +1461,90 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   // respectively), and since we choose not to keep state on the File thread,
   // this is the place where the races collide.  It's also possible for
   // interrupts to race with cancels.
+  switch (state_) {
+    case CANCELLED_INTERNAL:
+    // If the download is already cancelled, then there's no point in
+    // transitioning out to interrupted.
+    case COMPLETING_INTERNAL:
+    case COMPLETE_INTERNAL:
+      // Already complete.
+      return;
 
-  // Whatever happens, the first one to hit the UI thread wins.
-  if (state_ != IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)
-    return;
+    case TARGET_PENDING_INTERNAL:
+    case TARGET_RESOLVED_INTERNAL:
+    case IN_PROGRESS_INTERNAL:
+      break;
 
-  last_reason_ = reason;
+    case RESUMING_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;
 
-  ResumeMode resume_mode = GetResumeMode();
-
-  if (state_ == IN_PROGRESS_INTERNAL) {
-    // Cancel (delete file) if:
-    // 1) we're going to restart.
-    // 2) Resumption isn't possible (download was cancelled or blocked due to
-    //    security restrictions).
-    // 3) Resumption isn't enabled.
-    // No point in leaving data around we aren't going to use.
-    ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
-                        resume_mode == RESUME_MODE_USER_RESTART ||
-                        resume_mode == RESUME_MODE_INVALID ||
-                        !IsDownloadResumptionEnabled());
-
-    // Cancel the originating URL request.
-    request_handle_->CancelRequest();
-  } else {
-    DCHECK(!download_file_.get());
+      if (!current_path_.empty()) {
+        BrowserThread::PostTask(
+            BrowserThread::FILE, FROM_HERE,
+            base::Bind(base::IgnoreResult(&DeleteDownloadedFile),
+                       current_path_));
+        current_path_.clear();
+      }
+      break;
+
+    case INITIAL_INTERNAL:
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      NOTREACHED();
+      return;
   }
 
-  // Reset all data saved, as even if we did save all the data we're going
-  // to go through another round of downloading when we resume.
-  // There's a potential problem here in the abstract, as if we did download
-  // all the data and then run into a continuable error, on resumption we
-  // won't download any more data.  However, a) there are currently no
-  // continuable errors that can occur after we download all the data, and
-  // b) if there were, that would probably simply result in a null range
-  // request, which would generate a DestinationCompleted() notification
-  // from the DownloadFile, which would behave properly with setting
-  // all_data_saved_ to false here.
+  last_reason_ = reason;
+  if (download_file_) {
+    ResumeMode resume_mode = GetResumeMode();
+    ReleaseDownloadFile(resume_mode != RESUME_MODE_IMMEDIATE_CONTINUE &&
+                        resume_mode != RESUME_MODE_USER_CONTINUE);
+  }
+
+  // Reset all data saved, as even if we did save all the data we're going to go
+  // through another round of downloading when we resume. There's a potential
+  // problem here in the abstract, as if we did download all the data and then
+  // run into a continuable error, on resumption we won't download any more
+  // data.  However, a) there are currently no continuable errors that can occur
+  // after we download all the data, and b) if there were, that would probably
+  // simply result in a null range request, which would generate a
+  // DestinationCompleted() notification from the DownloadFile, which would
+  // behave properly with setting all_data_saved_ to false here.
   all_data_saved_ = false;
 
-  TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS);
-  RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
-  if (!GetWebContents())
-    RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
+  if (request_handle_)
+    request_handle_->CancelRequest();
+
+  if (reason == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED ||
+      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();
+  }
 
-  AutoResumeIfValid();
   UpdateObservers();
 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "() destroy_file:" << destroy_file;
 
   if (destroy_file) {
     BrowserThread::PostTask(
@@ -1514,6 +1570,11 @@ void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
 
 bool DownloadItemImpl::IsDownloadReadyForCompletion(
     const base::Closure& state_change_notification) {
+  // If the download hasn't progressed to the IN_PROGRESS state, then it's not
+  // ready for completion.
+  if (state_ != IN_PROGRESS_INTERNAL)
+    return false;
+
   // If we don't have all the data, the download is not ready for
   // completion.
   if (!AllDataSaved())
@@ -1524,20 +1585,11 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion(
   if (IsDangerous())
     return false;
 
-  // If the download isn't active (e.g. has been cancelled) it's not
-  // ready for completion.
-  if (state_ != IN_PROGRESS_INTERNAL)
-    return false;
-
-  // If the target filename hasn't been determined, then it's not ready for
-  // completion. This is checked in ReadyForDownloadCompletionDone().
-  if (GetTargetFilePath().empty())
-    return false;
-
-  // This is checked in NeedsRename(). Without this conditional,
-  // browser_tests:DownloadTest.DownloadMimeType fails the DCHECK.
-  if (target_path_.DirName() != current_path_.DirName())
-    return false;
+  // Invariants for the IN_PROGRESS state. DCHECKs here verify that the
+  // invariants are still true.
+  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.
@@ -1557,49 +1609,89 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
   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:
+      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, "
+                             "it must not be paused.";
+      break;
+
     case COMPLETING_INTERNAL:
+      DCHECK(all_data_saved_) << "All data must be saved prior to completion.";
+      DCHECK(!download_file_)
+          << "Download file must be released prior to completion.";
+      DCHECK(!target_path_.empty()) << "Target path must be known.";
+      DCHECK(current_path_ == target_path_)
+          << "Current output path must match target path.";
+
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_COMPLETING,
           base::Bind(&ItemCompletingNetLogCallback, received_bytes_, &hash_));
       break;
+
     case COMPLETE_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_FINISHED,
           base::Bind(&ItemFinishedNetLogCallback, auto_opened_));
       break;
+
     case INTERRUPTED_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_INTERRUPTED,
           base::Bind(&ItemInterruptedNetLogCallback, last_reason_,
                      received_bytes_, &hash_state_));
       break;
-    case IN_PROGRESS_INTERNAL:
-      if (old_state == INTERRUPTED_INTERNAL) {
-        bound_net_log_.AddEvent(
-            net::NetLog::TYPE_DOWNLOAD_ITEM_RESUMED,
-            base::Bind(&ItemResumingNetLogCallback,
-                       false, last_reason_, received_bytes_, &hash_state_));
-      }
+
+    case RESUMING_INTERNAL:
+      bound_net_log_.AddEvent(
+          net::NetLog::TYPE_DOWNLOAD_ITEM_RESUMED,
+          base::Bind(&ItemResumingNetLogCallback, false, last_reason_,
+                     received_bytes_, &hash_state_));
       break;
+
     case CANCELLED_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_CANCELED,
           base::Bind(&ItemCanceledNetLogCallback, received_bytes_,
                      &hash_state_));
       break;
-    default:
+
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      NOTREACHED();
       break;
   }
 
-  DVLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true)
-     << " " << InternalToExternalState(old_state)
-     << " " << InternalToExternalState(state_);
-
-  bool is_done = (state_ != IN_PROGRESS_INTERNAL &&
-                  state_ != COMPLETING_INTERNAL);
-  bool was_done = (old_state != IN_PROGRESS_INTERNAL &&
-                   old_state != COMPLETING_INTERNAL);
+  DVLOG(20) << " " << __FUNCTION__ << "()"
+            << " from:" << DebugDownloadStateString(old_state)
+            << " to:" << DebugDownloadStateString(state_)
+            << " this = " << DebugString(true);
+  bool is_done =
+      (state_ != TARGET_PENDING_INTERNAL &&
+       state_ != TARGET_RESOLVED_INTERNAL && state_ != IN_PROGRESS_INTERNAL &&
+       state_ != COMPLETING_INTERNAL);
+  bool was_done =
+      (old_state != TARGET_PENDING_INTERNAL &&
+       old_state != TARGET_RESOLVED_INTERNAL &&
+       old_state != IN_PROGRESS_INTERNAL && old_state != COMPLETING_INTERNAL);
   // Termination
   if (is_done && !was_done)
     bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
@@ -1704,17 +1796,23 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
       base::Bind(&DownloadItemImpl::OnResumeRequestStarted,
                  weak_ptr_factory_.GetWeakPtr()));
 
+  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
   delegate_->ResumeInterruptedDownload(std::move(download_params), GetId());
   // Just in case we were interrupted while paused.
   is_paused_ = false;
-
-  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
 }
 
 // static
 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:
@@ -1753,9 +1851,80 @@ DownloadItemImpl::ExternalToInternalState(
   return MAX_DOWNLOAD_INTERNAL_STATE;
 }
 
+#if DCHECK_IS_ON()
+// static
+bool DownloadItemImpl::IsValidSavePackageStateTransition(
+    DownloadInternalState from,
+    DownloadInternalState to) {
+  switch (from) {
+    case INITIAL_INTERNAL:
+    case 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;
+
+    case TARGET_PENDING_INTERNAL:
+      return to == TARGET_RESOLVED_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;
+
+    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 TARGET_RESOLVED_INTERNAL:
+      return "TARGET_RESOLVED";
     case IN_PROGRESS_INTERNAL:
       return "IN_PROGRESS";
     case COMPLETING_INTERNAL: