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 cef57731792e96e8832ee0c0962b58a5d0f75b8b..cb223de575ed1263ad56150508a502ed2d9433fd 100644 |
--- a/content/browser/download/download_item_impl.cc |
+++ b/content/browser/download/download_item_impl.cc |
@@ -191,9 +191,11 @@ 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_(IN_PROGRESS_INTERNAL), |
+ state_(info.result == DOWNLOAD_INTERRUPT_REASON_NONE |
+ ? IN_PROGRESS_INTERNAL |
+ : INTERRUPTED_INTERNAL), |
danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS), |
start_time_(info.start_time), |
delegate_(delegate), |
@@ -204,13 +206,14 @@ 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), |
weak_ptr_factory_(this) { |
delegate_->Attach(); |
- Init(true /* actively downloading */, SRC_ACTIVE_DOWNLOAD); |
+ Init(state_ == IN_PROGRESS_INTERNAL /* actively downloading */, |
+ SRC_ACTIVE_DOWNLOAD); |
// Link the event sources. |
bound_net_log_.AddEvent( |
@@ -355,6 +358,7 @@ void DownloadItemImpl::Pause() { |
void DownloadItemImpl::Resume() { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); |
switch (state_) { |
case IN_PROGRESS_INTERNAL: |
if (!is_paused_) |
@@ -373,6 +377,7 @@ void DownloadItemImpl::Resume() { |
case INTERRUPTED_INTERNAL: |
auto_resume_count_ = 0; // User input resets the counter. |
ResumeInterruptedDownload(); |
+ UpdateObservers(); |
return; |
case MAX_DOWNLOAD_INTERNAL_STATE: |
@@ -925,31 +930,19 @@ 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_); |
- DCHECK(!new_create_info.url_chain.empty()); |
- |
- // We are going to tack on any new redirects to our list of redirects. |
- // When a download is resumed, the URL used for the resumption request is the |
- // one at the end of the previous redirect chain. Tacking additional redirects |
- // to the end of this chain ensures that: |
- // - If the download needs to be resumed again, the ETag/Last-Modified headers |
- // will be used with the last server that sent them to us. |
- // - The redirect chain contains all the servers that were involved in this |
- // download since the initial request, in order. |
- std::vector<GURL>::const_iterator chain_iter = |
- new_create_info.url_chain.begin(); |
- if (*chain_iter == url_chain_.back()) |
- ++chain_iter; |
+ |
+ // Redirects are disallowed for a resumption request. |
+ DCHECK_EQ(1u, new_create_info.url_chain.size()); |
+ DCHECK_EQ(url_chain_.back(), new_create_info.url_chain.back()); |
Randy Smith (Not in Mondays)
2016/02/10 21:48:45
So I could be misunderstand the code, but it seems
asanka
2016/02/11 03:43:07
This is asserted by DownloadRequestCore which inte
|
// Record some stats. If the precondition failed (the server returned |
// HTTP_PRECONDITION_FAILED), then the download will automatically retried as |
// a full request rather than a partial. Full restarts clobber validators. |
int origin_state = 0; |
- if (chain_iter != new_create_info.url_chain.end()) |
- origin_state |= ORIGIN_STATE_ON_RESUMPTION_ADDITIONAL_REDIRECTS; |
if (etag_ != new_create_info.etag || |
last_modified_time_ != new_create_info.last_modified) |
origin_state |= ORIGIN_STATE_ON_RESUMPTION_VALIDATORS_CHANGED; |
@@ -958,8 +951,6 @@ void DownloadItemImpl::MergeOriginInfoOnResume( |
RecordOriginStateOnResumption(new_create_info.save_info->offset != 0, |
origin_state); |
- url_chain_.insert( |
- url_chain_.end(), chain_iter, new_create_info.url_chain.end()); |
etag_ = new_create_info.etag; |
last_modified_time_ = new_create_info.last_modified; |
content_disposition_ = new_create_info.content_disposition; |
@@ -1060,7 +1051,7 @@ void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) { |
if (current_path_.empty() || target_path_.empty()) |
destination_error_ = reason; |
else |
- Interrupt(reason); |
+ Interrupt(reason, UPDATE_OBSERVERS); |
} |
void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) { |
@@ -1111,11 +1102,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()); |
download_file_ = std::move(file); |
request_handle_ = std::move(req_handle); |
@@ -1128,6 +1118,53 @@ void DownloadItemImpl::Start( |
return; |
} |
+ // The state could be one of the following: |
+ // |
+ // IN_PROGRESS_INTERNAL: Download started normally. |
+ // |
+ // RESUMING_INTERNAL: A resumption attempt. May or may not have been |
+ // successful. |
+ // |
+ // INTERRUPTED_INTERNAL: Download was DOA. (E.g. a programmatic download that |
+ // encountered a network error and didn't progress to the point where a |
+ // server response was available, or the server responded with an error). |
+ DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == RESUMING_INTERNAL || |
+ state_ == INTERRUPTED_INTERNAL); |
+ |
+ // If a resumption attempted failed, then the download goes back to being |
+ // interrupted. |
+ if (state_ == RESUMING_INTERNAL && |
+ new_create_info.result != DOWNLOAD_INTERRUPT_REASON_NONE) { |
+ DCHECK(!download_file_.get()); |
+ |
+ Interrupt(new_create_info.result, DONT_UPDATE_OBSERVERS); |
+ |
+ // Transitioning to interrupted may trigger another resumption attempt. If |
+ // so, Start() will be called again once a response is available for that |
+ // request. Nothing more needs to be done now. Note that Start() is never |
+ // called synchronously. |
+ if (state_ == RESUMING_INTERNAL) |
+ return; |
+ |
+ // Otherwise, the download should now be interrupted. |
+ DCHECK_EQ(INTERRUPTED_INTERNAL, state_); |
+ } |
+ |
+ if (state_ == INTERRUPTED_INTERNAL) { |
+ DCHECK(!download_file_.get()); |
+ // If the download failed, then skip the download_file_ initialization. It |
+ // isn't going to be used anyway. |
+ DetermineDownloadTarget(); |
+ return; |
+ } |
+ |
+ // Successful download start. |
+ DCHECK(download_file_.get()); |
+ DCHECK(request_handle_.get()); |
+ |
+ if (state_ == RESUMING_INTERNAL) |
+ UpdateValidatorsOnResumption(new_create_info); |
+ |
TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS); |
BrowserThread::PostTask( |
@@ -1143,18 +1180,22 @@ void DownloadItemImpl::OnDownloadFileInitialized( |
DownloadInterruptReason result) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
if (result != DOWNLOAD_INTERRUPT_REASON_NONE) { |
- 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; |
+ // The download file failed to initialize. |
+ Interrupt(result, DONT_UPDATE_OBSERVERS); |
+ |
+ // The download may transition to RESUMING_INTERNAL as a result of the |
+ // Interrupt() call above if it was resumed automatically. If so, then |
+ // Start() is expected to be called again and the download progression |
+ // cascade will be redone. Therefore we are done for now. Start() is never |
+ // called synchronously. |
+ if (state_ == RESUMING_INTERNAL) |
+ return; |
} |
+ DetermineDownloadTarget(); |
Randy Smith (Not in Mondays)
2016/02/05 00:51:41
Random thought: Reading through this code, I'm str
asanka
2016/02/11 03:43:07
This is true. I'm also tempted to pull out the dow
|
+} |
+ |
+void DownloadItemImpl::DetermineDownloadTarget() { |
delegate_->DetermineDownloadTarget( |
this, base::Bind(&DownloadItemImpl::OnDownloadTargetDetermined, |
weak_ptr_factory_.GetWeakPtr())); |
@@ -1176,20 +1217,21 @@ 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); |
target_path_ = target_path; |
target_disposition_ = disposition; |
SetDangerType(danger_type); |
+ if (state_ != IN_PROGRESS_INTERNAL) { |
+ DCHECK(!download_file_); |
+ // Already interrupted or cancelled. Since there's now a filename for |
+ // display purposes, observers can be safely notified of the new [failed] |
+ // download. Nothing more needs to be done since there's no active request |
+ // or in-use file. |
Randy Smith (Not in Mondays)
2016/02/05 00:51:41
Possible alternative implementation: Create a new
asanka
2016/02/11 03:43:07
Yup. That's what I ended up doing, partly to deal
|
+ 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 |
@@ -1239,10 +1281,10 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName( |
// argument to the Interrupt() routine, as it happened first. |
if (reason == DOWNLOAD_INTERRUPT_REASON_NONE) |
SetFullPath(full_path); |
- Interrupt(destination_error_); |
+ Interrupt(destination_error_, UPDATE_OBSERVERS); |
destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE; |
} else if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) { |
- Interrupt(reason); |
+ Interrupt(reason, UPDATE_OBSERVERS); |
// 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. |
@@ -1339,7 +1381,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName( |
<< " " << DebugString(false); |
if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) { |
- Interrupt(reason); |
+ Interrupt(reason, UPDATE_OBSERVERS); |
// All file errors should have resulted in in file deletion above. On |
// resumption we will need to re-do filename determination. |
@@ -1408,28 +1450,16 @@ 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. |
-void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { |
+void DownloadItemImpl::Interrupt( |
+ DownloadInterruptReason reason, |
+ ShouldUpdateObservers should_update_observers) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason); |
+ DVLOG(20) << __FUNCTION__ |
+ << "() reason=" << DownloadInterruptReasonToString(reason); |
// Somewhat counter-intuitively, it is possible for us to receive an |
// interrupt after we've already been interrupted. The generation of |
@@ -1484,7 +1514,8 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { |
RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS); |
AutoResumeIfValid(); |
- UpdateObservers(); |
+ if (should_update_observers == UPDATE_OBSERVERS) |
+ UpdateObservers(); |
} |
void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) { |
@@ -1676,6 +1707,25 @@ 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(); |
+ |
+ TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS); |
+ BrowserThread::PostTask( |
+ BrowserThread::UI, FROM_HERE, |
+ base::Bind(&DownloadItemImpl::InvokeDelegateForResumption, |
+ weak_ptr_factory_.GetWeakPtr())); |
+} |
+ |
+void DownloadItemImpl::InvokeDelegateForResumption() { |
Randy Smith (Not in Mondays)
2016/02/05 00:51:41
Why replacing the sync invocation with an async in
asanka
2016/02/11 03:43:07
This CL relies on Start() never being called synch
|
+ DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ // A state change happend while we were out. This could be because of a |
+ // Cancel() call. Since we aren't resuming anymore, there's nothing else to |
+ // do. |
+ if (state_ != RESUMING_INTERNAL) |
+ return; |
+ |
// Reset the appropriate state if restarting. |
ResumeMode mode = GetResumeMode(); |
if (mode == RESUME_MODE_IMMEDIATE_RESTART || |
@@ -1700,15 +1750,10 @@ 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())); |
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 |