[downloads] Move platform specific code out of DownloadTargetDeterminer.

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 71758ea0f427250204f951a03ba493d4737dccce..db6668af9785ed63eef3133944f8af3977da239c 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -116,6 +116,11 @@ static void DownloadFileCancel(std::unique_ptr<DownloadFile> download_file) {
   download_file->Cancel();
 }
 
+bool IsCancellation(DownloadInterruptReason reason) {
+  return reason == DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN ||
+         reason == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED;
+}
+
 }  // namespace
 
 const uint32_t DownloadItem::kInvalidId = 0;
@@ -318,7 +323,6 @@ void DownloadItemImpl::ValidateDangerousDownload() {
   UpdateObservers();  // TODO(asanka): This is potentially unsafe. The download
                       // may not be in a consistent state or around at all after
                       // invoking observers. http://crbug.com/586610
-
   MaybeCompleteDownload();
 }
 
@@ -612,11 +616,11 @@ std::string DownloadItemImpl::GetRemoteAddress() const {
 
 bool DownloadItemImpl::HasUserGesture() const {
   return has_user_gesture_;
-};
+}
 
 ui::PageTransition DownloadItemImpl::GetTransitionType() const {
   return transition_type_;
-};
+}
 
 const std::string& DownloadItemImpl::GetLastModifiedTime() const {
   return last_modified_time_;
@@ -911,7 +915,7 @@ DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
   bool user_action_required =
       (auto_resume_count_ >= kMaxAutoResumeAttempts || IsPaused());
 
-  switch(last_reason_) {
+  switch (last_reason_) {
     case DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR:
     case DOWNLOAD_INTERRUPT_REASON_NETWORK_TIMEOUT:
       break;
@@ -972,7 +976,6 @@ DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
     case DOWNLOAD_INTERRUPT_REASON_SERVER_UNAUTHORIZED:
     case DOWNLOAD_INTERRUPT_REASON_SERVER_CERT_PROBLEM:
     case DOWNLOAD_INTERRUPT_REASON_SERVER_FORBIDDEN:
-      // Unhandled.
       return RESUME_MODE_INVALID;
   }
 
@@ -1097,8 +1100,8 @@ void DownloadItemImpl::DestinationUpdate(
   // callbacks.
   DCHECK(state_ == TARGET_PENDING_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
 
-  // There must be no pending destination_error_.
-  DCHECK_EQ(destination_error_, DOWNLOAD_INTERRUPT_REASON_NONE);
+  // There must be no pending deferred_interrupt_reason_.
+  DCHECK_EQ(deferred_interrupt_reason_, DOWNLOAD_INTERRUPT_REASON_NONE);
 
   DVLOG(20) << __func__ << "() so_far=" << bytes_so_far
             << " per_sec=" << bytes_per_sec
@@ -1127,18 +1130,9 @@ void DownloadItemImpl::DestinationError(
   // callbacks.
   DCHECK(state_ == TARGET_PENDING_INTERNAL || state_ == IN_PROGRESS_INTERNAL);
   DVLOG(20) << __func__
-            << "() reason:" << DownloadInterruptReasonToString(reason);
-
-  // Postpone recognition of this error until after file name determination
-  // has completed and the intermediate file has been renamed to simplify
-  // resumption conditions.
-  if (state_ == TARGET_PENDING_INTERNAL) {
-    received_bytes_ = bytes_so_far;
-    hash_state_ = std::move(secure_hash);
-    hash_.clear();
-    destination_error_ = reason;
-    return;
-  }
+            << "() reason:" << DownloadInterruptReasonToString(reason)
+            << " this:" << DebugString(true);
+
   InterruptWithPartialState(bytes_so_far, std::move(secure_hash), reason);
   UpdateObservers();
 }
@@ -1206,7 +1200,7 @@ void DownloadItemImpl::Start(
   download_file_ = std::move(file);
   job_ = DownloadJobFactory::CreateJob(this, std::move(req_handle),
                                        new_create_info);
-  destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
+  deferred_interrupt_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
 
   if (state_ == CANCELLED_INTERNAL) {
     // The download was in the process of resuming when it was cancelled. Don't
@@ -1242,24 +1236,13 @@ void DownloadItemImpl::Start(
             ? new_create_info.save_info->hash_state->Clone()
             : nullptr;
 
-    // Interrupted downloads also need a target path.
-    if (target_path_.empty()) {
-      received_bytes_ = offset;
-      hash_state_ = std::move(hash_state);
-      hash_.clear();
-      destination_error_ = new_create_info.result;
-      received_slices_.clear();
-      TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
-      DetermineDownloadTarget();
-      return;
-    }
-
-    // Otherwise, this was a resumption attempt which ended with an
-    // interruption. Continue with current target path.
-    TransitionTo(TARGET_RESOLVED_INTERNAL);
-    InterruptWithPartialState(
-        offset, std::move(hash_state), new_create_info.result);
-    UpdateObservers();
+    current_path_ = new_create_info.save_info->file_path;
+    received_bytes_ = offset;
+    hash_state_ = std::move(hash_state);
+    hash_.clear();
+    deferred_interrupt_reason_ = new_create_info.result;
+    TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+    DetermineDownloadTarget();
     return;
   }
 
@@ -1300,19 +1283,15 @@ void DownloadItemImpl::StartDownload() {
 void DownloadItemImpl::OnDownloadFileInitialized(
     DownloadInterruptReason result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
+  DCHECK(state_ == TARGET_PENDING_INTERNAL ||
+         state_ == INTERRUPTED_TARGET_PENDING_INTERNAL)
+      << "Unexpected state: " << DebugDownloadStateString(state_);
+
   DVLOG(20) << __func__
             << "() result:" << DownloadInterruptReasonToString(result);
   if (result != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    // Whoops. That didn't work. Proceed as an interrupted download, but reset
-    // the partial state. Currently, the partial stub cannot be recovered if the
-    // download file initialization fails.
-    received_bytes_ = 0;
-    hash_state_.reset();
-    hash_.clear();
-    destination_error_ = result;
-    received_slices_.clear();
-    TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+    ReleaseDownloadFile(true);
+    InterruptAndDiscardPartialState(result);
   }
 
   DetermineDownloadTarget();
@@ -1332,33 +1311,41 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
     const base::FilePath& target_path,
     TargetDisposition disposition,
     DownloadDangerType danger_type,
-    const base::FilePath& intermediate_path) {
+    const base::FilePath& intermediate_path,
+    DownloadInterruptReason interrupt_reason) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(state_ == TARGET_PENDING_INTERNAL ||
          state_ == INTERRUPTED_TARGET_PENDING_INTERNAL);
+  DVLOG(20) << __func__ << "() target_path:" << target_path.value()
+            << " intermediate_path:" << intermediate_path.value()
+            << " disposition:" << disposition << " danger_type:" << danger_type
+            << " interrupt_reason:"
+            << DownloadInterruptReasonToString(interrupt_reason)
+            << " this:" << DebugString(true);
 
-  // If the |target_path| is empty, then we consider this download to be
-  // canceled.
-  if (target_path.empty()) {
+  if (IsCancellation(interrupt_reason) || target_path.empty()) {
     Cancel(true);
     return;
   }
 
-  DVLOG(20) << __func__ << "() target_path:" << target_path.value()
-            << " disposition:" << disposition << " danger_type:" << danger_type
-            << " this:" << DebugString(true);
-
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
 
-  // This was an interrupted download that was looking for a filename. Now that
-  // it has one, transition to interrupted.
-  if (state_ == INTERRUPTED_TARGET_PENDING_INTERNAL) {
-    InterruptWithPartialState(
-        received_bytes_, std::move(hash_state_), destination_error_);
-    destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
-    UpdateObservers();
+  // There were no other pending errors, and we just failed to determined the
+  // download target.
+  if (state_ == TARGET_PENDING_INTERNAL &&
+      interrupt_reason != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    deferred_interrupt_reason_ = interrupt_reason;
+    TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+  }
+
+  // This was an interrupted download that was looking for a filename. Resolve
+  // early without performing the intermediate rename. If there is a
+  // DownloadFile, then that should be renamed to the intermediate name before
+  // we can interrupt the download. Otherwise we may lose intermediate state.
+  if (state_ == INTERRUPTED_TARGET_PENDING_INTERNAL && !download_file_) {
+    OnTargetResolved();
     return;
   }
 
@@ -1386,7 +1373,6 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   //               filename. Unnecessary renames may cause bugs like
   //               http://crbug.com/74187.
   DCHECK(!is_save_package_download_);
-  DCHECK(download_file_.get());
   DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
@@ -1402,32 +1388,53 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     DownloadInterruptReason reason,
     const base::FilePath& full_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
+  DCHECK(state_ == TARGET_PENDING_INTERNAL ||
+         state_ == INTERRUPTED_TARGET_PENDING_INTERNAL);
+  DCHECK(download_file_);
   DVLOG(20) << __func__ << "() download=" << DebugString(true);
 
-  TransitionTo(TARGET_RESOLVED_INTERNAL);
-
-  // If the intermediate rename fails while there's also a destination_error_,
-  // then the former is considered the critical error since it requires
-  // discarding the partial state.
-  if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
+  if (DOWNLOAD_INTERRUPT_REASON_NONE == reason) {
+    SetFullPath(full_path);
+  } else {
     // TODO(asanka): Even though the rename failed, it may still be possible to
     // recover the partial state from the 'before' name.
-    InterruptAndDiscardPartialState(reason);
-    UpdateObservers();
-    return;
+    deferred_interrupt_reason_ = reason;
+    TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
   }
+  OnTargetResolved();
+}
 
-  if (DOWNLOAD_INTERRUPT_REASON_NONE != destination_error_) {
-    SetFullPath(full_path);
-    InterruptWithPartialState(
-        received_bytes_, std::move(hash_state_), destination_error_);
-    destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
+void DownloadItemImpl::OnTargetResolved() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __func__ << "() download=" << DebugString(true);
+  DCHECK((state_ == TARGET_PENDING_INTERNAL &&
+          deferred_interrupt_reason_ == DOWNLOAD_INTERRUPT_REASON_NONE) ||
+         (state_ == INTERRUPTED_TARGET_PENDING_INTERNAL &&
+          deferred_interrupt_reason_ != DOWNLOAD_INTERRUPT_REASON_NONE))
+      << " deferred_interrupt_reason_:"
+      << DownloadInterruptReasonToString(deferred_interrupt_reason_)
+      << " this:" << DebugString(true);
+
+  // This transition is here to ensure that the DownloadItemImpl state machines
+  // doesn't transition to INTERRUPTED or IN_PROGRESS from
+  // TARGET_PENDING_INTERNAL directly. Doing so without passing through
+  // OnTargetResolved() can result in an externally visible state where the
+  // download is interrupted but doesn't have a target path associated with it.
+  //
+  // While not terrible, this complicates the DownloadItem<->Observer
+  // relationship since an observer that needs a target path in order to respond
+  // properly to an interruption will need to wait for another OnDownloadUpdated
+  // notification.  This requirement currently affects all of our UIs.
+  TransitionTo(TARGET_RESOLVED_INTERNAL);
+
+  if (DOWNLOAD_INTERRUPT_REASON_NONE != deferred_interrupt_reason_) {
+    InterruptWithPartialState(received_bytes_, std::move(hash_state_),
+                              deferred_interrupt_reason_);
+    deferred_interrupt_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
     UpdateObservers();
     return;
   }
 
-  SetFullPath(full_path);
   TransitionTo(IN_PROGRESS_INTERNAL);
   // TODO(asanka): Calling UpdateObservers() prior to MaybeCompleteDownload() is
   // not safe. The download could be in an underminate state after invoking
@@ -1631,9 +1638,28 @@ void DownloadItemImpl::InterruptWithPartialState(
       NOTREACHED();
       return;
 
+    case TARGET_PENDING_INTERNAL:
     case INTERRUPTED_TARGET_PENDING_INTERNAL:
+      // Postpone recognition of this error until after file name determination
+      // has completed and the intermediate file has been renamed to simplify
+      // resumption conditions. The target determination logic is much simpler
+      // if the state of the download remains constant until that stage
+      // completes.
+      //
+      // current_path_ may be empty because it is possible for DownloadItem to
+      // receive a DestinationError prior to the download file initialization
+      // complete callback.
+      if (!IsCancellation(reason)) {
+        UpdateProgress(bytes_so_far, 0);
+        SetHashState(std::move(hash_state));
+        deferred_interrupt_reason_ = reason;
+        TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+        return;
+      }
+    // else - Fallthrough for cancellation handling which is equivalent to the
+    // IN_PROGRESS state.
+
     case IN_PROGRESS_INTERNAL:
-    case TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
       // last_reason_ needs to be set for GetResumeMode() to work.
       last_reason_ = reason;
@@ -1650,8 +1676,7 @@ void DownloadItemImpl::InterruptWithPartialState(
       DCHECK(!download_file_);
       // 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)
+      if (!IsCancellation(reason))
         return;
 
       last_reason_ = reason;
@@ -1692,8 +1717,7 @@ void DownloadItemImpl::InterruptWithPartialState(
   if (job_)
     job_->Cancel(false);
 
-  if (reason == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED ||
-      reason == DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN) {
+  if (IsCancellation(reason)) {
     if (IsDangerous()) {
       RecordDangerousDownloadDiscard(
           reason == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
@@ -1703,6 +1727,7 @@ void DownloadItemImpl::InterruptWithPartialState(
     }
 
     RecordDownloadCount(CANCELLED_COUNT);
+    DCHECK_EQ(last_reason_, reason);
     TransitionTo(CANCELLED_INTERNAL);
     return;
   }
@@ -1711,6 +1736,10 @@ void DownloadItemImpl::InterruptWithPartialState(
   if (!GetWebContents())
     RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
 
+  // TODO(asanka): This is not good. We can transition to interrupted from
+  // target-pending, which is something we don't want to do. Perhaps we should
+  // explicitly transition to target-resolved prior to switching to interrupted.
+  DCHECK_EQ(last_reason_, reason);
   TransitionTo(INTERRUPTED_INTERNAL);
   AutoResumeIfValid();
 }
@@ -1857,6 +1886,9 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
       break;
 
     case INTERRUPTED_INTERNAL:
+      DCHECK(!download_file_)
+          << "Download file must be released prior to interruption.";
+      DCHECK_NE(last_reason_, DOWNLOAD_INTERRUPT_REASON_NONE);
       net_log_.AddEvent(net::NetLogEventType::DOWNLOAD_ITEM_INTERRUPTED,
                               base::Bind(&ItemInterruptedNetLogCallback,
                                          last_reason_, received_bytes_));
@@ -1966,6 +1998,7 @@ void DownloadItemImpl::ResumeInterruptedDownload(
   ResumeMode mode = GetResumeMode();
   if (mode == RESUME_MODE_IMMEDIATE_RESTART ||
       mode == RESUME_MODE_USER_RESTART) {
+    DCHECK(current_path_.empty());
     received_bytes_ = 0;
     last_modified_time_.clear();
     etag_.clear();
@@ -2107,7 +2140,7 @@ bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from,
              to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL;
 
     case INTERRUPTED_TARGET_PENDING_INTERNAL:
-      return to == INTERRUPTED_INTERNAL || to == CANCELLED_INTERNAL;
+      return to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL;
 
     case TARGET_RESOLVED_INTERNAL:
       return to == IN_PROGRESS_INTERNAL || to == INTERRUPTED_INTERNAL ||
@@ -2168,7 +2201,7 @@ const char* DownloadItemImpl::DebugDownloadStateString(
       return "RESUMING";
     case MAX_DOWNLOAD_INTERNAL_STATE:
       break;
-  };
+  }
   NOTREACHED() << "Unknown download state " << state;
   return "unknown";
 }