Skip to content

Commit

Permalink
Make URLRequestJob::SetStatus private.
Browse files Browse the repository at this point in the history 
Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
[email protected], [email protected]

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

Cr-Commit-Position: refs/heads/master@{#371072}
  • Loading branch information
mmenke authored and Commit bot committed Jan 22, 2016
1 parent 6ce02b6 commit 0fdf6eb
Show file tree
Hide file tree
Showing 17 changed files with 190 additions and 183 deletions.
12 changes: 0 additions & 12 deletions android_webview/browser/net/android_stream_reader_url_request_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,6 @@ void AndroidStreamReaderURLRequestJob::OnInputStreamOpened(
if (restart_required) {
NotifyRestartRequired();
} else {
// Clear the IO_PENDING status set in Start().
SetStatus(net::URLRequestStatus());
HeadersComplete(kHTTPNotFound, kHTTPNotFoundText);
}
return;
Expand All @@ -201,8 +199,6 @@ void AndroidStreamReaderURLRequestJob::OnInputStreamOpened(

void AndroidStreamReaderURLRequestJob::OnReaderSeekCompleted(int result) {
DCHECK(thread_checker_.CalledOnValidThread());
// Clear the IO_PENDING status set in Start().
SetStatus(net::URLRequestStatus());
if (result >= 0) {
set_expected_content_size(result);
HeadersComplete(kHTTPOk, kHTTPOkText);
Expand Down Expand Up @@ -252,10 +248,6 @@ bool AndroidStreamReaderURLRequestJob::GetMimeType(
if (!input_stream_reader_wrapper_.get())
return false;

// Since it's possible for this call to alter the InputStream a
// Seek or ReadRawData operation running in the background is not permitted.
DCHECK(!request_->status().is_io_pending());

return delegate_->GetMimeType(
env, request(), input_stream_reader_wrapper_->input_stream(), mime_type);
}
Expand Down Expand Up @@ -295,10 +287,6 @@ void AndroidStreamReaderURLRequestJob::DoStart() {
range_parse_result_));
return;
}
// Start reading asynchronously so that all error reporting and data
// callbacks happen as they would for network requests.
SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING,
net::ERR_IO_PENDING));

// This could be done in the InputStreamReader but would force more
// complex synchronization in the delegate.
Expand Down
2 changes: 0 additions & 2 deletions content/browser/service_worker/service_worker_read_from_cache_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ void ServiceWorkerReadFromCacheJob::StartAsync() {
http_info_io_buffer_.get(),
base::Bind(&ServiceWorkerReadFromCacheJob::OnReadInfoComplete,
weak_factory_.GetWeakPtr()));
SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
}

const net::HttpResponseInfo* ServiceWorkerReadFromCacheJob::http_info() const {
Expand All @@ -161,7 +160,6 @@ void ServiceWorkerReadFromCacheJob::OnReadInfoComplete(int result) {
return;
}
DCHECK_GE(result, 0);
SetStatus(net::URLRequestStatus()); // Clear the IO_PENDING status
http_info_.reset(http_info_io_buffer_->http_info.release());
if (is_range_request())
SetupRangeResponse(http_info_io_buffer_->response_data_size);
Expand Down
53 changes: 32 additions & 21 deletions content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <stdint.h>

#include "base/macros.h"
#include "base/memory/scoped_ptr.h"
#include "base/run_loop.h"
#include "content/browser/fileapi/mock_url_request_delegate.h"
#include "content/browser/service_worker/embedded_worker_test_helper.h"
Expand All @@ -20,6 +21,7 @@
#include "net/url_request/url_request_context.h"
#include "net/url_request/url_request_job_factory_impl.h"
#include "net/url_request/url_request_status.h"
#include "net/url_request/url_request_test_util.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace content {
Expand Down Expand Up @@ -59,16 +61,23 @@ class ServiceWorkerReadFromCacheJobTest : public testing::Test {
kResourceSize),
imported_script_(kImportedScriptResourceId,
GURL("http://example.com/imported.js"),
kResourceSize) {}
kResourceSize),
test_job_interceptor_(nullptr) {}
~ServiceWorkerReadFromCacheJobTest() override {}

void SetUp() override {
helper_.reset(new EmbeddedWorkerTestHelper(base::FilePath()));
InitializeStorage();

url_request_context_.reset(new net::URLRequestContext);
url_request_job_factory_.reset(new net::URLRequestJobFactoryImpl);
url_request_context_->set_job_factory(url_request_job_factory_.get());
url_request_context_.reset(new net::TestURLRequestContext(true));

// The |test_job_factory_| takes ownership of the interceptor.
test_job_interceptor_ = new net::TestJobInterceptor();
EXPECT_TRUE(test_job_factory_.SetProtocolHandler(
url::kHttpScheme, make_scoped_ptr(test_job_interceptor_)));
url_request_context_->set_job_factory(&test_job_factory_);

url_request_context_->Init();
}

void InitializeStorage() {
Expand Down Expand Up @@ -145,9 +154,8 @@ class ServiceWorkerReadFromCacheJobTest : public testing::Test {
return status;
}

void StartAndWaitForJob(
const scoped_ptr<ServiceWorkerReadFromCacheJob>& job) {
job->Start();
void StartAndWaitForRequest(net::URLRequest* request) {
request->Start();
// MockURLRequestDelegate quits the loop when the request is completed.
base::RunLoop().RunUntilIdle();
}
Expand All @@ -168,21 +176,24 @@ class ServiceWorkerReadFromCacheJobTest : public testing::Test {
ServiceWorkerDatabase::ResourceRecord main_script_;
ServiceWorkerDatabase::ResourceRecord imported_script_;

scoped_ptr<net::URLRequestContext> url_request_context_;
scoped_ptr<net::URLRequestJobFactoryImpl> url_request_job_factory_;
// |test_job_interceptor_| is owned by |test_job_factory_|.
net::TestJobInterceptor* test_job_interceptor_;
net::URLRequestJobFactoryImpl test_job_factory_;

scoped_ptr<net::TestURLRequestContext> url_request_context_;
MockURLRequestDelegate delegate_;
};

TEST_F(ServiceWorkerReadFromCacheJobTest, ReadMainScript) {
// Read the main script from the diskcache.
scoped_ptr<net::URLRequest> request = url_request_context_->CreateRequest(
main_script_.url, net::DEFAULT_PRIORITY, &delegate_);
scoped_ptr<ServiceWorkerReadFromCacheJob> job(
new ServiceWorkerReadFromCacheJob(
test_job_interceptor_->set_main_intercept_job(
make_scoped_ptr(new ServiceWorkerReadFromCacheJob(
request.get(), nullptr /* NetworkDelegate */,
RESOURCE_TYPE_SERVICE_WORKER, context()->AsWeakPtr(), version_,
main_script_.resource_id));
StartAndWaitForJob(job);
main_script_.resource_id)));
StartAndWaitForRequest(request.get());

EXPECT_EQ(net::URLRequestStatus::SUCCESS, request->status().status());
EXPECT_EQ(0, request->status().error());
Expand All @@ -194,11 +205,11 @@ TEST_F(ServiceWorkerReadFromCacheJobTest, ReadImportedScript) {
// Read the imported script from the diskcache.
scoped_ptr<net::URLRequest> request = url_request_context_->CreateRequest(
imported_script_.url, net::DEFAULT_PRIORITY, &delegate_);
scoped_ptr<ServiceWorkerReadFromCacheJob> job(
new ServiceWorkerReadFromCacheJob(
test_job_interceptor_->set_main_intercept_job(
make_scoped_ptr(new ServiceWorkerReadFromCacheJob(
request.get(), nullptr /* NetworkDelegate */, RESOURCE_TYPE_SCRIPT,
context()->AsWeakPtr(), version_, imported_script_.resource_id));
StartAndWaitForJob(job);
context()->AsWeakPtr(), version_, imported_script_.resource_id)));
StartAndWaitForRequest(request.get());

EXPECT_EQ(net::URLRequestStatus::SUCCESS, request->status().status());
EXPECT_EQ(0, request->status().error());
Expand All @@ -214,12 +225,12 @@ TEST_F(ServiceWorkerReadFromCacheJobTest, ResourceNotFound) {
GURL("http://example.com/nonexistent"), net::DEFAULT_PRIORITY,
&delegate_);
const int64_t kNonexistentResourceId = 100;
scoped_ptr<ServiceWorkerReadFromCacheJob> job(
new ServiceWorkerReadFromCacheJob(
test_job_interceptor_->set_main_intercept_job(
make_scoped_ptr(new ServiceWorkerReadFromCacheJob(
request.get(), nullptr /* NetworkDelegate */,
RESOURCE_TYPE_SERVICE_WORKER, context()->AsWeakPtr(), version_,
kNonexistentResourceId));
StartAndWaitForJob(job);
kNonexistentResourceId)));
StartAndWaitForRequest(request.get());

EXPECT_EQ(net::URLRequestStatus::FAILED, request->status().status());
EXPECT_EQ(net::ERR_CACHE_MISS, request->status().error());
Expand Down
3 changes: 0 additions & 3 deletions content/browser/service_worker/service_worker_write_to_cache_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,12 +365,10 @@ void ServiceWorkerWriteToCacheJob::OnWriteHeadersComplete(net::Error error) {
NotifyStartError(net::URLRequestStatus::FromError(error));
return;
}
SetStatus(net::URLRequestStatus());
NotifyHeadersComplete();
}

void ServiceWorkerWriteToCacheJob::OnWriteDataComplete(net::Error error) {
SetStatus(net::URLRequestStatus::FromError(error));
DCHECK_NE(net::ERR_IO_PENDING, error);
if (io_buffer_bytes_ == 0)
error = NotifyFinishedCaching(net::URLRequestStatus::FromError(error), "");
Expand Down Expand Up @@ -452,7 +450,6 @@ void ServiceWorkerWriteToCacheJob::NotifyStartErrorHelper(
net::URLRequestStatus reported_status = status;
std::string reported_status_message = status_message;

SetStatus(reported_status);
NotifyStartError(reported_status);
}

Expand Down
33 changes: 10 additions & 23 deletions content/browser/streams/stream_url_request_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ StreamURLRequestJob::StreamURLRequestJob(
net::URLRequest* request,
net::NetworkDelegate* network_delegate,
scoped_refptr<Stream> stream)
: net::URLRequestJob(request, network_delegate),
: net::URLRangeRequestJob(request, network_delegate),
stream_(stream),
headers_set_(false),
pending_buffer_size_(0),
Expand Down Expand Up @@ -147,31 +147,18 @@ int StreamURLRequestJob::GetResponseCode() const {
return response_info_->headers->response_code();
}

void StreamURLRequestJob::SetExtraRequestHeaders(
const net::HttpRequestHeaders& headers) {
std::string range_header;
if (headers.GetHeader(net::HttpRequestHeaders::kRange, &range_header)) {
std::vector<net::HttpByteRange> ranges;
if (net::HttpUtil::ParseRangeHeader(range_header, &ranges)) {
if (ranges.size() == 1) {
// Streams don't support seeking, so a non-zero starting position
// doesn't make sense.
if (ranges[0].first_byte_position() == 0) {
max_range_ = ranges[0].last_byte_position() + 1;
} else {
NotifyFailure(net::ERR_METHOD_NOT_SUPPORTED);
return;
}
} else {
NotifyFailure(net::ERR_METHOD_NOT_SUPPORTED);
return;
}
void StreamURLRequestJob::DidStart() {
if (range_parse_result() == net::OK && ranges().size() > 0) {
// Only one range is supported, and it must start at the first byte.
if (ranges().size() > 1 || ranges()[0].first_byte_position() != 0) {
NotifyFailure(net::ERR_METHOD_NOT_SUPPORTED);
return;
}

max_range_ = ranges()[0].last_byte_position() + 1;
}
}

void StreamURLRequestJob::DidStart() {
// We only support GET request.
// This class only supports GET requests.
if (request()->method() != "GET") {
NotifyFailure(net::ERR_METHOD_NOT_SUPPORTED);
return;
Expand Down
5 changes: 2 additions & 3 deletions content/browser/streams/stream_url_request_job.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@
#include "content/browser/streams/stream_read_observer.h"
#include "content/common/content_export.h"
#include "net/http/http_status_code.h"
#include "net/url_request/url_request_job.h"
#include "net/url_request/url_range_request_job.h"

namespace content {

class Stream;

// A request job that handles reading stream URLs.
class CONTENT_EXPORT StreamURLRequestJob
: public net::URLRequestJob,
: public net::URLRangeRequestJob,
public StreamReadObserver {
public:
StreamURLRequestJob(net::URLRequest* request,
Expand All @@ -34,7 +34,6 @@ class CONTENT_EXPORT StreamURLRequestJob
bool GetMimeType(std::string* mime_type) const override;
void GetResponseInfo(net::HttpResponseInfo* info) override;
int GetResponseCode() const override;
void SetExtraRequestHeaders(const net::HttpRequestHeaders& headers) override;

protected:
~StreamURLRequestJob() override;
Expand Down
5 changes: 2 additions & 3 deletions content/public/test/test_download_request_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,6 @@ int TestDownloadRequestHandler::PartialResponseJob::ReadRawData(

if (offset_of_next_read_ == injected_error.offset) {
int error = injected_error.error;
SetStatus(net::URLRequestStatus(net::URLRequestStatus::FAILED, error));
DVLOG(1) << "Returning error " << net::ErrorToString(error);
ReportCompletedRequest(injected_error.offset - requested_range_begin_);
parameters_->injected_errors.pop();
Expand Down Expand Up @@ -353,7 +352,6 @@ void TestDownloadRequestHandler::PartialResponseJob::OnStartResponseCallback(

void TestDownloadRequestHandler::PartialResponseJob::HandleOnStartDefault() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
SetStatus(net::URLRequestStatus());

const net::HttpRequestHeaders& extra_headers =
request()->extra_request_headers();
Expand All @@ -367,8 +365,9 @@ void TestDownloadRequestHandler::PartialResponseJob::HandleOnStartDefault() {
// ETag, then try to handle the range request.
if (parameters_->support_byte_ranges &&
extra_headers.GetHeader(net::HttpRequestHeaders::kIfRange, &value) &&
value == parameters_->etag && HandleRangeAssumingValidatorMatch())
value == parameters_->etag && HandleRangeAssumingValidatorMatch()) {
return;
}

if (parameters_->support_byte_ranges &&
extra_headers.GetHeader("If-Match", &value)) {
Expand Down
1 change: 0 additions & 1 deletion net/test/url_request/url_request_failed_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ void URLRequestFailedJob::StartAsync() {
NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED, net_error_));
return;
}
SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
return;
}
response_info_.headers = new net::HttpResponseHeaders("HTTP/1.1 200 OK");
Expand Down
22 changes: 16 additions & 6 deletions net/url_request/url_request.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,10 +663,13 @@ void URLRequest::StartJob(URLRequestJob* job) {
}
}

// Don't allow errors to be sent from within Start().
// TODO(brettw) this may cause NotifyDone to be sent synchronously,
// we probably don't want this: they should be sent asynchronously so
// the caller does not get reentered.
// Start() always completes asynchronously.
//
// Status is generally set by URLRequestJob itself, but Start() calls
// directly into the URLRequestJob subclass, so URLRequestJob can't set it
// here.
// TODO(mmenke): Make the URLRequest manage its own status.
status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
job_->Start();
}

Expand Down Expand Up @@ -860,36 +863,41 @@ void URLRequest::NotifyResponseStarted() {
}

void URLRequest::FollowDeferredRedirect() {
CHECK(job_.get());
CHECK(status_.is_success());
DCHECK(job_.get());
DCHECK(status_.is_success());

status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
job_->FollowDeferredRedirect();
}

void URLRequest::SetAuth(const AuthCredentials& credentials) {
DCHECK(job_.get());
DCHECK(job_->NeedsAuth());

status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
job_->SetAuth(credentials);
}

void URLRequest::CancelAuth() {
DCHECK(job_.get());
DCHECK(job_->NeedsAuth());

status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
job_->CancelAuth();
}

void URLRequest::ContinueWithCertificate(X509Certificate* client_cert,
SSLPrivateKey* client_private_key) {
DCHECK(job_.get());

status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
job_->ContinueWithCertificate(client_cert, client_private_key);
}

void URLRequest::ContinueDespiteLastError() {
DCHECK(job_.get());

status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
job_->ContinueDespiteLastError();
}

Expand Down Expand Up @@ -1116,11 +1124,13 @@ void URLRequest::NotifyAuthRequiredComplete(

void URLRequest::NotifyCertificateRequested(
SSLCertRequestInfo* cert_request_info) {
status_ = URLRequestStatus();
delegate_->OnCertificateRequested(this, cert_request_info);
}

void URLRequest::NotifySSLCertificateError(const SSLInfo& ssl_info,
bool fatal) {
status_ = URLRequestStatus();
delegate_->OnSSLCertificateError(this, ssl_info, fatal);
}

Expand Down
Loading

0 comments on commit 0fdf6eb

Please sign in to comment.