[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 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;

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

suggestion: DCHECK(!is_paused_)?

[asanka, 2016/02/12 05:04:26]

Done.

+
+    case TARGET_PENDING_INTERNAL:
+    case TARGET_RESOLVED_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:
       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_)

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Interesting.  Was the is_save_package_download_ check vestigial?  I can't come
up with a reason why it would have been needed before this CL, so I presume
removing it was just a cleanup?

[asanka, 2016/02/12 05:04:26]

It was cleanup. I don't know why it was there either, but in its current form,
it wasn't doing anything since SavePackage downloads don't have a
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:

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Not in this CL (as not changed in this CL): I find this contradictory with the
external state for this state being COMPLETE.

[asanka, 2016/02/12 05:04:26]

Acknowledged.

+    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;
+
+  // Cnly support resumption for HTTP(S).

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

Interesting.  This seems like a semantic change that isn't really part of this
CL; what am I missing?  

[asanka, 2016/02/12 05:04:26]

I moved this check from CanResume() to here. Earlier even though GetResumeMode()
returns a valid mode, CanResume() will block based on the URL scheme and whether
resumption is enabled. Now GetResumeMode() will return RESUME_MODE_INVALID if
the download is in a state where it cannot be resumed.

Previously, we could leave a partial file on disk on the false assumption that
it could be continued when it could not because the only gate for deciding
whether to delete the partial file is the result of calling GetResumeMode().

The set of states where resumption is allowed and their effective modes should
be the same.

+  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(state_ == TARGET_PENDING_INTERNAL ||
+         state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL);

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

DestinationCompleted comes in via post from the file thread, right?  So why
couldn't we be interrupted when it arrives?  (I.e. I'm wondering if removing the
conditional in DestinationCompleted() might be a bug.)

[asanka, 2016/02/12 05:04:26]

This one is a bit subtle. Basically, if the DII transitions to interrupted or
cancel or complete, we call ReleaseDownloadFile, which unconditionally
invalidates the weak pointers. This effectively cuts off any destination
observer callbacks at the point where we release the download file.

I moved the DCHECKs to the destination observer methods along with a comment
explaining why we don't expect callbacks in any other state.

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

Ah, thank you.  However, as I read the code, what you say isn't precisely true. 
In the Interrupt/Cancel case, we only call ReleaseDownloadFile download_file_ is
not null.  I don't think that's a problem, since the code only sets it to null
in ReleaseDownloadFile(), but I wanted to mention the very slight imprecision in
case you saw a place where it might be a problem.

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

Yeah. We rely on download_file_ only being set to null in ReleaseDownloadFile.
It's a scoped_ptr and needs to be destroyed on the FILE thread (enforced via
DCHECKs). Setting download_file_ to null isn't trivial. I think it's fair to
rely on ReleaseDownloadFile being the only place where download_file_ is
discarded.

   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();
 }
@@ skipping 38 matching lines @@
   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(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)

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Confirming I understand (if this is true, I'm ok with it): For this CL, the
paths being empty is equivalent to being in TARGET_PENDING/RESOLVED, but for the
next CL resumption would have allowed those variables to be set even if target
determination hadn't completed, therefore we need to actually base the
conditional on the state variable?

[asanka, 2016/02/12 05:04:26]

Correct.

     destination_error_ = reason;
   else
     Interrupt(reason);
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
   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 TARGET_PENDING_INTERNAL:
+    case TARGET_RESOLVED_INTERNAL:
+    case IN_PROGRESS_INTERNAL:
+      break;
+
+    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;
+
+      if (!current_path_.empty()) {
+        BrowserThread::PostTask(
+            BrowserThread::FILE, FROM_HERE,
+            base::Bind(base::IgnoreResult(&DeleteDownloadedFile),
+                       current_path_));
+        current_path_.clear();

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Am I right in thinking that if this path has been taken, the download_file_ will
be null, and thus the download file logic below won't be executed?  If so,
willing to put in a DCHECK?  I spent a bit of time puzzling over this code
trying to figure out how this PostTask and the below ReleaseDownloadFile()
interacted with each other, and if they don't it would be nice to state that. 
I'm tempted to suggest you put all the download file handling in this routine in
one place and cascade it to make it even clearer that only one of these paths
can be taken, but I'm happy to go with your judgement as to whether or not that
would be clearer.

[asanka, 2016/02/12 05:04:26]

I moved the download_file_ handling to one place, but then it wasn't quite clear
what was going on. Instead I moved the 'if (download_file_) bit to the
IN_PROGRESS branch above and removed the condition. The IN_PROGRESS states
should always have the download_file_ intact.

And then I left the above code as-is.

That way it should be clear that the two code paths are mutually exclusive.

+      }
+      break;
+
+    case INITIAL_INTERNAL:
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      NOTREACHED();
+      return;
+  }
 
   last_reason_ = reason;
-
-  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 (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.
+  // 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,

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

Nice invariant checking.

[asanka, 2016/02/12 05:04:26]

Thanks!

                                     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_);
-
-  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 =

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

nit, suggestion: I'm feeling tempted to suggest inclusive rather than exclusive
state testing here (i.e. list the states that are final states, not the ones
that aren't).

[asanka, 2016/02/12 05:04:26]

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

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

nit, suggestion: I wince a bit at declaration a routine and not defining it (as
will happen in release builds AIUTC); maybe either just null out the inside of
these routines to always return true (or false) or put this #if in the .h file
as well?

[asanka, 2016/02/12 05:04:26]

Opted to put the #if in the .h file.

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

Well, that didn't. Apparently the symbol still needs to be declared for some
reason I was too tired to figure out. Went with #if inside the definition.

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

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

I think you were typing a little too fast here :-}.  (i.e. ITYM "return (to ==
CANCELLED_INTERNAL) || (to == COMPLETE_INTERNAL);")

[asanka, 2016/02/12 05:04:26]

Hehe :) Done.

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