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"; |
} |