From 5febe1427ee8a7c5a0c6feb1ee3f69e2c93f57a4 Mon Sep 17 00:00:00 2001 From: falken Date: Fri, 8 May 2015 03:28:08 -0700 Subject: [PATCH] Service Worker: Fallback to network when SW interception unexpectedly fails. Failing to dispatch the fetch event is an exceptional case: the SW may be unexpectedly missing or it failed to start up, etc. For a main resource load, responding with error just blocks access to the page, so try to fallback to network in that case. For a subresource load, it'd be strange for some subresources to come from the SW and some from the network, so return error as currently implemented. BUG=448003 Review URL: https://codereview.chromium.org/1134483003 Cr-Commit-Position: refs/heads/master@{#328942} --- .../service_worker_controllee_request_handler.cc | 14 ++--- .../service_worker_url_request_job.cc | 12 +++- .../service_worker_url_request_job.h | 2 + .../service_worker_url_request_job_unittest.cc | 69 +++++++++++++++++----- 4 files changed, 70 insertions(+), 27 deletions(-) diff --git a/content/browser/service_worker/service_worker_controllee_request_handler.cc b/content/browser/service_worker/service_worker_controllee_request_handler.cc index e2e598034acc..4cf50cdf01f3 100644 --- a/content/browser/service_worker/service_worker_controllee_request_handler.cc +++ b/content/browser/service_worker/service_worker_controllee_request_handler.cc @@ -87,16 +87,10 @@ net::URLRequestJob* ServiceWorkerControlleeRequestHandler::MaybeCreateJob( // It's for original request (A) or redirect case (B-a or B-b). DCHECK(!job_.get() || job_->ShouldForwardToServiceWorker()); - job_ = new ServiceWorkerURLRequestJob(request, - network_delegate, - provider_host_, - blob_storage_context_, - resource_context, - request_mode_, - credentials_mode_, - request_context_type_, - frame_type_, - body_); + job_ = new ServiceWorkerURLRequestJob( + request, network_delegate, provider_host_, blob_storage_context_, + resource_context, request_mode_, credentials_mode_, + is_main_resource_load_, request_context_type_, frame_type_, body_); resource_context_ = resource_context; if (is_main_resource_load_) diff --git a/content/browser/service_worker/service_worker_url_request_job.cc b/content/browser/service_worker/service_worker_url_request_job.cc index c6bdfd762faf..03a953414181 100644 --- a/content/browser/service_worker/service_worker_url_request_job.cc +++ b/content/browser/service_worker/service_worker_url_request_job.cc @@ -46,6 +46,7 @@ ServiceWorkerURLRequestJob::ServiceWorkerURLRequestJob( const ResourceContext* resource_context, FetchRequestMode request_mode, FetchCredentialsMode credentials_mode, + bool is_main_resource_load, RequestContextType request_context_type, RequestContextFrameType frame_type, scoped_refptr body) @@ -59,6 +60,7 @@ ServiceWorkerURLRequestJob::ServiceWorkerURLRequestJob( stream_pending_buffer_size_(0), request_mode_(request_mode), credentials_mode_(credentials_mode), + is_main_resource_load_(is_main_resource_load), request_context_type_(request_context_type), frame_type_(frame_type), fall_back_required_(false), @@ -486,9 +488,13 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( return; if (status != SERVICE_WORKER_OK) { - // Dispatching event has been failed, returns an error response. - // TODO(kinuko): Would be nice to log the error case. - DeliverErrorResponse(); + // TODO(falken): Add UMA and the report error to the version. + if (is_main_resource_load_) { + response_type_ = FALLBACK_TO_NETWORK; + NotifyRestartRequired(); + } else { + DeliverErrorResponse(); + } return; } diff --git a/content/browser/service_worker/service_worker_url_request_job.h b/content/browser/service_worker/service_worker_url_request_job.h index b9faa7a0a554..e29183a11f5e 100644 --- a/content/browser/service_worker/service_worker_url_request_job.h +++ b/content/browser/service_worker/service_worker_url_request_job.h @@ -58,6 +58,7 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob const ResourceContext* resource_context, FetchRequestMode request_mode, FetchCredentialsMode credentials_mode, + bool is_main_resource_load, RequestContextType request_context_type, RequestContextFrameType frame_type, scoped_refptr body); @@ -196,6 +197,7 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob FetchRequestMode request_mode_; FetchCredentialsMode credentials_mode_; + bool is_main_resource_load_; RequestContextType request_context_type_; RequestContextFrameType frame_type_; bool fall_back_required_; diff --git a/content/browser/service_worker/service_worker_url_request_job_unittest.cc b/content/browser/service_worker/service_worker_url_request_job_unittest.cc index d75461ce828e..f39244c2c015 100644 --- a/content/browser/service_worker/service_worker_url_request_job_unittest.cc +++ b/content/browser/service_worker/service_worker_url_request_job_unittest.cc @@ -40,6 +40,7 @@ #include "net/url_request/url_request.h" #include "net/url_request/url_request_context.h" #include "net/url_request/url_request_job_factory_impl.h" +#include "net/url_request/url_request_test_job.h" #include "storage/browser/blob/blob_data_builder.h" #include "storage/browser/blob/blob_storage_context.h" #include "storage/browser/blob/blob_url_request_job.h" @@ -65,31 +66,35 @@ class MockHttpProtocolHandler base::WeakPtr blob_storage_context) : provider_host_(provider_host), resource_context_(resource_context), - blob_storage_context_(blob_storage_context) {} + blob_storage_context_(blob_storage_context), + job_(nullptr) {} ~MockHttpProtocolHandler() override {} net::URLRequestJob* MaybeCreateJob( net::URLRequest* request, net::NetworkDelegate* network_delegate) const override { - ServiceWorkerURLRequestJob* job = - new ServiceWorkerURLRequestJob(request, - network_delegate, - provider_host_, - blob_storage_context_, - resource_context_, - FETCH_REQUEST_MODE_NO_CORS, - FETCH_CREDENTIALS_MODE_OMIT, - REQUEST_CONTEXT_TYPE_HYPERLINK, - REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL, - scoped_refptr()); - job->ForwardToServiceWorker(); - return job; + if (job_ && job_->ShouldFallbackToNetwork()) { + // Simulate fallback to network by constructing a valid response. + return new net::URLRequestTestJob(request, network_delegate, + net::URLRequestTestJob::test_headers(), + "PASS", true); + } + + job_ = new ServiceWorkerURLRequestJob( + request, network_delegate, provider_host_, blob_storage_context_, + resource_context_, FETCH_REQUEST_MODE_NO_CORS, + FETCH_CREDENTIALS_MODE_OMIT, true /* is_main_resource_load */, + REQUEST_CONTEXT_TYPE_HYPERLINK, REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL, + scoped_refptr()); + job_->ForwardToServiceWorker(); + return job_; } private: base::WeakPtr provider_host_; const ResourceContext* resource_context_; base::WeakPtr blob_storage_context_; + mutable ServiceWorkerURLRequestJob* job_; }; // Returns a BlobProtocolHandler that uses |blob_storage_context|. Caller owns @@ -578,6 +583,42 @@ TEST_F(ServiceWorkerURLRequestJobTest, EXPECT_FALSE(request_->status().is_success()); } +// Helper to simulate failing to dispatch a fetch event to a worker. +class FailFetchHelper : public EmbeddedWorkerTestHelper { + public: + FailFetchHelper(int mock_render_process_id) + : EmbeddedWorkerTestHelper(base::FilePath(), mock_render_process_id) {} + ~FailFetchHelper() override {} + + protected: + void OnFetchEvent(int embedded_worker_id, + int request_id, + const ServiceWorkerFetchRequest& request) override { + SimulateWorkerStopped(embedded_worker_id); + } + + private: + DISALLOW_COPY_AND_ASSIGN(FailFetchHelper); +}; + +TEST_F(ServiceWorkerURLRequestJobTest, FailFetchDispatch) { + SetUpWithHelper(new FailFetchHelper(kProcessID)); + + version_->SetStatus(ServiceWorkerVersion::ACTIVATED); + request_ = url_request_context_.CreateRequest( + GURL("http://example.com/foo.html"), net::DEFAULT_PRIORITY, + &url_request_delegate_); + request_->set_method("GET"); + request_->Start(); + + base::RunLoop().RunUntilIdle(); + EXPECT_TRUE(request_->status().is_success()); + // We should have fallen back to network. + EXPECT_EQ(200, request_->GetResponseCode()); + EXPECT_EQ("PASS", url_request_delegate_.response_data()); + EXPECT_FALSE(HasInflightRequests()); +} + // TODO(kinuko): Add more tests with different response data and also for // FallbackToNetwork case. -- 2.11.4.GIT