Index: content/browser/download/download_resource_handler.cc |
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc |
index ab9e19ef3df08ba3c7f79af4dce81caa8c5143c1..829884a567d060e1d6c0c0d506a767373ef9435d 100644 |
--- a/content/browser/download/download_resource_handler.cc |
+++ b/content/browser/download/download_resource_handler.cc |
@@ -34,20 +34,9 @@ |
namespace content { |
namespace { |
-void CallStartedCBOnUIThread( |
- const DownloadUrlParameters::OnStartedCallback& started_cb, |
- DownloadItem* item, |
- DownloadInterruptReason interrupt_reason) { |
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
- |
- if (started_cb.is_null()) |
- return; |
- started_cb.Run(item, interrupt_reason); |
-} |
- |
// Static function in order to prevent any accidental accesses to |
// DownloadResourceHandler members from the UI thread. |
Randy Smith (Not in Mondays)
2014/03/18 21:07:28
nit: This comment seems outdated; it implies that
asanka
2014/03/19 20:37:06
Removed comment.
|
-static void StartOnUIThread( |
+void StartOnUIThread( |
scoped_ptr<DownloadCreateInfo> info, |
scoped_ptr<ByteStreamReader> stream, |
const DownloadUrlParameters::OnStartedCallback& started_cb) { |
@@ -69,6 +58,37 @@ static void StartOnUIThread( |
download_manager->StartDownload(info.Pass(), stream.Pass(), started_cb); |
} |
+DownloadInterruptReason HTTPResponseCodeToInterruptReason(int response_code) { |
+ switch (response_code) { |
+ case -1: // Non-HTTP request. |
+ case net::HTTP_OK: |
+ case net::HTTP_CREATED: |
+ case net::HTTP_ACCEPTED: |
+ case net::HTTP_NON_AUTHORITATIVE_INFORMATION: |
+ case net::HTTP_RESET_CONTENT: |
+ case net::HTTP_PARTIAL_CONTENT: |
+ return DOWNLOAD_INTERRUPT_REASON_NONE; |
+ case net::HTTP_NO_CONTENT: |
+ case net::HTTP_NOT_FOUND: |
+ return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT; |
+ case net::HTTP_PRECONDITION_FAILED: |
+ // Failed our 'If-Unmodified-Since' or 'If-Match'; see |
+ // download_manager_impl.cc BeginDownload() |
+ return DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION; |
+ case net::HTTP_REQUESTED_RANGE_NOT_SATISFIABLE: |
+ // Retry by downloading from the start automatically: |
+ // If we haven't received data when we get this error, we won't. |
+ return DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE; |
+ // TODO(asanka): Add handling for 5xx errors with backoff semantics. |
+ default: // All other errors. |
+ // Redirection and informational codes should have been handled earlier |
+ // in the stack. |
+ DCHECK_NE(3, response_code / 100); |
+ DCHECK_NE(1, response_code / 100); |
+ return DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED; |
+ } |
+} |
+ |
} // namespace |
const int DownloadResourceHandler::kDownloadByteStreamSize = 100 * 1024; |
@@ -86,7 +106,7 @@ DownloadResourceHandler::DownloadResourceHandler( |
bytes_read_(0), |
pause_count_(0), |
was_deferred_(false), |
- on_response_started_called_(false) { |
+ download_manager_notified_(false) { |
RecordDownloadCount(UNTHROTTLED_COUNT); |
} |
@@ -113,10 +133,6 @@ bool DownloadResourceHandler::OnResponseStarted( |
ResourceResponse* response, |
bool* defer) { |
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); |
- // There can be only one (call) |
- DCHECK(!on_response_started_called_); |
- on_response_started_called_ = true; |
- |
VLOG(20) << __FUNCTION__ << "()" << DebugString() |
<< " request_id = " << request_id; |
download_start_time_ = base::TimeTicks::Now(); |
@@ -128,15 +144,35 @@ bool DownloadResourceHandler::OnResponseStarted( |
// with main frames. |
request()->SetPriority(net::IDLE); |
+ const net::HttpResponseHeaders* headers = request()->response_headers(); |
+ DownloadInterruptReason interrupt_reason = |
+ (headers ? HTTPResponseCodeToInterruptReason(headers->response_code()) |
+ : DOWNLOAD_INTERRUPT_REASON_NONE); |
+ |
+ if (interrupt_reason != DOWNLOAD_INTERRUPT_REASON_NONE) |
+ NotifyDownloadManagerOfFailedRequest(interrupt_reason); |
+ else |
+ NotifyDownloadManagerOfSuccessfulResponseStart(response); |
+ |
+ return true; |
+} |
+ |
+void DownloadResourceHandler::NotifyDownloadManagerOfSuccessfulResponseStart( |
+ ResourceResponse* response) { |
+ // NotifyDownloadManagerOfFailedRequest or |
+ // NotifyDownloadManagerOfSuccessfulResponseStart can only be called once. |
+ DCHECK(!download_manager_notified_); |
+ download_manager_notified_ = true; |
+ |
+ const ResourceRequestInfoImpl* request_info = GetRequestInfo(); |
+ const net::HttpResponseHeaders* headers = request()->response_headers(); |
+ |
// If the content-length header is not present (or contains something other |
// than numbers), the incoming content_length is -1 (unknown size). |
// Set the content length to 0 to indicate unknown size to DownloadManager. |
int64 content_length = |
response->head.content_length > 0 ? response->head.content_length : 0; |
Randy Smith (Not in Mondays)
2014/03/18 21:07:28
nit, question: It feels a little funny to be both
asanka
2014/03/19 20:37:06
The URLRequestJob provides the expected content le
|
- const ResourceRequestInfoImpl* request_info = GetRequestInfo(); |
- |
- // Deleted in DownloadManager. |
scoped_ptr<DownloadCreateInfo> info( |
new DownloadCreateInfo(base::Time::Now(), |
content_length, |
@@ -145,32 +181,6 @@ bool DownloadResourceHandler::OnResponseStarted( |
request_info->GetPageTransition(), |
save_info_.Pass())); |
- // Create the ByteStream for sending data to the download sink. |
- scoped_ptr<ByteStreamReader> stream_reader; |
- CreateByteStream( |
- base::MessageLoopProxy::current(), |
- BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE), |
- kDownloadByteStreamSize, &stream_writer_, &stream_reader); |
- stream_writer_->RegisterCallback( |
- base::Bind(&DownloadResourceHandler::ResumeRequest, AsWeakPtr())); |
- |
- info->download_id = download_id_; |
- info->url_chain = request()->url_chain(); |
- info->referrer_url = GURL(request()->referrer()); |
- info->mime_type = response->head.mime_type; |
- info->remote_address = request()->GetSocketAddress().host(); |
- request()->GetResponseHeaderByName("content-disposition", |
- &info->content_disposition); |
- RecordDownloadMimeType(info->mime_type); |
- RecordDownloadContentDisposition(info->content_disposition); |
- |
- info->request_handle = |
- DownloadRequestHandle(AsWeakPtr(), request_info->GetChildID(), |
- request_info->GetRouteID(), |
- request_info->GetRequestID()); |
- |
- // Get the last modified time and etag. |
- const net::HttpResponseHeaders* headers = request()->response_headers(); |
if (headers) { |
if (headers->HasStrongValidators()) { |
// If we don't have strong validators as per RFC 2616 section 13.3.3, then |
@@ -194,6 +204,33 @@ bool DownloadResourceHandler::OnResponseStarted( |
info->original_mime_type.clear(); |
} |
+ info->download_id = download_id_; |
+ info->url_chain = request()->url_chain(); |
+ info->referrer_url = GURL(request()->referrer()); |
+ |
+ scoped_ptr<ByteStreamReader> stream_reader; |
+ // Create the ByteStream for sending data to the download sink. |
+ CreateByteStream( |
+ base::MessageLoopProxy::current(), |
+ BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE), |
+ kDownloadByteStreamSize, |
+ &stream_writer_, |
+ &stream_reader); |
+ stream_writer_->RegisterCallback( |
+ base::Bind(&DownloadResourceHandler::ResumeRequest, AsWeakPtr())); |
+ |
+ info->mime_type = response->head.mime_type; |
+ info->remote_address = request()->GetSocketAddress().host(); |
+ request()->GetResponseHeaderByName("content-disposition", |
+ &info->content_disposition); |
Randy Smith (Not in Mondays)
2014/03/18 21:07:28
nit, suggestion: It makes some sense to me to swit
asanka
2014/03/19 20:37:06
Done.
|
+ RecordDownloadMimeType(info->mime_type); |
+ RecordDownloadContentDisposition(info->content_disposition); |
+ |
+ info->request_handle = DownloadRequestHandle(AsWeakPtr(), |
+ request_info->GetChildID(), |
+ request_info->GetRouteID(), |
+ request_info->GetRequestID()); |
+ |
BrowserThread::PostTask( |
BrowserThread::UI, FROM_HERE, |
base::Bind(&StartOnUIThread, |
@@ -205,21 +242,42 @@ bool DownloadResourceHandler::OnResponseStarted( |
started_cb_)); |
// Guaranteed to be called in StartOnUIThread |
started_cb_.Reset(); |
- |
- return true; |
} |
-void DownloadResourceHandler::CallStartedCB( |
- DownloadItem* item, |
+void DownloadResourceHandler::NotifyDownloadManagerOfFailedRequest( |
DownloadInterruptReason interrupt_reason) { |
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); |
- if (started_cb_.is_null()) |
- return; |
- BrowserThread::PostTask( |
- BrowserThread::UI, |
- FROM_HERE, |
- base::Bind( |
- &CallStartedCBOnUIThread, started_cb_, item, interrupt_reason)); |
+ // NotifyDownloadManagerOfFailedRequest or |
+ // NotifyDownloadManagerOfSuccessfulResponseStart can only be called once. |
+ DCHECK(!download_manager_notified_); |
+ DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason); |
+ download_manager_notified_ = true; |
+ |
+ const ResourceRequestInfoImpl* request_info = GetRequestInfo(); |
+ |
+ scoped_ptr<DownloadCreateInfo> info( |
+ new DownloadCreateInfo(base::Time::Now(), |
+ -1, |
+ request()->net_log(), |
+ false, |
+ request_info->GetPageTransition(), |
+ save_info_.Pass())); |
+ info->download_id = download_id_; |
+ info->url_chain = request()->url_chain(); |
+ info->referrer_url = GURL(request()->referrer()); |
+ info->interrupt_reason = interrupt_reason; |
+ info->remote_address = request()->GetSocketAddress().host(); |
+ info->request_handle = DownloadRequestHandle(AsWeakPtr(), |
+ request_info->GetChildID(), |
+ request_info->GetRouteID(), |
+ request_info->GetRequestID()); |
+ |
+ scoped_ptr<ByteStreamReader> stream_reader; |
+ BrowserThread::PostTask(BrowserThread::UI, |
+ FROM_HERE, |
+ base::Bind(&StartOnUIThread, |
+ base::Passed(&info), |
+ base::Passed(&stream_reader), |
Randy Smith (Not in Mondays)
2014/03/18 21:07:28
nit: I think it'd be clearer to cons up a null one
asanka
2014/03/19 20:37:06
Done.
|
+ started_cb_)); |
started_cb_.Reset(); |
} |
@@ -277,6 +335,12 @@ bool DownloadResourceHandler::OnReadCompleted(int request_id, int bytes_read, |
bytes_read_ += bytes_read; |
DCHECK(read_buffer_.get()); |
+ // If this was an error response, then stream_writer_ would be NULL. |
+ if (!stream_writer_) { |
+ read_buffer_ = NULL; |
+ return true; |
+ } |
+ |
// Take the data ship it down the stream. If the stream is full, pause the |
// request; the stream callback will resume it. |
if (!stream_writer_->Write(read_buffer_, bytes_read)) { |
@@ -341,44 +405,6 @@ void DownloadResourceHandler::OnResponseCompleted( |
reason = DOWNLOAD_INTERRUPT_REASON_USER_CANCELED; |
} |
- if (status.is_success() && |
- reason == DOWNLOAD_INTERRUPT_REASON_NONE && |
- request()->response_headers()) { |
- // Handle server's response codes. |
- switch(response_code) { |
- case -1: // Non-HTTP request. |
- case net::HTTP_OK: |
- case net::HTTP_CREATED: |
- case net::HTTP_ACCEPTED: |
- case net::HTTP_NON_AUTHORITATIVE_INFORMATION: |
- case net::HTTP_RESET_CONTENT: |
- case net::HTTP_PARTIAL_CONTENT: |
- // Expected successful codes. |
- break; |
- case net::HTTP_NO_CONTENT: |
- case net::HTTP_NOT_FOUND: |
- reason = DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT; |
- break; |
- case net::HTTP_PRECONDITION_FAILED: |
- // Failed our 'If-Unmodified-Since' or 'If-Match'; see |
- // download_manager_impl.cc BeginDownload() |
- reason = DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION; |
- break; |
- case net::HTTP_REQUESTED_RANGE_NOT_SATISFIABLE: |
- // Retry by downloading from the start automatically: |
- // If we haven't received data when we get this error, we won't. |
- reason = DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE; |
- break; |
- default: // All other errors. |
- // Redirection and informational codes should have been handled earlier |
- // in the stack. |
- DCHECK_NE(3, response_code / 100); |
- DCHECK_NE(1, response_code / 100); |
- reason = DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED; |
- break; |
- } |
- } |
- |
std::string accept_ranges; |
bool has_strong_validators = false; |
if (request()->response_headers()) { |
@@ -391,12 +417,19 @@ void DownloadResourceHandler::OnResponseCompleted( |
RecordNetworkBlockage(base::TimeTicks::Now() - download_start_time_, |
total_pause_time_); |
- CallStartedCB(NULL, reason); |
- |
- // Send the info down the stream. Conditional is in case we get |
- // OnResponseCompleted without OnResponseStarted. |
- if (stream_writer_) |
+ // If OnResponseCompleted() is called without a corresponding |
+ // OnResponseStarted() call, then the DownloadManager wouldn't have been |
+ // notified. |
+ if (!download_manager_notified_ && |
+ status.status() != net::URLRequestStatus::CANCELED) { |
+ DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason); |
+ NotifyDownloadManagerOfFailedRequest(reason); |
+ } else if (stream_writer_) { |
+ // Send the info down the stream. Conditional is in case we get |
+ // OnResponseCompleted without OnResponseStarted. |
stream_writer_->Close(reason); |
+ stream_writer_.reset(); // We no longer need the stream. |
+ } |
// If the error mapped to something unknown, record it so that |
// we can drill down. |
@@ -406,7 +439,6 @@ void DownloadResourceHandler::OnResponseCompleted( |
net::GetAllErrorCodesForUma()); |
} |
- stream_writer_.reset(); // We no longer need the stream. |
read_buffer_ = NULL; |
} |
@@ -473,10 +505,22 @@ std::string DownloadResourceHandler::DebugString() const { |
DownloadResourceHandler::~DownloadResourceHandler() { |
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); |
- // This won't do anything if the callback was called before. |
- // If it goes through, it will likely be because OnWillStart() returned |
- // false somewhere in the chain of resource handlers. |
- CallStartedCB(NULL, DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED); |
+ // DownloadManager would not be notified if: |
+ // |
+ // - The Content-Type or Content-Disposition indicated that the entity should |
+ // be downloaded, but the response code was not 2xx. BufferedResourceHandler |
+ // will create a DownloadResourceHandler, but won't invoke it. |
+ // TODO(asanka): Fix this so that ResourceHandlers are only created if they |
+ // are going to be used. |
+ // |
+ // - OnWillStart() returned false somewhere in the chain of resource handlers. |
+ if (!download_manager_notified_ && !started_cb_.is_null()) |
Randy Smith (Not in Mondays)
2014/03/18 21:07:28
DCHECK opportunity? Shouldn't these two condition
asanka
2014/03/19 20:37:06
After verifying the OnWillStart() pathway, I decid
|
+ BrowserThread::PostTask( |
+ BrowserThread::UI, |
+ FROM_HERE, |
+ base::Bind(started_cb_, |
Randy Smith (Not in Mondays)
2014/03/18 21:07:28
Is the eventual intent to get rid of started_cb?
asanka
2014/03/19 20:37:06
Yeah. I'd like to get rid of started_cb_. I added
|
+ static_cast<DownloadItem*>(NULL), |
+ DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED)); |
// Remove output stream callback if a stream exists. |
if (stream_writer_) |