Skip to content

Commit

Permalink
Allow fetchers to watch URLRequestContextGetters for context shutdown.
Browse files Browse the repository at this point in the history
Fetchers outliving the context they're using has been the main cause of
AssertNoURLRequests crashes.  This will let the ~26
URLRequestContextGetter implementations be responsible for dealing with
lifetime issues, rather than their 160+ consumers.

URLRequestContextGetter subclasses still need to be modified to actually
inform the fetcher of URLRequestContext shutdown.

BUG=471069

Review URL: https://codereview.chromium.org/1100603002

Cr-Commit-Position: refs/heads/master@{#329046}
  • Loading branch information
mmenke authored and Commit bot committed May 9, 2015
1 parent ca6cf17 commit 3675383
Show file tree
Hide file tree
Showing 8 changed files with 264 additions and 54 deletions.
4 changes: 4 additions & 0 deletions net/base/net_error_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ NET_ERROR(BLOCKED_ENROLLMENT_CHECK_PENDING, -24)
// retry or a redirect, but the upload stream doesn't support that operation.
NET_ERROR(UPLOAD_STREAM_REWIND_NOT_SUPPORTED, -25)

// The request failed because the URLRequestContext is shutting down, or has
// been shut down.
NET_ERROR(CONTEXT_SHUT_DOWN, -26)

// A connection was closed (corresponding to a TCP FIN).
NET_ERROR(CONNECTION_CLOSED, -100)

Expand Down
1 change: 1 addition & 0 deletions net/net.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -1178,6 +1178,7 @@
'url_request/url_request_context_builder.h',
'url_request/url_request_context_getter.cc',
'url_request/url_request_context_getter.h',
'url_request/url_request_context_getter_observer.h',
'url_request/url_request_context_storage.cc',
'url_request/url_request_context_storage.h',
'url_request/url_request_data_job.cc',
Expand Down
74 changes: 45 additions & 29 deletions net/url_request/url_fetcher_core.cc
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,11 @@ void URLFetcherCore::OnReadCompleted(URLRequest* request,
}
}

void URLFetcherCore::OnContextShuttingDown() {
DCHECK(request_);
CancelRequestAndInformDelegate(ERR_CONTEXT_SHUT_DOWN);
}

void URLFetcherCore::CancelAll() {
g_registry.Get().CancelAll();
}
Expand Down Expand Up @@ -508,11 +513,17 @@ void URLFetcherCore::StartURLRequest() {
return;
}

if (!request_context_getter_->GetURLRequestContext()) {
CancelRequestAndInformDelegate(ERR_CONTEXT_SHUT_DOWN);
return;
}

DCHECK(request_context_getter_.get());
DCHECK(!request_.get());

g_registry.Get().AddURLFetcherCore(this);
current_response_bytes_ = 0;
request_context_getter_->AddObserver(this);
request_ = request_context_getter_->GetURLRequestContext()->CreateRequest(
original_url_, DEFAULT_PRIORITY, this);
request_->set_stack_trace(stack_trace_);
Expand Down Expand Up @@ -600,10 +611,7 @@ void URLFetcherCore::StartURLRequest() {

void URLFetcherCore::DidInitializeWriter(int result) {
if (result != OK) {
CancelURLRequest(result);
delegate_task_runner_->PostTask(
FROM_HERE,
base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));
CancelRequestAndInformDelegate(result);
return;
}
StartURLRequestWhenAppropriate();
Expand All @@ -617,27 +625,33 @@ void URLFetcherCore::StartURLRequestWhenAppropriate() {

DCHECK(request_context_getter_.get());

int64 delay = 0;
if (!original_url_throttler_entry_.get()) {
URLRequestThrottlerManager* manager =
request_context_getter_->GetURLRequestContext()->throttler_manager();
if (manager) {
// Check if the request should be delayed, and if so, post a task to start it
// after the delay has expired. Otherwise, start it now.

URLRequestContext* context = request_context_getter_->GetURLRequestContext();
// If the context has been shut down, or there's no ThrottlerManager, just
// start the request. In the former case, StartURLRequest() will just inform
// the URLFetcher::Delegate the request has been canceled.
if (context && context->throttler_manager()) {
if (!original_url_throttler_entry_.get()) {
original_url_throttler_entry_ =
manager->RegisterRequestUrl(original_url_);
context->throttler_manager()->RegisterRequestUrl(original_url_);
}
}
if (original_url_throttler_entry_.get()) {
delay = original_url_throttler_entry_->ReserveSendingTimeForNextRequest(
GetBackoffReleaseTime());
}

if (delay == 0) {
StartURLRequest();
} else {
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE, base::Bind(&URLFetcherCore::StartURLRequest, this),
base::TimeDelta::FromMilliseconds(delay));
if (original_url_throttler_entry_.get()) {
int64 delay =
original_url_throttler_entry_->ReserveSendingTimeForNextRequest(
GetBackoffReleaseTime());
if (delay != 0) {
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE, base::Bind(&URLFetcherCore::StartURLRequest, this),
base::TimeDelta::FromMilliseconds(delay));
return;
}
}
}

StartURLRequest();
}

void URLFetcherCore::CancelURLRequest(int error) {
Expand Down Expand Up @@ -703,10 +717,7 @@ void URLFetcherCore::NotifyMalformedContent() {

void URLFetcherCore::DidFinishWriting(int result) {
if (result != OK) {
CancelURLRequest(result);
delegate_task_runner_->PostTask(
FROM_HERE,
base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));
CancelRequestAndInformDelegate(result);
return;
}
// If the file was successfully closed, then the URL request is complete.
Expand Down Expand Up @@ -768,7 +779,15 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() {
DCHECK(posted || !delegate_);
}

void URLFetcherCore::CancelRequestAndInformDelegate(int result) {
CancelURLRequest(result);
delegate_task_runner_->PostTask(
FROM_HERE,
base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));
}

void URLFetcherCore::ReleaseRequest() {
request_context_getter_->RemoveObserver(this);
upload_progress_checker_timer_.reset();
request_.reset();
g_registry.Get().RemoveURLFetcherCore(this);
Expand Down Expand Up @@ -827,11 +846,8 @@ int URLFetcherCore::WriteBuffer(scoped_refptr<DrainableIOBuffer> data) {
void URLFetcherCore::DidWriteBuffer(scoped_refptr<DrainableIOBuffer> data,
int result) {
if (result < 0) { // Handle errors.
CancelURLRequest(result);
response_writer_->Finish(base::Bind(&EmptyCompletionCallback));
delegate_task_runner_->PostTask(
FROM_HERE,
base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));
CancelRequestAndInformDelegate(result);
return;
}

Expand Down
15 changes: 12 additions & 3 deletions net/url_request/url_fetcher_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "net/http/http_request_headers.h"
#include "net/url_request/url_fetcher.h"
#include "net/url_request/url_request.h"
#include "net/url_request/url_request_context_getter_observer.h"
#include "net/url_request/url_request_status.h"
#include "url/gurl.h"

Expand All @@ -36,9 +37,9 @@ class URLFetcherResponseWriter;
class URLRequestContextGetter;
class URLRequestThrottlerEntryInterface;

class URLFetcherCore
: public base::RefCountedThreadSafe<URLFetcherCore>,
public URLRequest::Delegate {
class URLFetcherCore : public base::RefCountedThreadSafe<URLFetcherCore>,
public URLRequest::Delegate,
public URLRequestContextGetterObserver {
public:
URLFetcherCore(URLFetcher* fetcher,
const GURL& original_url,
Expand Down Expand Up @@ -133,6 +134,9 @@ class URLFetcherCore
void OnCertificateRequested(URLRequest* request,
SSLCertRequestInfo* cert_request_info) override;

// Overridden from URLRequestContextGetterObserver:
void OnContextShuttingDown() override;

URLFetcherDelegate* delegate() const { return delegate_; }
static void CancelAll();
static int GetNumFetcherCores();
Expand All @@ -142,6 +146,7 @@ class URLFetcherCore
private:
friend class base::RefCountedThreadSafe<URLFetcherCore>;

// TODO(mmenke): Remove this class.
class Registry {
public:
Registry();
Expand Down Expand Up @@ -177,6 +182,10 @@ class URLFetcherCore
void DidFinishWriting(int result);
void RetryOrCompleteUrlFetch();

// Cancels the URLRequest and informs the delegate that it failed with the
// specified error. Must be called on network thread.
void CancelRequestAndInformDelegate(int result);

// Deletes the request, removes it from the registry, and removes the
// destruction observer.
void ReleaseRequest();
Expand Down
Loading

0 comments on commit 3675383

Please sign in to comment.