From 7c76e32b639e3e8c89d1d3c9aa5de96cb1f0d1ff Mon Sep 17 00:00:00 2001 From: nhiroki Date: Thu, 15 Jan 2015 18:32:33 -0800 Subject: [PATCH] ServiceWorker: Consolidate version manipulation functions in SWProviderContext This is a preparation patch for exposing SWRegistration object into SWGlobalScope. According to the design doc[1], we're going to make SWProviderContext thread-safe using the lock mechanism so that the worker thread can also use the context. But, in the current implementation, functions to manipulate versions (eg. OnSetInstallingServiceWorker()) are fine-grained and they can be interleaved from multiple threads if we naively use the lock on a per function basis. To avoid the situation, this CL consolidates those functions as SetVersionAttributes() and makes it easier to protect by lock. [1] https://docs.google.com/document/d/1qDGbMlwKOXxCRBlw9IirK8Qmna3JqvnO3Aogkfu9UJQ/edit?usp=sharing BUG=437677 TEST=run-webkit-tests http/tests/serviceworker/ Review URL: https://codereview.chromium.org/849203002 Cr-Commit-Position: refs/heads/master@{#311822} --- .../service_worker/service_worker_dispatcher.cc | 111 +++++++-------------- .../service_worker/service_worker_dispatcher.h | 12 --- .../service_worker_provider_context.cc | 82 +++++++-------- .../service_worker_provider_context.h | 8 +- 4 files changed, 73 insertions(+), 140 deletions(-) diff --git a/content/child/service_worker/service_worker_dispatcher.cc b/content/child/service_worker/service_worker_dispatcher.cc index ea6f2b9127a1..0a30cfbb25c6 100644 --- a/content/child/service_worker/service_worker_dispatcher.cc +++ b/content/child/service_worker/service_worker_dispatcher.cc @@ -495,27 +495,47 @@ void ServiceWorkerDispatcher::OnSetVersionAttributes( int provider_id, int registration_handle_id, int changed_mask, - const ServiceWorkerVersionAttributes& attributes) { + const ServiceWorkerVersionAttributes& attrs) { TRACE_EVENT1("ServiceWorker", "ServiceWorkerDispatcher::OnSetVersionAttributes", "Thread ID", thread_id); + ChangedVersionAttributesMask mask(changed_mask); - if (mask.installing_changed()) { - SetInstallingServiceWorker(provider_id, - registration_handle_id, - attributes.installing); + ProviderContextMap::iterator provider = provider_contexts_.find(provider_id); + if (provider != provider_contexts_.end() && + provider->second->registration_handle_id() == registration_handle_id) { + if (mask.installing_changed()) { + worker_to_provider_.erase(provider->second->installing_handle_id()); + if (attrs.installing.handle_id != kInvalidServiceWorkerHandleId) + worker_to_provider_[attrs.installing.handle_id] = provider->second; + } + if (mask.waiting_changed()) { + worker_to_provider_.erase(provider->second->waiting_handle_id()); + if (attrs.waiting.handle_id != kInvalidServiceWorkerHandleId) + worker_to_provider_[attrs.waiting.handle_id] = provider->second; + } + if (mask.active_changed()) { + worker_to_provider_.erase(provider->second->active_handle_id()); + if (attrs.active.handle_id != kInvalidServiceWorkerHandleId) + worker_to_provider_[attrs.active.handle_id] = provider->second; + } + provider->second->SetVersionAttributes(mask, attrs); } - if (mask.waiting_changed()) { - SetWaitingServiceWorker(provider_id, - registration_handle_id, - attributes.waiting); + + RegistrationObjectMap::iterator found = + registrations_.find(registration_handle_id); + if (found != registrations_.end()) { + // Populate the version fields (eg. .installing) with new worker objects. + if (mask.installing_changed()) + found->second->SetInstalling(GetServiceWorker(attrs.installing, false)); + if (mask.waiting_changed()) + found->second->SetWaiting(GetServiceWorker(attrs.waiting, false)); + if (mask.active_changed()) + found->second->SetActive(GetServiceWorker(attrs.active, false)); } - if (mask.active_changed()) { - SetActiveServiceWorker(provider_id, - registration_handle_id, - attributes.active); + + if (mask.active_changed()) SetReadyRegistration(provider_id, registration_handle_id); - } } void ServiceWorkerDispatcher::OnUpdateFound( @@ -528,69 +548,6 @@ void ServiceWorkerDispatcher::OnUpdateFound( found->second->OnUpdateFound(); } -void ServiceWorkerDispatcher::SetInstallingServiceWorker( - int provider_id, - int registration_handle_id, - const ServiceWorkerObjectInfo& info) { - ProviderContextMap::iterator provider = provider_contexts_.find(provider_id); - if (provider != provider_contexts_.end() && - provider->second->registration_handle_id() == registration_handle_id) { - worker_to_provider_.erase(provider->second->installing_handle_id()); - if (info.handle_id != kInvalidServiceWorkerHandleId) - worker_to_provider_[info.handle_id] = provider->second; - provider->second->OnSetInstallingServiceWorker(info); - } - - RegistrationObjectMap::iterator found = - registrations_.find(registration_handle_id); - if (found != registrations_.end()) { - // Populate the .installing field with the new worker object. - found->second->SetInstalling(GetServiceWorker(info, false)); - } -} - -void ServiceWorkerDispatcher::SetWaitingServiceWorker( - int provider_id, - int registration_handle_id, - const ServiceWorkerObjectInfo& info) { - ProviderContextMap::iterator provider = provider_contexts_.find(provider_id); - if (provider != provider_contexts_.end() && - provider->second->registration_handle_id() == registration_handle_id) { - worker_to_provider_.erase(provider->second->waiting_handle_id()); - if (info.handle_id != kInvalidServiceWorkerHandleId) - worker_to_provider_[info.handle_id] = provider->second; - provider->second->OnSetWaitingServiceWorker(info); - } - - RegistrationObjectMap::iterator found = - registrations_.find(registration_handle_id); - if (found != registrations_.end()) { - // Populate the .waiting field with the new worker object. - found->second->SetWaiting(GetServiceWorker(info, false)); - } -} - -void ServiceWorkerDispatcher::SetActiveServiceWorker( - int provider_id, - int registration_handle_id, - const ServiceWorkerObjectInfo& info) { - ProviderContextMap::iterator provider = provider_contexts_.find(provider_id); - if (provider != provider_contexts_.end() && - provider->second->registration_handle_id() == registration_handle_id) { - worker_to_provider_.erase(provider->second->active_handle_id()); - if (info.handle_id != kInvalidServiceWorkerHandleId) - worker_to_provider_[info.handle_id] = provider->second; - provider->second->OnSetActiveServiceWorker(info); - } - - RegistrationObjectMap::iterator found = - registrations_.find(registration_handle_id); - if (found != registrations_.end()) { - // Populate the .active field with the new worker object. - found->second->SetActive(GetServiceWorker(info, false)); - } -} - void ServiceWorkerDispatcher::SetReadyRegistration( int provider_id, int registration_handle_id) { diff --git a/content/child/service_worker/service_worker_dispatcher.h b/content/child/service_worker/service_worker_dispatcher.h index d67421e6bb36..5ad6e35ace5f 100644 --- a/content/child/service_worker/service_worker_dispatcher.h +++ b/content/child/service_worker/service_worker_dispatcher.h @@ -205,18 +205,6 @@ class ServiceWorkerDispatcher : public WorkerTaskRunner::Observer { int request_id, int provider_id); - void SetInstallingServiceWorker( - int provider_id, - int registration_handle_id, - const ServiceWorkerObjectInfo& info); - void SetWaitingServiceWorker( - int provider_id, - int registration_handle_id, - const ServiceWorkerObjectInfo& info); - void SetActiveServiceWorker( - int provider_id, - int registration_handle_id, - const ServiceWorkerObjectInfo& info); void SetReadyRegistration( int provider_id, int registration_handle_id); diff --git a/content/child/service_worker/service_worker_provider_context.cc b/content/child/service_worker/service_worker_provider_context.cc index 2bd131cd516a..326b25b6f75d 100644 --- a/content/child/service_worker/service_worker_provider_context.cc +++ b/content/child/service_worker/service_worker_provider_context.cc @@ -37,21 +37,6 @@ ServiceWorkerProviderContext::~ServiceWorkerProviderContext() { } } -ServiceWorkerHandleReference* ServiceWorkerProviderContext::installing() { - DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); - return installing_.get(); -} - -ServiceWorkerHandleReference* ServiceWorkerProviderContext::waiting() { - DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); - return waiting_.get(); -} - -ServiceWorkerHandleReference* ServiceWorkerProviderContext::active() { - DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); - return active_.get(); -} - ServiceWorkerHandleReference* ServiceWorkerProviderContext::controller() { DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); return controller_.get(); @@ -66,30 +51,55 @@ ServiceWorkerProviderContext::registration() { ServiceWorkerVersionAttributes ServiceWorkerProviderContext::GetVersionAttributes() { ServiceWorkerVersionAttributes attrs; - if (installing()) - attrs.installing = installing()->info(); - if (waiting()) - attrs.waiting = waiting()->info(); - if (active()) - attrs.active = active()->info(); + if (installing_) + attrs.installing = installing_->info(); + if (waiting_) + attrs.waiting = waiting_->info(); + if (active_) + attrs.active = active_->info(); return attrs; } +void ServiceWorkerProviderContext::SetVersionAttributes( + ChangedVersionAttributesMask mask, + const ServiceWorkerVersionAttributes& attrs) { + DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); + DCHECK(registration_); + + if (mask.installing_changed()) { + installing_ = ServiceWorkerHandleReference::Adopt( + attrs.installing, thread_safe_sender_.get()); + } + if (mask.waiting_changed()) { + waiting_ = ServiceWorkerHandleReference::Adopt( + attrs.waiting, thread_safe_sender_.get()); + } + if (mask.active_changed()) { + active_ = ServiceWorkerHandleReference::Adopt( + attrs.active, thread_safe_sender_.get()); + } +} + void ServiceWorkerProviderContext::OnAssociateRegistration( const ServiceWorkerRegistrationObjectInfo& info, const ServiceWorkerVersionAttributes& attrs) { + DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); DCHECK(!registration_); DCHECK_NE(kInvalidServiceWorkerRegistrationId, info.registration_id); DCHECK_NE(kInvalidServiceWorkerRegistrationHandleId, info.handle_id); registration_ = ServiceWorkerRegistrationHandleReference::Adopt( info, thread_safe_sender_.get()); - OnSetInstallingServiceWorker(attrs.installing); - OnSetWaitingServiceWorker(attrs.waiting); - OnSetActiveServiceWorker(attrs.active); + installing_ = ServiceWorkerHandleReference::Adopt( + attrs.installing, thread_safe_sender_.get()); + waiting_ = ServiceWorkerHandleReference::Adopt( + attrs.waiting, thread_safe_sender_.get()); + active_ = ServiceWorkerHandleReference::Adopt( + attrs.active, thread_safe_sender_.get()); } void ServiceWorkerProviderContext::OnDisassociateRegistration() { + DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); controller_.reset(); active_.reset(); waiting_.reset(); @@ -100,6 +110,8 @@ void ServiceWorkerProviderContext::OnDisassociateRegistration() { void ServiceWorkerProviderContext::OnServiceWorkerStateChanged( int handle_id, blink::WebServiceWorkerState state) { + DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); + ServiceWorkerHandleReference* which = NULL; if (handle_id == controller_handle_id()) which = controller_.get(); @@ -120,29 +132,9 @@ void ServiceWorkerProviderContext::OnServiceWorkerStateChanged( // when we support navigator.serviceWorker in dedicated workers. } -void ServiceWorkerProviderContext::OnSetInstallingServiceWorker( - const ServiceWorkerObjectInfo& info) { - DCHECK(registration_); - installing_ = - ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_.get()); -} - -void ServiceWorkerProviderContext::OnSetWaitingServiceWorker( - const ServiceWorkerObjectInfo& info) { - DCHECK(registration_); - waiting_ = - ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_.get()); -} - -void ServiceWorkerProviderContext::OnSetActiveServiceWorker( - const ServiceWorkerObjectInfo& info) { - DCHECK(registration_); - active_ = - ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_.get()); -} - void ServiceWorkerProviderContext::OnSetControllerServiceWorker( const ServiceWorkerObjectInfo& info) { + DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread()); DCHECK(registration_); // This context is is the primary owner of this handle, keeps the diff --git a/content/child/service_worker/service_worker_provider_context.h b/content/child/service_worker/service_worker_provider_context.h index 6081dda14fa1..03a1d289c42e 100644 --- a/content/child/service_worker/service_worker_provider_context.h +++ b/content/child/service_worker/service_worker_provider_context.h @@ -44,20 +44,16 @@ class ServiceWorkerProviderContext void OnDisassociateRegistration(); void OnServiceWorkerStateChanged(int handle_id, blink::WebServiceWorkerState state); - void OnSetInstallingServiceWorker(const ServiceWorkerObjectInfo& info); - void OnSetWaitingServiceWorker(const ServiceWorkerObjectInfo& info); - void OnSetActiveServiceWorker(const ServiceWorkerObjectInfo& info); void OnSetControllerServiceWorker(const ServiceWorkerObjectInfo& info); int provider_id() const { return provider_id_; } - ServiceWorkerHandleReference* installing(); - ServiceWorkerHandleReference* waiting(); - ServiceWorkerHandleReference* active(); ServiceWorkerHandleReference* controller(); ServiceWorkerRegistrationHandleReference* registration(); ServiceWorkerVersionAttributes GetVersionAttributes(); + void SetVersionAttributes(ChangedVersionAttributesMask mask, + const ServiceWorkerVersionAttributes& attrs); // Gets the handle ID of the installing Service Worker, or // kInvalidServiceWorkerHandleId if the provider does not have a -- 2.11.4.GIT