diff options
author | Matt Falkenhagen <falken@chromium.org> | 2015-05-15 10:03:18 +0900 |
---|---|---|
committer | Matt Falkenhagen <falken@chromium.org> | 2015-05-15 01:04:21 +0000 |
commit | bee610e3fc2c416b295c4619c610cfb6f5b13d35 (patch) | |
tree | d992a7a7070707facdbcf0c78bd61997481d8d7d | |
parent | 1619d40a0ad1efb8887d6d8fd7b377d76c8a05db (diff) | |
download | chromium_src-bee610e3fc2c416b295c4619c610cfb6f5b13d35.zip chromium_src-bee610e3fc2c416b295c4619c610cfb6f5b13d35.tar.gz chromium_src-bee610e3fc2c416b295c4619c610cfb6f5b13d35.tar.bz2 |
(Merge to M43) Evict Service Worker when reading it from disk cache fails.
If reading the SW resources from the disk cache fails, we should evict
the worker or else we will keep getting the same failure.
This patch unsets the worker from the live registration, and then
deletes the registration if the SW was the stored version.
This relands the original commit (64e3ffb1c), and fixes a problem where
the test terminated before the eviction, which starts on the IO thread,
then hops to the database task runner, finishes. Now the test ends by
looking up the registration in storage, which hops to the database task
runner, so it can only finish once eviction finishes.
Original patch: https://codereview.chromium.org/1098083003/
BUG=488061
R=kinuko@chromium.org
TBR=kinuko
Review URL: https://codereview.chromium.org/1108253002
Cr-Commit-Position: refs/heads/master@{#327260}
(cherry picked from commit 0e476caf67493d79b6dc40891e7ef8868871983e)
Review URL: https://codereview.chromium.org/1141053002
Cr-Commit-Position: refs/branch-heads/2357@{#395}
Cr-Branched-From: 59d4494849b405682265ed5d3f5164573b9a939b-refs/heads/master@{#323860}
4 files changed, 204 insertions, 7 deletions
diff --git a/content/browser/service_worker/service_worker_browsertest.cc b/content/browser/service_worker/service_worker_browsertest.cc index 3a23937..1f41351 100644 --- a/content/browser/service_worker/service_worker_browsertest.cc +++ b/content/browser/service_worker/service_worker_browsertest.cc @@ -131,6 +131,25 @@ ServiceWorkerVersion::FetchCallback CreateResponseReceiver( result); } +void ReceiveFindRegistrationStatus( + BrowserThread::ID run_quit_thread, + const base::Closure& quit, + ServiceWorkerStatusCode* out_status, + ServiceWorkerStatusCode status, + const scoped_refptr<ServiceWorkerRegistration>& registration) { + *out_status = status; + if (!quit.is_null()) + BrowserThread::PostTask(run_quit_thread, FROM_HERE, quit); +} + +ServiceWorkerStorage::FindRegistrationCallback CreateFindRegistrationReceiver( + BrowserThread::ID run_quit_thread, + const base::Closure& quit, + ServiceWorkerStatusCode* status) { + return base::Bind(&ReceiveFindRegistrationStatus, run_quit_thread, quit, + status); +} + void ReadResponseBody(std::string* body, storage::BlobDataHandle* blob_data_handle) { ASSERT_TRUE(blob_data_handle); @@ -228,7 +247,7 @@ class LongLivedResourceInterceptor : public net::URLRequestInterceptor { void CreateLongLivedResourceInterceptors( const GURL& worker_url, const GURL& import_url) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); scoped_ptr<net::URLRequestInterceptor> interceptor; interceptor.reset(new LongLivedResourceInterceptor( @@ -515,6 +534,7 @@ class ServiceWorkerVersionBrowserTest : public ServiceWorkerBrowserTest { } void SetUpRegistrationOnIOThread(const std::string& worker_url) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); const GURL pattern = embedded_test_server()->GetURL("/service_worker/"); registration_ = new ServiceWorkerRegistration( pattern, @@ -539,6 +559,32 @@ class ServiceWorkerVersionBrowserTest : public ServiceWorkerBrowserTest { version_->OnPingTimeout(); } + void AddControlleeOnIOThread() { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); + scoped_ptr<ServiceWorkerProviderHost> host(new ServiceWorkerProviderHost( + 33 /* dummy render process id */, + MSG_ROUTING_NONE /* render_frame_id */, 1 /* dummy provider_id */, + SERVICE_WORKER_PROVIDER_FOR_WINDOW, wrapper()->context()->AsWeakPtr(), + NULL)); + host->SetDocumentUrl( + embedded_test_server()->GetURL("/service_worker/host")); + host->AssociateRegistration(registration_.get(), + false /* notify_controllerchange */); + wrapper()->context()->AddProviderHost(host.Pass()); + } + + void AddWaitingWorkerOnIOThread(const std::string& worker_url) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); + scoped_refptr<ServiceWorkerVersion> waiting_version( + new ServiceWorkerVersion( + registration_.get(), embedded_test_server()->GetURL(worker_url), + wrapper()->context()->storage()->NewVersionId(), + wrapper()->context()->AsWeakPtr())); + waiting_version->SetStatus(ServiceWorkerVersion::INSTALLED); + registration_->SetWaitingVersion(waiting_version.get()); + registration_->ActivateWaitingVersionWhenReady(); + } + void StartWorker(ServiceWorkerStatusCode expected_status) { ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI)); ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED; @@ -563,6 +609,58 @@ class ServiceWorkerVersionBrowserTest : public ServiceWorkerBrowserTest { ASSERT_EQ(expected_status, status); } + void StoreRegistration(int64 version_id, + ServiceWorkerStatusCode expected_status) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI)); + ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED; + base::RunLoop store_run_loop; + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&self::StoreOnIOThread, this, store_run_loop.QuitClosure(), + &status, version_id)); + store_run_loop.Run(); + ASSERT_EQ(expected_status, status); + + RunOnIOThread(base::Bind(&self::NotifyDoneInstallingRegistrationOnIOThread, + this, status)); + } + + void FindRegistrationForId(int64 id, + const GURL& origin, + ServiceWorkerStatusCode expected_status) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI)); + ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED; + base::RunLoop run_loop; + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&self::FindRegistrationForIdOnIOThread, this, + run_loop.QuitClosure(), &status, id, origin)); + run_loop.Run(); + ASSERT_EQ(expected_status, status); + } + + void FindRegistrationForIdOnIOThread(const base::Closure& done, + ServiceWorkerStatusCode* result, + int64 id, + const GURL& origin) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); + wrapper()->context()->storage()->FindRegistrationForId( + id, origin, + CreateFindRegistrationReceiver(BrowserThread::UI, done, result)); + } + + void NotifyDoneInstallingRegistrationOnIOThread( + ServiceWorkerStatusCode status) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); + wrapper()->context()->storage()->NotifyDoneInstallingRegistration( + registration_.get(), version_.get(), status); + } + + void RemoveLiveRegistrationOnIOThread(int64 id) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); + wrapper()->context()->RemoveLiveRegistration(id); + } + void StartOnIOThread(const base::Closure& done, ServiceWorkerStatusCode* result) { ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); @@ -577,10 +675,22 @@ class ServiceWorkerVersionBrowserTest : public ServiceWorkerBrowserTest { CreateReceiver(BrowserThread::UI, done, result)); } + void StoreOnIOThread(const base::Closure& done, + ServiceWorkerStatusCode* result, + int64 version_id) { + ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); + ServiceWorkerVersion* version = + wrapper()->context()->GetLiveVersion(version_id); + wrapper()->context()->storage()->StoreRegistration( + registration_.get(), version, + CreateReceiver(BrowserThread::UI, done, result)); + } + void ActivateOnIOThread(const base::Closure& done, ServiceWorkerStatusCode* result) { ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); version_->SetStatus(ServiceWorkerVersion::ACTIVATING); + registration_->SetActiveVersion(version_.get()); version_->DispatchActivateEvent( CreateReceiver(BrowserThread::UI, done, result)); } @@ -697,6 +807,76 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest, StartNotFound) { StartWorker(SERVICE_WORKER_ERROR_NETWORK); } +IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest, ReadResourceFailure) { + // Create and store a registration. + RunOnIOThread(base::Bind(&self::SetUpRegistrationOnIOThread, this, + "/service_worker/worker.js")); + version_->SetStatus(ServiceWorkerVersion::ACTIVATED); + StoreRegistration(version_->version_id(), SERVICE_WORKER_OK); + + // Add a non-existent resource to the version. + std::vector<ServiceWorkerDatabase::ResourceRecord> records; + records.push_back( + ServiceWorkerDatabase::ResourceRecord(30, version_->script_url(), 100)); + version_->script_cache_map()->SetResources(records); + + // Start the worker. We'll fail to read the resource. + StartWorker(SERVICE_WORKER_ERROR_START_WORKER_FAILED); + EXPECT_EQ(ServiceWorkerVersion::REDUNDANT, version_->status()); + + // The registration should be deleted from storage since the broken worker was + // the stored one. + RunOnIOThread(base::Bind(&self::RemoveLiveRegistrationOnIOThread, this, + registration_->id())); + FindRegistrationForId(registration_->id(), + registration_->pattern().GetOrigin(), + SERVICE_WORKER_ERROR_NOT_FOUND); +} + +IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest, + ReadResourceFailure_WaitingWorker) { + // Create a registration and active version. + RunOnIOThread(base::Bind(&self::SetUpRegistrationOnIOThread, this, + "/service_worker/worker.js")); + base::RunLoop activate_run_loop; + ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&self::ActivateOnIOThread, this, + activate_run_loop.QuitClosure(), &status)); + activate_run_loop.Run(); + EXPECT_EQ(SERVICE_WORKER_OK, status); + ASSERT_TRUE(registration_->active_version()); + + // Give the version a controllee. + RunOnIOThread(base::Bind(&self::AddControlleeOnIOThread, this)); + + // Add a non-existent resource to the version. + std::vector<ServiceWorkerDatabase::ResourceRecord> records; + records.push_back( + ServiceWorkerDatabase::ResourceRecord(30, version_->script_url(), 100)); + version_->script_cache_map()->SetResources(records); + + // Make a waiting version and store it. + RunOnIOThread(base::Bind(&self::AddWaitingWorkerOnIOThread, this, + "/service_worker/worker.js")); + StoreRegistration(registration_->waiting_version()->version_id(), + SERVICE_WORKER_OK); + + // Start the broken worker. We'll fail to read from disk and the worker should + // be doomed. + StopWorker(SERVICE_WORKER_OK); // in case it's already running + StartWorker(SERVICE_WORKER_ERROR_START_WORKER_FAILED); + EXPECT_EQ(ServiceWorkerVersion::REDUNDANT, version_->status()); + + // The registration should still be in storage since the waiting worker was + // the stored one. + RunOnIOThread(base::Bind(&self::RemoveLiveRegistrationOnIOThread, this, + registration_->id())); + FindRegistrationForId(registration_->id(), + registration_->pattern().GetOrigin(), + SERVICE_WORKER_OK); +} + IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest, Install) { InstallTestHelper("/service_worker/worker.js", SERVICE_WORKER_OK); } @@ -1196,11 +1376,11 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserV8CacheTest, Restart) { // Activate the worker. ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED; - base::RunLoop acrivate_run_loop; + base::RunLoop activate_run_loop; BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(&self::ActivateOnIOThread, this, - acrivate_run_loop.QuitClosure(), &status)); - acrivate_run_loop.Run(); + activate_run_loop.QuitClosure(), &status)); + activate_run_loop.Run(); ASSERT_EQ(SERVICE_WORKER_OK, status); // Stop the worker. StopWorker(SERVICE_WORKER_OK); diff --git a/content/browser/service_worker/service_worker_read_from_cache_job.cc b/content/browser/service_worker/service_worker_read_from_cache_job.cc index 135c589..3c8400b 100644 --- a/content/browser/service_worker/service_worker_read_from_cache_job.cc +++ b/content/browser/service_worker/service_worker_read_from_cache_job.cc @@ -12,6 +12,7 @@ #include "content/browser/service_worker/service_worker_context_core.h" #include "content/browser/service_worker/service_worker_disk_cache.h" #include "content/browser/service_worker/service_worker_metrics.h" +#include "content/public/browser/browser_thread.h" #include "net/base/io_buffer.h" #include "net/base/net_errors.h" #include "net/http/http_request_headers.h" @@ -157,7 +158,7 @@ void ServiceWorkerReadFromCacheJob::OnReadInfoComplete(int result) { DCHECK_LT(result, 0); ServiceWorkerMetrics::CountReadResponseResult( ServiceWorkerMetrics::READ_HEADERS_ERROR); - NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED, result)); + Done(net::URLRequestStatus(net::URLRequestStatus::FAILED, result)); return; } DCHECK_GE(result, 0); @@ -198,14 +199,27 @@ void ServiceWorkerReadFromCacheJob::SetupRangeResponse(int resource_size) { range_requested_, resource_size, true /* replace status line */); } +void ServiceWorkerReadFromCacheJob::Done(const net::URLRequestStatus& status) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + if (!status.is_success()) { + // TODO(falken): Retry before evicting. + if (context_) { + ServiceWorkerRegistration* registration = + context_->GetLiveRegistration(version_->registration_id()); + registration->DeleteVersion(version_); + } + } + NotifyDone(status); +} + void ServiceWorkerReadFromCacheJob::OnReadComplete(int result) { ServiceWorkerMetrics::ReadResponseResult check_result; if (result == 0) { check_result = ServiceWorkerMetrics::READ_OK; - NotifyDone(net::URLRequestStatus()); + Done(net::URLRequestStatus()); } else if (result < 0) { check_result = ServiceWorkerMetrics::READ_DATA_ERROR; - NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED, result)); + Done(net::URLRequestStatus(net::URLRequestStatus::FAILED, result)); } else { check_result = ServiceWorkerMetrics::READ_OK; SetStatus(net::URLRequestStatus()); // Clear the IO_PENDING status diff --git a/content/browser/service_worker/service_worker_read_from_cache_job.h b/content/browser/service_worker/service_worker_read_from_cache_job.h index 48c2aa5..82d9d5f 100644 --- a/content/browser/service_worker/service_worker_read_from_cache_job.h +++ b/content/browser/service_worker/service_worker_read_from_cache_job.h @@ -55,6 +55,7 @@ class CONTENT_EXPORT ServiceWorkerReadFromCacheJob const net::HttpResponseInfo* http_info() const; bool is_range_request() const { return range_requested_.IsValid(); } void SetupRangeResponse(int response_data_size); + void Done(const net::URLRequestStatus& status); base::WeakPtr<ServiceWorkerContextCore> context_; scoped_refptr<ServiceWorkerVersion> version_; diff --git a/content/browser/service_worker/service_worker_registration.cc b/content/browser/service_worker/service_worker_registration.cc index a71daf1..2edf34a 100644 --- a/content/browser/service_worker/service_worker_registration.cc +++ b/content/browser/service_worker/service_worker_registration.cc @@ -242,6 +242,8 @@ void ServiceWorkerRegistration::ClearUserData( } void ServiceWorkerRegistration::OnNoControllees(ServiceWorkerVersion* version) { + if (!context_) + return; DCHECK_EQ(active_version(), version); if (is_uninstalling_) Clear(); |