[Downloads] Enforce state transition integrity and state invariants.

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 142 matching lines @@
       file_externally_removed_(false),
       auto_opened_(false),
       is_temporary_(false),
       all_data_saved_(state == COMPLETE),
       destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       opened_(opened),
       delegate_delayed_complete_(false),
       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);
 }
 
 // Constructing for a regular download:
 DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
                                    uint32_t download_id,
                                    const DownloadCreateInfo& info,
                                    const net::BoundNetLog& bound_net_log)
     : is_save_package_download_(false),
       download_id_(download_id),
@@ skipping 12 matching lines @@
       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),
       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),
       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),
@@ skipping 113 matching lines @@
   UpdateObservers();
 
   MaybeCompleteDownload();
 }
 
 void DownloadItemImpl::StealDangerousDownload(
     const AcquireFileCallback& callback) {
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(IsDangerous());
+
   if (download_file_) {
     BrowserThread::PostTaskAndReplyWithResult(
         BrowserThread::FILE,
         FROM_HERE,
         base::Bind(&DownloadFileDetach, base::Passed(&download_file_)),
         callback);
   } else {
     callback.Run(current_path_);
   }
   current_path_.clear();
   Remove();
   // We have now been deleted.
 }
 
 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 IN_PROGRESS_INTERNAL:
+      request_handle_->PauseRequest();
+      is_paused_ = true;
+      UpdateObservers();
+      return;
+
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+    case TARGET_RESOLVED_INTERNAL:
+      NOTREACHED();
+  }
 }
 
 void DownloadItemImpl::Resume() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   switch (state_) {
+    case INITIAL_INTERNAL:
+    case COMPLETING_INTERNAL:
+    case COMPLETE_INTERNAL:
+    case CANCELLED_INTERNAL:
+    // Nothing to resume.
+    case RESUMING_INTERNAL:
+      // Resumption in progress.
+      DCHECK(!is_paused_);
+      return;
+
+    case TARGET_PENDING_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       if (!is_paused_)
         return;
       request_handle_->ResumeRequest();
       is_paused_ = false;
       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();
       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
+    case TARGET_RESOLVED_INTERNAL:
       NOTREACHED();
   }
 }
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  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() {
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   delegate_->AssertStateConsistent(this);
-  Cancel(true);
+  Interrupt(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED);
   delegate_->AssertStateConsistent(this);
 
   NotifyRemoved();
   delegate_->DownloadRemoved(this);
   // We have now been deleted.
 }
 
 void DownloadItemImpl::OpenDownload() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ skipping 39 matching lines @@
 
 bool DownloadItemImpl::IsPaused() const {
   return is_paused_;
 }
 
 bool DownloadItemImpl::IsTemporary() const {
   return is_temporary_;
 }
 
 bool DownloadItemImpl::CanResume() const {
-  if ((GetState() == IN_PROGRESS) && IsPaused())
-    return true;
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  switch (state_) {
+    case INITIAL_INTERNAL:
+    case COMPLETING_INTERNAL:
+    case COMPLETE_INTERNAL:
+    case CANCELLED_INTERNAL:
+    case RESUMING_INTERNAL:
+      return false;
 
-  if (state_ != INTERRUPTED_INTERNAL)
-    return false;
+    case TARGET_PENDING_INTERNAL:
+    case TARGET_RESOLVED_INTERNAL:
+    case IN_PROGRESS_INTERNAL:
+      return is_paused_;
 
-  // We currently only support HTTP(S) requests for download resumption.
-  if (!GetURL().SchemeIsHTTPOrHTTPS())
-    return false;
+    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 INITIAL_INTERNAL:
+    case COMPLETING_INTERNAL:
+    case RESUMING_INTERNAL:
+    case TARGET_PENDING_INTERNAL:
+    case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
-    case COMPLETING_INTERNAL:
       return false;
 
     case COMPLETE_INTERNAL:
     case CANCELLED_INTERNAL:
       return true;
 
     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 {
   return url_chain_.empty() ? GURL::EmptyGURL() : url_chain_.back();
 }
 
 const std::vector<GURL>& DownloadItemImpl::GetUrlChain() const {
   return url_chain_;
 }
 
@@ skipping 218 matching lines @@
   // Currently such items have null request_handle_s, where other items
   // (regular and SavePackage downloads) have actual objects off the pointer.
   if (request_handle_)
     return request_handle_->GetWebContents();
   return NULL;
 }
 
 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);
   UpdateObservers();
 }
 
 void DownloadItemImpl::SetOpenWhenComplete(bool open) {
   open_when_complete_ = open;
 }
 
@@ skipping 64 matching lines @@
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
 
   description += " }";
 
   return description;
 }
 
 DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (!IsDownloadResumptionEnabled())
+    return RESUME_MODE_INVALID;
+
+  // Only 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.
   const bool force_restart =
       (current_path_.empty() || (etag_.empty() && last_modified_time_.empty()));
 
   // We won't auto-restart if we've used up our attempts or the
   // download has been paused by user action.
   const bool force_user =
       (auto_resume_count_ >= kMaxAutoResumeAttempts || is_paused_);
@@ skipping 120 matching lines @@
 const net::BoundNetLog& DownloadItemImpl::GetBoundNetLog() const {
   return bound_net_log_;
 }
 
 void DownloadItemImpl::SetTotalBytes(int64_t total_bytes) {
   total_bytes_ = total_bytes;
 }
 
 void DownloadItemImpl::OnAllDataSaved(const std::string& final_hash) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
 
   // Store final hash and null out intermediate serialized hash state.
   hash_ = final_hash;
   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);
 }
 
 void DownloadItemImpl::DestinationUpdate(int64_t bytes_so_far,
                                          int64_t bytes_per_sec,
                                          const std::string& hash_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // If the download is in any other state we don't expect any
+  // DownloadDestinationObserver callbacks. An interruption or a cancellation
+  // results in a call to ReleaseDownloadFile which invalidates the weak
+  // reference held by the DownloadFile and hence cuts off any pending
+  // callbacks.
+  DCHECK(state_ == TARGET_PENDING_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
   DVLOG(20) << __FUNCTION__ << " so_far=" << bytes_so_far
-            << " per_sec=" << bytes_per_sec << " download="
-            << DebugString(true);
+            << " per_sec=" << bytes_per_sec
+            << " download=" << DebugString(true);
 
-  if (GetState() != IN_PROGRESS) {
-    // 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.
-    return;
-  }
   bytes_per_sec_ = bytes_per_sec;
   hash_state_ = hash_state;
   received_bytes_ = bytes_so_far;
 
   // If we've received more data than we were expecting (bad server info?),
   // revert to 'unknown size mode'.
   if (received_bytes_ > total_bytes_)
     total_bytes_ = 0;
 
   if (bound_net_log_.IsCapturing()) {
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_UPDATED,
         net::NetLog::Int64Callback("bytes_so_far", received_bytes_));
   }
 
   UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // If the download is in any other state we don't expect any
+  // DownloadDestinationObserver callbacks. An interruption or a cancellation
+  // results in a call to ReleaseDownloadFile which invalidates the weak
+  // reference held by the DownloadFile and hence cuts off any pending
+  // callbacks.
+  DCHECK(state_ == TARGET_PENDING_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
+  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);
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // If the download is in any other state we don't expect any
+  // DownloadDestinationObserver callbacks. An interruption or a cancellation
+  // results in a call to ReleaseDownloadFile which invalidates the weak
+  // reference held by the DownloadFile and hence cuts off any pending
+  // callbacks.
+  DCHECK(state_ == TARGET_PENDING_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
-  if (GetState() != IN_PROGRESS)
-    return;
+
   OnAllDataSaved(final_hash);
   MaybeCompleteDownload();
 }
 
 // **** Download progression cascade
 
 void DownloadItemImpl::Init(bool active,
                             DownloadType download_type) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ skipping 29 matching lines @@
 }
 
 // We're starting the download.
 void DownloadItemImpl::Start(
     scoped_ptr<DownloadFile> file,
     scoped_ptr<DownloadRequestHandleInterface> req_handle) {
   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 (GetState() == CANCELLED) {
+  if (state_ == CANCELLED_INTERNAL) {
     // The download was in the process of resuming when it was cancelled. Don't
     // proceed.
     ReleaseDownloadFile(true);
     request_handle_->CancelRequest();
     return;
   }
 
-  TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
+  TransitionTo(TARGET_PENDING_INTERNAL, UPDATE_OBSERVERS);
 
   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;
   }
 
   delegate_->DetermineDownloadTarget(
       this, base::Bind(&DownloadItemImpl::OnDownloadTargetDetermined,
                        weak_ptr_factory_.GetWeakPtr()));
 }
 
 // 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);
 
   // 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
@@ skipping 43 matching lines @@
       base::Bind(&DownloadFile::RenameAndUniquify,
                  // Safe because we control download file lifetime.
                  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);
 
   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);
-    UpdateObservers();
+    TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
     MaybeCompleteDownload();
   }
 }
 
 // When SavePackage downloads MHTML to GData (see
 // SavePackageFilePickerChromeOS), GData calls MaybeCompleteDownload() like it
 // does for non-SavePackage downloads, but SavePackage downloads never satisfy
 // IsDownloadReadyForCompletion(). GDataDownloadObserver manually calls
 // DownloadItem::UpdateObservers() when the upload completes so that SavePackage
 // notices that the upload has completed and runs its normal Finish() pathway.
 // MaybeCompleteDownload() is never the mechanism by which SavePackage completes
 // downloads. SavePackage always uses its own Finish() to mark downloads
 // complete.
 void DownloadItemImpl::MaybeCompleteDownload() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!is_save_package_download_);
 
   if (!IsDownloadReadyForCompletion(
           base::Bind(&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_);
 
   OnDownloadCompleting();
 }
 
 // Called by MaybeCompleteDownload() when it has determined that the download
 // is ready for completion.
 void DownloadItemImpl::OnDownloadCompleting() {
@@ skipping 58 matching lines @@
 
   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_.get());
+  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);
 
   if (delegate_->ShouldOpenDownload(
           this, base::Bind(&DownloadItemImpl::DelayedDownloadOpened,
@@ skipping 53 matching lines @@
   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);
 
   // Somewhat counter-intuitively, it is possible for us to receive an
   // interrupt after we've already been interrupted.  The generation of
   // interrupts from the file thread Renames and the generation of
   // interrupts from disk writes go through two different mechanisms (driven
   // by rename requests from UI thread and by write requests from IO thread,
   // 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 INITIAL_INTERNAL:
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      NOTREACHED();
+      return;
 
-  last_reason_ = reason;
+    case TARGET_PENDING_INTERNAL:
+    case TARGET_RESOLVED_INTERNAL:
+    case IN_PROGRESS_INTERNAL:
+      // last_reason_ needs to be set for GetResumeMode() to work.
+      last_reason_ = reason;
 
-  ResumeMode resume_mode = GetResumeMode();
+      if (download_file_) {
+        ResumeMode resume_mode = GetResumeMode();
+        ReleaseDownloadFile(resume_mode != RESUME_MODE_IMMEDIATE_CONTINUE &&
+                            resume_mode != RESUME_MODE_USER_CONTINUE);
+      } else {
+        // download_file_ will be missing if we get here via
+        // StealDangerousDownload().
+        DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED, reason);

[asanka, 2016/02/12 15:53:37]

SavePackage also hits this code path. I removed the DCHECK.

[Randy Smith (Not in Mondays), 2016/02/12 17:37:02]

It's ok either way, but any reason not to just make the DCHECK dependent on SP
as well?

[asanka, 2016/02/12 20:21:24]

It tipped the scales against the assertion being useful. I.e. it would be
important to know if download_file_ is always non-null here, but once there's
more than one reason why it wouldn't, then enforcing those causes via DCHECKs
becomes of questionable utility. I opted to not enforce it.

+      }
+      break;
 
-  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());
+    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;
 
-    // Cancel the originating URL request.
-    request_handle_->CancelRequest();
-  } else {
-    DCHECK(!download_file_.get());
+      last_reason_ = reason;
+      if (!current_path_.empty()) {
+        // There is no download file and this is transitioning from INTERRUPTED
+        // to CANCELLED. The intermediate file is no longer usable, and should
+        // be deleted.
+        BrowserThread::PostTask(
+            BrowserThread::FILE, FROM_HERE,
+            base::Bind(base::IgnoreResult(&DeleteDownloadedFile),
+                       current_path_));
+        current_path_.clear();
+      }
+      break;
   }
 
-  // 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.
+  // 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();
 
-  AutoResumeIfValid();
+  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();
+  }
+
   UpdateObservers();
 }
 
 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.
         base::Bind(&DownloadFileCancel, base::Passed(&download_file_)));
     // Avoid attempting to reuse the intermediate file by clearing out
     // current_path_.
     current_path_.clear();
   } else {
     BrowserThread::PostTask(
         BrowserThread::FILE,
         FROM_HERE,
         base::Bind(base::IgnoreResult(&DownloadFileDetach),
                    // Will be deleted at end of task execution.
                    base::Passed(&download_file_)));
   }
   // Don't accept any more messages from the DownloadFile, and null
   // out any previous "all data received".  This also breaks links to
   // other entities we've given out weak pointers to.
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
 
 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())
     return false;
 
   // If the download is dangerous, but not yet validated, it's not ready for
   // completion.
   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.
   if (!delegate_->ShouldCompleteDownload(this, state_change_notification))
     return false;
 
   return true;
 }
 
 void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
                                     ShouldUpdateObservers notify_action) {
   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:
+      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_);
+  DVLOG(20) << " " << __FUNCTION__ << "()"
+            << " from:" << DebugDownloadStateString(old_state)
+            << " to:" << DebugDownloadStateString(state_)
+            << " this = " << DebugString(true);
+  bool is_done =
+      (state_ == COMPLETE_INTERNAL || state_ == INTERRUPTED_INTERNAL ||
+       state_ == RESUMING_INTERNAL || state_ == CANCELLED_INTERNAL);
+  bool was_done =
+      (old_state == COMPLETE_INTERNAL || old_state == INTERRUPTED_INTERNAL ||
+       old_state == RESUMING_INTERNAL || old_state == CANCELLED_INTERNAL);
 
-  bool is_done = (state_ != IN_PROGRESS_INTERNAL &&
-                  state_ != COMPLETING_INTERNAL);
-  bool was_done = (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);
 
   // 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,
@@ skipping 84 matching lines @@
 
   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);
   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:
       return IN_PROGRESS;
     case COMPLETE_INTERNAL:
       return COMPLETE;
     case CANCELLED_INTERNAL:
       return CANCELLED;
     case INTERRUPTED_INTERNAL:
       return INTERRUPTED;
@@ skipping 18 matching lines @@
     case CANCELLED:
       return CANCELLED_INTERNAL;
     case INTERRUPTED:
       return INTERRUPTED_INTERNAL;
     default:
       NOTREACHED();
   }
   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 || to == 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;
+  return true;
+}
+#endif  // DCHECK_IS_ON()
+
 const char* DownloadItemImpl::DebugDownloadStateString(
     DownloadInternalState state) {
   switch (state) {
+    case INITIAL_INTERNAL:
+      return "INITIAL";
+    case TARGET_PENDING_INTERNAL:
+      return "TARGET_PENDING";
+    case 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";
     case INTERRUPTED_INTERNAL:
       return "INTERRUPTED";
@@ skipping 17 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