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 d1482bc95fd9dbf182413da766c570e934b32dd3..75858d102ce77bec9147385f0f2dff4731ab0770 100644 |
--- a/content/browser/download/download_item_impl.cc |
+++ b/content/browser/download/download_item_impl.cc |
@@ -192,7 +192,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate, |
bytes_per_sec_(0), |
last_modified_time_(info.last_modified), |
etag_(info.etag), |
- last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE), |
+ last_reason_(info.result), |
start_tick_(base::TimeTicks::Now()), |
state_(INITIAL_INTERNAL), |
danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS), |
@@ -205,7 +205,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate, |
auto_opened_(false), |
is_temporary_(!info.save_info->file_path.empty()), |
all_data_saved_(false), |
- destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE), |
+ destination_error_(DOWNLOAD_INTERRUPT_REASON_NONE), |
opened_(false), |
delegate_delayed_complete_(false), |
bound_net_log_(bound_net_log), |
@@ -361,6 +361,7 @@ void DownloadItemImpl::Pause() { |
return; |
case TARGET_PENDING_INTERNAL: |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
case TARGET_RESOLVED_INTERNAL: |
case IN_PROGRESS_INTERNAL: |
request_handle_->PauseRequest(); |
@@ -375,6 +376,7 @@ void DownloadItemImpl::Pause() { |
void DownloadItemImpl::Resume() { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); |
switch (state_) { |
case INITIAL_INTERNAL: |
case COMPLETING_INTERNAL: |
@@ -386,6 +388,7 @@ void DownloadItemImpl::Resume() { |
return; |
case TARGET_PENDING_INTERNAL: |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
case TARGET_RESOLVED_INTERNAL: |
case IN_PROGRESS_INTERNAL: |
if (!is_paused_) |
@@ -398,6 +401,7 @@ void DownloadItemImpl::Resume() { |
case INTERRUPTED_INTERNAL: |
auto_resume_count_ = 0; // User input resets the counter. |
ResumeInterruptedDownload(); |
+ UpdateObservers(); |
return; |
case MAX_DOWNLOAD_INTERNAL_STATE: |
@@ -410,6 +414,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) { |
DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); |
Interrupt(user_cancel ? DOWNLOAD_INTERRUPT_REASON_USER_CANCELED |
: DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN); |
+ UpdateObservers(); |
} |
void DownloadItemImpl::Remove() { |
@@ -487,6 +492,7 @@ bool DownloadItemImpl::CanResume() const { |
return false; |
case TARGET_PENDING_INTERNAL: |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
case TARGET_RESOLVED_INTERNAL: |
case IN_PROGRESS_INTERNAL: |
return is_paused_; |
@@ -512,6 +518,7 @@ bool DownloadItemImpl::IsDone() const { |
case COMPLETING_INTERNAL: |
case RESUMING_INTERNAL: |
case TARGET_PENDING_INTERNAL: |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
case TARGET_RESOLVED_INTERNAL: |
case IN_PROGRESS_INTERNAL: |
return false; |
@@ -936,7 +943,7 @@ DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const { |
return mode; |
} |
-void DownloadItemImpl::MergeOriginInfoOnResume( |
+void DownloadItemImpl::UpdateValidatorsOnResumption( |
const DownloadCreateInfo& new_create_info) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
DCHECK_EQ(RESUMING_INTERNAL, state_); |
@@ -1023,7 +1030,8 @@ void DownloadItemImpl::MarkAsComplete() { |
DCHECK(all_data_saved_); |
end_time_ = base::Time::Now(); |
- TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS); |
+ TransitionTo(COMPLETE_INTERNAL); |
+ UpdateObservers(); |
} |
void DownloadItemImpl::DestinationUpdate(int64_t bytes_so_far, |
@@ -1072,10 +1080,12 @@ void DownloadItemImpl::DestinationError(DownloadInterruptReason 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_ != IN_PROGRESS_INTERNAL) |
+ if (state_ != IN_PROGRESS_INTERNAL) { |
destination_error_ = reason; |
- else |
- Interrupt(reason); |
+ return; |
+ } |
+ Interrupt(reason); |
+ UpdateObservers(); |
} |
void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) { |
@@ -1124,11 +1134,10 @@ void DownloadItemImpl::Init(bool active, |
// We're starting the download. |
void DownloadItemImpl::Start( |
scoped_ptr<DownloadFile> file, |
- scoped_ptr<DownloadRequestHandleInterface> req_handle) { |
+ scoped_ptr<DownloadRequestHandleInterface> req_handle, |
+ const DownloadCreateInfo& new_create_info) { |
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); |
@@ -1138,11 +1147,49 @@ void DownloadItemImpl::Start( |
// The download was in the process of resuming when it was cancelled. Don't |
// proceed. |
ReleaseDownloadFile(true); |
- request_handle_->CancelRequest(); |
+ if (request_handle_) |
+ request_handle_->CancelRequest(); |
return; |
} |
- TransitionTo(TARGET_PENDING_INTERNAL, UPDATE_OBSERVERS); |
+ // The state could be one of the following: |
+ // |
+ // INITIAL_INTERNAL: A normal download attempt. |
+ // |
+ // RESUMING_INTERNAL: A resumption attempt. May or may not have been |
+ // successful. |
+ DCHECK(state_ == INITIAL_INTERNAL || state_ == RESUMING_INTERNAL); |
+ |
+ // If a resumption attempted failed, or if the download was DOA, then the |
+ // download should go back to being interrupted. |
+ if (new_create_info.result != DOWNLOAD_INTERRUPT_REASON_NONE) { |
+ DCHECK(!download_file_.get()); |
+ |
+ // For downloads that are interrupted on initiation, invoke target |
+ // determination so that we'll end up with a usable filename. |
+ if (state_ == INITIAL_INTERNAL) { |
+ TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL); |
+ destination_error_ = new_create_info.result; |
+ DetermineDownloadTarget(); |
+ return; |
+ } |
+ |
+ // Otherwise, this was a resumption attempt which ended with an |
+ // interruption. |
+ Interrupt(new_create_info.result); |
+ UpdateObservers(); |
+ return; |
+ } |
+ |
+ // Successful download start. |
+ DCHECK(download_file_.get()); |
+ DCHECK(request_handle_.get()); |
+ |
+ if (state_ == RESUMING_INTERNAL) |
+ UpdateValidatorsOnResumption(new_create_info); |
+ |
+ TransitionTo(TARGET_PENDING_INTERNAL); |
+ UpdateObservers(); |
BrowserThread::PostTask( |
BrowserThread::FILE, FROM_HERE, |
@@ -1160,35 +1207,34 @@ void DownloadItemImpl::OnDownloadFileInitialized( |
DVLOG(20) << __FUNCTION__ |
<< "() result:" << DownloadInterruptReasonToString(result); |
if (result != DOWNLOAD_INTERRUPT_REASON_NONE) { |
- // Transition out to IN_PROGRESS since this DownloadItem is skipping the |
- // download target determination process. |
- 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; |
+ // Whoops. That didn't work. Proceed as an interrupted download. Update the |
+ // observers since this is an externally visible transition. |
+ destination_error_ = result; |
+ TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL); |
+ UpdateObservers(); |
} |
+ DetermineDownloadTarget(); |
+} |
+ |
+void DownloadItemImpl::DetermineDownloadTarget() { |
+ DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ DVLOG(20) << __FUNCTION__ << "() " << DebugString(true); |
+ |
delegate_->DetermineDownloadTarget( |
this, base::Bind(&DownloadItemImpl::OnDownloadTargetDetermined, |
weak_ptr_factory_.GetWeakPtr())); |
} |
-// Called by delegate_ when the download target path has been |
-// determined. |
+// 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); |
+ DCHECK(state_ == TARGET_PENDING_INTERNAL || |
+ state_ == INTERRUPTED_TARGET_PENDING_INTERNAL); |
// If the |target_path| is empty, then we consider this download to be |
// canceled. |
@@ -1197,14 +1243,6 @@ void DownloadItemImpl::OnDownloadTargetDetermined( |
return; |
} |
- // TODO(rdsmith,asanka): We are ignoring the possibility that the download |
- // has been interrupted at this point until we finish the intermediate |
- // rename and set the full path. That's dangerous, because we might race |
- // with resumption, either manual (because the interrupt is visible to the |
- // UI) or automatic. If we keep the "ignore an error on download until file |
- // name determination complete" semantics, we need to make sure that the |
- // error is kept completely invisible until that point. |
- |
DVLOG(20) << __FUNCTION__ << " " << target_path.value() << " " << disposition |
<< " " << danger_type << " " << DebugString(true); |
@@ -1212,6 +1250,14 @@ void DownloadItemImpl::OnDownloadTargetDetermined( |
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) { |
+ Interrupt(destination_error_); |
+ UpdateObservers(); |
+ return; |
+ } |
+ |
// We want the intermediate and target paths to refer to the same directory so |
// that they are both on the same device and subject to same |
// space/permission/availability constraints. |
@@ -1254,7 +1300,7 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName( |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
DCHECK_EQ(state_, TARGET_PENDING_INTERNAL); |
DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true); |
- TransitionTo(TARGET_RESOLVED_INTERNAL, DONT_UPDATE_OBSERVERS); |
+ TransitionTo(TARGET_RESOLVED_INTERNAL); |
if (DOWNLOAD_INTERRUPT_REASON_NONE != destination_error_) { |
// Process destination error. If both |reason| and |destination_error_| |
@@ -1272,9 +1318,10 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName( |
DCHECK(current_path_.empty()); |
} else { |
SetFullPath(full_path); |
- TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS); |
+ TransitionTo(IN_PROGRESS_INTERNAL); |
MaybeCompleteDownload(); |
} |
+ UpdateObservers(); |
} |
// When SavePackage downloads MHTML to GData (see |
@@ -1320,9 +1367,8 @@ void DownloadItemImpl::OnDownloadCompleting() { |
// TODO(rdsmith/benjhayden): Remove as part of SavePackage integration. |
if (is_save_package_download_) { |
// Avoid doing anything on the file thread; there's nothing we control |
- // there. |
- // Strictly speaking, this skips giving the embedder a chance to open |
- // the download. But on a save package download, there's no real |
+ // there. Strictly speaking, this skips giving the embedder a chance to |
+ // open the download. But on a save package download, there's no real |
// concept of opening. |
Completed(); |
return; |
@@ -1363,6 +1409,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName( |
// All file errors should have resulted in in file deletion above. On |
// resumption we will need to re-do filename determination. |
DCHECK(current_path_.empty()); |
+ UpdateObservers(); |
return; |
} |
@@ -1382,7 +1429,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName( |
// 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); |
+ TransitionTo(COMPLETING_INTERNAL); |
if (delegate_->ShouldOpenDownload( |
this, base::Bind(&DownloadItemImpl::DelayedDownloadOpened, |
@@ -1408,7 +1455,8 @@ void DownloadItemImpl::Completed() { |
DCHECK(all_data_saved_); |
end_time_ = base::Time::Now(); |
- TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS); |
+ TransitionTo(COMPLETE_INTERNAL); |
+ UpdateObservers(); |
RecordDownloadCompleted(start_tick_, received_bytes_); |
if (auto_opened_) { |
@@ -1427,22 +1475,6 @@ void DownloadItemImpl::Completed() { |
} |
} |
-void DownloadItemImpl::OnResumeRequestStarted( |
- DownloadItem* item, |
- DownloadInterruptReason interrupt_reason) { |
- // If |item| is not NULL, then Start() has been called already, and nothing |
- // more needs to be done here. |
- if (item) { |
- DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason); |
- DCHECK_EQ(static_cast<DownloadItem*>(this), item); |
- return; |
- } |
- // Otherwise, the request failed without passing through |
- // DownloadResourceHandler::OnResponseStarted. |
- DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason); |
- Interrupt(interrupt_reason); |
-} |
- |
// **** End of Download progression cascade |
// An error occurred somewhere. |
@@ -1476,6 +1508,7 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { |
break; |
case RESUMING_INTERNAL: |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
case INTERRUPTED_INTERNAL: |
// The first non-cancel interrupt reason wins in cases where multiple |
// things go wrong. |
@@ -1530,16 +1563,16 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { |
} |
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(); |
+ TransitionTo(CANCELLED_INTERNAL); |
asanka
2016/02/11 23:57:25
Needs an UpdateObservers() call.
Randy Smith (Not in Mondays)
2016/02/12 18:58:10
This looks like a note you made to yourself that y
asanka
2016/02/12 20:52:28
Stale. We've relieved Interrupt() of the responsib
|
+ return; |
} |
- UpdateObservers(); |
+ RecordDownloadInterrupted(reason, received_bytes_, total_bytes_); |
+ if (!GetWebContents()) |
+ RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS); |
+ |
+ TransitionTo(INTERRUPTED_INTERNAL); |
+ AutoResumeIfValid(); |
} |
void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) { |
@@ -1585,8 +1618,10 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion( |
if (IsDangerous()) |
return false; |
- // Invariants for the IN_PROGRESS state. DCHECKs here verify that the |
- // invariants are still true. |
+ // Check for consistency before invoking delegate. Since there are no pending |
+ // target determination calls and the download is in progress, both the target |
+ // and current paths should be non-empty and they should point to the same |
+ // directory. |
DCHECK(!target_path_.empty()); |
DCHECK(!current_path_.empty()); |
DCHECK(target_path_.DirName() == current_path_.DirName()); |
@@ -1599,8 +1634,7 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion( |
return true; |
} |
-void DownloadItemImpl::TransitionTo(DownloadInternalState new_state, |
- ShouldUpdateObservers notify_action) { |
+void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
if (state_ == new_state) |
@@ -1622,6 +1656,7 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state, |
case TARGET_PENDING_INTERNAL: |
case TARGET_RESOLVED_INTERNAL: |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
break; |
case IN_PROGRESS_INTERNAL: |
@@ -1704,9 +1739,6 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state, |
this, SRC_ACTIVE_DOWNLOAD, |
&file_name)); |
} |
- |
- if (notify_action == UPDATE_OBSERVERS) |
- UpdateObservers(); |
} |
void DownloadItemImpl::SetDangerType(DownloadDangerType danger_type) { |
@@ -1768,6 +1800,10 @@ void DownloadItemImpl::ResumeInterruptedDownload() { |
if (state_ != INTERRUPTED_INTERNAL) |
return; |
+ // We are starting a new request. Shake off all pending operations. |
+ DCHECK(!download_file_); |
+ weak_ptr_factory_.InvalidateWeakPtrs(); |
asanka
2016/02/11 23:57:25
Perhaps we should do this when the previous reques
Randy Smith (Not in Mondays)
2016/02/12 18:58:10
I think we do (given that Interrupt() does a Relea
asanka
2016/02/12 20:52:28
This was a note to self to check. It is possible t
|
+ |
// Reset the appropriate state if restarting. |
ResumeMode mode = GetResumeMode(); |
if (mode == RESUME_MODE_IMMEDIATE_RESTART || |
@@ -1792,11 +1828,8 @@ void DownloadItemImpl::ResumeInterruptedDownload() { |
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); |
+ TransitionTo(RESUMING_INTERNAL); |
delegate_->ResumeInterruptedDownload(std::move(download_params), GetId()); |
// Just in case we were interrupted while paused. |
is_paused_ = false; |
@@ -1821,6 +1854,7 @@ DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState( |
return COMPLETE; |
case CANCELLED_INTERNAL: |
return CANCELLED; |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
case INTERRUPTED_INTERNAL: |
return INTERRUPTED; |
case RESUMING_INTERNAL: |
@@ -1859,6 +1893,7 @@ bool DownloadItemImpl::IsValidSavePackageStateTransition( |
switch (from) { |
case INITIAL_INTERNAL: |
case TARGET_PENDING_INTERNAL: |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
case TARGET_RESOLVED_INTERNAL: |
case COMPLETING_INTERNAL: |
case COMPLETE_INTERNAL: |
@@ -1881,10 +1916,15 @@ bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from, |
DownloadInternalState to) { |
switch (from) { |
case INITIAL_INTERNAL: |
- return to == TARGET_PENDING_INTERNAL || to == INTERRUPTED_INTERNAL; |
+ return to == TARGET_PENDING_INTERNAL || |
+ to == INTERRUPTED_TARGET_PENDING_INTERNAL; |
case TARGET_PENDING_INTERNAL: |
- return to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL; |
+ return to == INTERRUPTED_TARGET_PENDING_INTERNAL || |
+ to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL; |
+ |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
+ return to == INTERRUPTED_INTERNAL || to == CANCELLED_INTERNAL; |
case TARGET_RESOLVED_INTERNAL: |
return to == IN_PROGRESS_INTERNAL || to == INTERRUPTED_INTERNAL || |
@@ -1904,7 +1944,9 @@ bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from, |
return to == RESUMING_INTERNAL || to == CANCELLED_INTERNAL; |
case RESUMING_INTERNAL: |
- return to == TARGET_PENDING_INTERNAL || to == CANCELLED_INTERNAL; |
+ return to == TARGET_PENDING_INTERNAL || |
+ to == INTERRUPTED_TARGET_PENDING_INTERNAL || |
+ to == CANCELLED_INTERNAL; |
case CANCELLED_INTERNAL: |
return false; |
@@ -1923,6 +1965,8 @@ const char* DownloadItemImpl::DebugDownloadStateString( |
return "INITIAL"; |
case TARGET_PENDING_INTERNAL: |
return "TARGET_PENDING"; |
+ case INTERRUPTED_TARGET_PENDING_INTERNAL: |
+ return "INTERRUPTED_TARGET_PENDING"; |
case TARGET_RESOLVED_INTERNAL: |
return "TARGET_RESOLVED"; |
case IN_PROGRESS_INTERNAL: |