From 8148543da0da6062980504f2851afb69e4bdaa4f Mon Sep 17 00:00:00 2001 From: nhiroki Date: Sun, 31 Aug 2014 23:11:47 -0700 Subject: [PATCH] ServiceWorker: Update the install sequence as per the latest spec This change... (a) updates the install sequence based on the latest spec. Before this patch, the installing version is set after resolving the register promise. After this patch, the installing version is set before resolving the promise. (b) fills registration's version attributes before resolving the register promise. This should fix the problem mentioned c#5 in the issue and should absorb the change of the timing to set the installing version due to (a). (c) adds a separate IPC for updatefound event to adjust the timing to fire the event. Before this patch, the event shares SetVersionAttributes message. [1] Blink: https://codereview.chromium.org/524193003/ [2] Chromium: THIS PATCH [3] Blink: https://codereview.chromium.org/517223002/ Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#installation-algorithm BUG=406240 TEST=content_unittests --gtest_filter=ServiceWorker* TEST=run_webkit_tests.py http/tests/serviceworker/ Review URL: https://codereview.chromium.org/517493002 Cr-Commit-Position: refs/heads/master@{#292838} --- .../service_worker_dispatcher_host.cc | 11 +++++- .../service_worker/service_worker_job_unittest.cc | 18 +++++++++- .../service_worker/service_worker_register_job.cc | 13 +++---- .../service_worker/service_worker_registration.cc | 4 +++ .../service_worker/service_worker_registration.h | 3 ++ .../service_worker_registration_handle.cc | 42 +++++++++++++--------- .../service_worker_registration_handle.h | 7 ++-- .../service_worker_registration_unittest.cc | 10 ++++++ .../service_worker/service_worker_dispatcher.cc | 23 +++++++++--- .../service_worker/service_worker_dispatcher.h | 5 ++- .../service_worker_message_filter.cc | 17 ++++++--- .../service_worker/service_worker_message_filter.h | 5 +-- .../service_worker/web_service_worker_impl.cc | 3 +- .../service_worker/service_worker_messages.h | 11 ++++-- 14 files changed, 128 insertions(+), 44 deletions(-) diff --git a/content/browser/service_worker/service_worker_dispatcher_host.cc b/content/browser/service_worker/service_worker_dispatcher_host.cc index d20bbbeccb96..44d5b796cc17 100644 --- a/content/browser/service_worker/service_worker_dispatcher_host.cc +++ b/content/browser/service_worker/service_worker_dispatcher_host.cc @@ -376,11 +376,20 @@ void ServiceWorkerDispatcherHost::RegistrationComplete( new ServiceWorkerRegistrationHandle( GetContext()->AsWeakPtr(), this, provider_id, registration)); info = new_handle->GetObjectInfo(); + handle = new_handle.get(); RegisterServiceWorkerRegistrationHandle(new_handle.Pass()); } + ServiceWorkerVersionAttributes attrs; + attrs.installing = handle->CreateServiceWorkerHandleAndPass( + registration->installing_version()); + attrs.waiting = handle->CreateServiceWorkerHandleAndPass( + registration->waiting_version()); + attrs.active = handle->CreateServiceWorkerHandleAndPass( + registration->active_version()); + Send(new ServiceWorkerMsg_ServiceWorkerRegistered( - thread_id, request_id, info)); + thread_id, request_id, info, attrs)); } void ServiceWorkerDispatcherHost::OnWorkerReadyForInspection( diff --git a/content/browser/service_worker/service_worker_job_unittest.cc b/content/browser/service_worker/service_worker_job_unittest.cc index dd2c165450ba..879316950dcf 100644 --- a/content/browser/service_worker/service_worker_job_unittest.cc +++ b/content/browser/service_worker/service_worker_job_unittest.cc @@ -894,7 +894,8 @@ class UpdateJobTestHelper }; UpdateJobTestHelper(int mock_render_process_id) - : EmbeddedWorkerTestHelper(mock_render_process_id) {} + : EmbeddedWorkerTestHelper(mock_render_process_id), + update_found_(false) {} virtual ~UpdateJobTestHelper() { if (registration_.get()) registration_->RemoveListener(this); @@ -972,6 +973,17 @@ class UpdateJobTestHelper NOTREACHED(); } + virtual void OnRegistrationFinishedUninstalling( + ServiceWorkerRegistration* registration) OVERRIDE { + NOTREACHED(); + } + + virtual void OnUpdateFound( + ServiceWorkerRegistration* registration) OVERRIDE { + ASSERT_FALSE(update_found_); + update_found_ = true; + } + // ServiceWorkerVersion::Listener overrides virtual void OnVersionStateChanged(ServiceWorkerVersion* version) OVERRIDE { StateChangeLogEntry entry; @@ -984,6 +996,7 @@ class UpdateJobTestHelper std::vector attribute_change_log_; std::vector state_change_log_; + bool update_found_; }; } // namespace @@ -1024,6 +1037,7 @@ TEST_F(ServiceWorkerJobTest, Update_NoChange) { update_helper->state_change_log_[0].version_id); EXPECT_EQ(ServiceWorkerVersion::REDUNDANT, update_helper->state_change_log_[0].status); + EXPECT_FALSE(update_helper->update_found_); } TEST_F(ServiceWorkerJobTest, Update_NewVersion) { @@ -1104,6 +1118,8 @@ TEST_F(ServiceWorkerJobTest, Update_NewVersion) { update_helper->state_change_log_[4].version_id); EXPECT_EQ(ServiceWorkerVersion::ACTIVATED, update_helper->state_change_log_[4].status); + + EXPECT_TRUE(update_helper->update_found_); } TEST_F(ServiceWorkerJobTest, Update_NewestVersionChanged) { diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc index 8b3f388b019d..5c96d87a364e 100644 --- a/content/browser/service_worker/service_worker_register_job.cc +++ b/content/browser/service_worker/service_worker_register_job.cc @@ -339,24 +339,25 @@ void ServiceWorkerRegisterJob::OnStartWorkerFinished( return; } - // "Resolve promise with serviceWorker." - DCHECK(!registration()->installing_version()); - ResolvePromise(status, registration(), new_version()); InstallAndContinue(); } -// This function corresponds to the spec's _Install algorithm. +// This function corresponds to the spec's [[Install]] algorithm. void ServiceWorkerRegisterJob::InstallAndContinue() { SetPhase(INSTALL); - // "3. Set registration.installingWorker to worker." + // "2. Set registration.installingWorker to worker." registration()->SetInstallingVersion(new_version()); + // "3. Resolve promise with registration." + ResolvePromise(SERVICE_WORKER_OK, registration(), new_version()); + // "4. Run the [[UpdateState]] algorithm passing registration.installingWorker // and "installing" as the arguments." new_version()->SetStatus(ServiceWorkerVersion::INSTALLING); - // TODO(nhiroki,michaeln): "5. Fire a simple event named updatefound..." + // "5. Fire a simple event named updatefound..." + registration()->NotifyUpdateFound(); // "6. Fire an event named install..." new_version()->DispatchInstallEvent( diff --git a/content/browser/service_worker/service_worker_registration.cc b/content/browser/service_worker/service_worker_registration.cc index 51bd701c358c..33fdfa6767fe 100644 --- a/content/browser/service_worker/service_worker_registration.cc +++ b/content/browser/service_worker/service_worker_registration.cc @@ -66,6 +66,10 @@ void ServiceWorkerRegistration::NotifyRegistrationFailed() { FOR_EACH_OBSERVER(Listener, listeners_, OnRegistrationFailed(this)); } +void ServiceWorkerRegistration::NotifyUpdateFound() { + FOR_EACH_OBSERVER(Listener, listeners_, OnUpdateFound(this)); +} + ServiceWorkerRegistrationInfo ServiceWorkerRegistration::GetInfo() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); return ServiceWorkerRegistrationInfo( diff --git a/content/browser/service_worker/service_worker_registration.h b/content/browser/service_worker/service_worker_registration.h index 55d527e8296c..caa98a2cc8f0 100644 --- a/content/browser/service_worker/service_worker_registration.h +++ b/content/browser/service_worker/service_worker_registration.h @@ -39,6 +39,8 @@ class CONTENT_EXPORT ServiceWorkerRegistration ServiceWorkerRegistration* registration) {} virtual void OnRegistrationFinishedUninstalling( ServiceWorkerRegistration* registration) {} + virtual void OnUpdateFound( + ServiceWorkerRegistration* registration) {} }; ServiceWorkerRegistration(const GURL& pattern, @@ -70,6 +72,7 @@ class CONTENT_EXPORT ServiceWorkerRegistration void AddListener(Listener* listener); void RemoveListener(Listener* listener); void NotifyRegistrationFailed(); + void NotifyUpdateFound(); ServiceWorkerRegistrationInfo GetInfo(); diff --git a/content/browser/service_worker/service_worker_registration_handle.cc b/content/browser/service_worker/service_worker_registration_handle.cc index b32f5a92b9fe..21aa31ef1692 100644 --- a/content/browser/service_worker/service_worker_registration_handle.cc +++ b/content/browser/service_worker/service_worker_registration_handle.cc @@ -45,6 +45,23 @@ ServiceWorkerRegistrationHandle::GetObjectInfo() { return info; } +ServiceWorkerObjectInfo +ServiceWorkerRegistrationHandle::CreateServiceWorkerHandleAndPass( + ServiceWorkerVersion* version) { + ServiceWorkerObjectInfo info; + if (context_ && version) { + scoped_ptr handle = + ServiceWorkerHandle::Create(context_, + dispatcher_host_, + kDocumentMainThreadId, + provider_id_, + version); + info = handle->GetObjectInfo(); + dispatcher_host_->RegisterServiceWorkerHandle(handle.Pass()); + } + return info; +} + void ServiceWorkerRegistrationHandle::IncrementRefCount() { DCHECK_GT(ref_count_, 0); ++ref_count_; @@ -71,6 +88,14 @@ void ServiceWorkerRegistrationHandle::OnRegistrationFailed( ClearVersionAttributes(); } +void ServiceWorkerRegistrationHandle::OnUpdateFound( + ServiceWorkerRegistration* registration) { + if (!dispatcher_host_) + return; // Could be NULL in some tests. + dispatcher_host_->Send(new ServiceWorkerMsg_UpdateFound( + kDocumentMainThreadId, GetObjectInfo())); +} + void ServiceWorkerRegistrationHandle::SetVersionAttributes( ServiceWorkerVersion* installing_version, ServiceWorkerVersion* waiting_version, @@ -118,21 +143,4 @@ void ServiceWorkerRegistrationHandle::ClearVersionAttributes() { SetVersionAttributes(NULL, NULL, NULL); } -ServiceWorkerObjectInfo -ServiceWorkerRegistrationHandle::CreateServiceWorkerHandleAndPass( - ServiceWorkerVersion* version) { - ServiceWorkerObjectInfo info; - if (context_ && version) { - scoped_ptr handle = - ServiceWorkerHandle::Create(context_, - dispatcher_host_, - kDocumentMainThreadId, - provider_id_, - version); - info = handle->GetObjectInfo(); - dispatcher_host_->RegisterServiceWorkerHandle(handle.Pass()); - } - return info; -} - } // namespace content diff --git a/content/browser/service_worker/service_worker_registration_handle.h b/content/browser/service_worker/service_worker_registration_handle.h index c1a3bdd5c8ad..d6d7e05fa81d 100644 --- a/content/browser/service_worker/service_worker_registration_handle.h +++ b/content/browser/service_worker/service_worker_registration_handle.h @@ -34,6 +34,8 @@ class ServiceWorkerRegistrationHandle virtual ~ServiceWorkerRegistrationHandle(); ServiceWorkerRegistrationObjectInfo GetObjectInfo(); + ServiceWorkerObjectInfo CreateServiceWorkerHandleAndPass( + ServiceWorkerVersion* version); bool HasNoRefCount() const { return ref_count_ <= 0; } void IncrementRefCount(); @@ -52,6 +54,8 @@ class ServiceWorkerRegistrationHandle const ServiceWorkerRegistrationInfo& info) OVERRIDE; virtual void OnRegistrationFailed( ServiceWorkerRegistration* registration) OVERRIDE; + virtual void OnUpdateFound( + ServiceWorkerRegistration* registration) OVERRIDE; // Sets the corresponding version field to the given version or if the given // version is NULL, clears the field. @@ -63,9 +67,6 @@ class ServiceWorkerRegistrationHandle // Clears all version fields. void ClearVersionAttributes(); - ServiceWorkerObjectInfo CreateServiceWorkerHandleAndPass( - ServiceWorkerVersion* version); - base::WeakPtr context_; ServiceWorkerDispatcherHost* dispatcher_host_; const int provider_id_; diff --git a/content/browser/service_worker/service_worker_registration_unittest.cc b/content/browser/service_worker/service_worker_registration_unittest.cc index 1550d1ec6c86..c38977480599 100644 --- a/content/browser/service_worker/service_worker_registration_unittest.cc +++ b/content/browser/service_worker/service_worker_registration_unittest.cc @@ -61,6 +61,16 @@ class ServiceWorkerRegistrationTest : public testing::Test { NOTREACHED(); } + virtual void OnRegistrationFinishedUninstalling( + ServiceWorkerRegistration* registration) OVERRIDE { + NOTREACHED(); + } + + virtual void OnUpdateFound( + ServiceWorkerRegistration* registration) OVERRIDE { + NOTREACHED(); + } + void Reset() { observed_registration_ = NULL; observed_changed_mask_ = ChangedVersionAttributesMask(); diff --git a/content/child/service_worker/service_worker_dispatcher.cc b/content/child/service_worker/service_worker_dispatcher.cc index 74ec956efc85..032dfa2921d3 100644 --- a/content/child/service_worker/service_worker_dispatcher.cc +++ b/content/child/service_worker/service_worker_dispatcher.cc @@ -62,6 +62,8 @@ void ServiceWorkerDispatcher::OnMessageReceived(const IPC::Message& msg) { OnServiceWorkerStateChanged) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_SetVersionAttributes, OnSetVersionAttributes) + IPC_MESSAGE_HANDLER(ServiceWorkerMsg_UpdateFound, + OnUpdateFound) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_SetControllerServiceWorker, OnSetControllerServiceWorker) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_MessageToDocument, @@ -238,14 +240,21 @@ ServiceWorkerDispatcher::GetServiceWorkerRegistration( void ServiceWorkerDispatcher::OnRegistered( int thread_id, int request_id, - const ServiceWorkerRegistrationObjectInfo& info) { + const ServiceWorkerRegistrationObjectInfo& info, + const ServiceWorkerVersionAttributes& attrs) { WebServiceWorkerRegistrationCallbacks* callbacks = pending_callbacks_.Lookup(request_id); DCHECK(callbacks); if (!callbacks) return; - callbacks->onSuccess(GetServiceWorkerRegistration(info, true)); + WebServiceWorkerRegistrationImpl* registration = + GetServiceWorkerRegistration(info, true); + registration->SetInstalling(GetServiceWorker(attrs.installing, true)); + registration->SetWaiting(GetServiceWorker(attrs.waiting, true)); + registration->SetActive(GetServiceWorker(attrs.active, true)); + + callbacks->onSuccess(registration); pending_callbacks_.Remove(request_id); } @@ -316,6 +325,14 @@ void ServiceWorkerDispatcher::OnSetVersionAttributes( } } +void ServiceWorkerDispatcher::OnUpdateFound( + int thread_id, + const ServiceWorkerRegistrationObjectInfo& info) { + RegistrationObjectMap::iterator found = registrations_.find(info.handle_id); + if (found != registrations_.end()) + found->second->OnUpdateFound(); +} + void ServiceWorkerDispatcher::SetInstallingServiceWorker( int provider_id, int registration_handle_id, @@ -341,8 +358,6 @@ void ServiceWorkerDispatcher::SetInstallingServiceWorker( if (found != registrations_.end()) { // Populate the .installing field with the new worker object. found->second->SetInstalling(GetServiceWorker(info, false)); - if (info.handle_id != kInvalidServiceWorkerHandleId) - found->second->OnUpdateFound(); } } diff --git a/content/child/service_worker/service_worker_dispatcher.h b/content/child/service_worker/service_worker_dispatcher.h index 9997afd0cd10..3f136c28730f 100644 --- a/content/child/service_worker/service_worker_dispatcher.h +++ b/content/child/service_worker/service_worker_dispatcher.h @@ -129,7 +129,8 @@ class ServiceWorkerDispatcher : public WorkerTaskRunner::Observer { void OnRegistered(int thread_id, int request_id, - const ServiceWorkerRegistrationObjectInfo& info); + const ServiceWorkerRegistrationObjectInfo& info, + const ServiceWorkerVersionAttributes& attrs); void OnUnregistered(int thread_id, int request_id); void OnRegistrationError(int thread_id, @@ -144,6 +145,8 @@ class ServiceWorkerDispatcher : public WorkerTaskRunner::Observer { int registration_handle_id, int changed_mask, const ServiceWorkerVersionAttributes& attributes); + void OnUpdateFound(int thread_id, + const ServiceWorkerRegistrationObjectInfo& info); void OnSetControllerServiceWorker(int thread_id, int provider_id, const ServiceWorkerObjectInfo& info); diff --git a/content/child/service_worker/service_worker_message_filter.cc b/content/child/service_worker/service_worker_message_filter.cc index 15ac1d6ed4e7..72cf320084a6 100644 --- a/content/child/service_worker/service_worker_message_filter.cc +++ b/content/child/service_worker/service_worker_message_filter.cc @@ -81,7 +81,14 @@ void ServiceWorkerMessageFilter::OnStaleMessageReceived( void ServiceWorkerMessageFilter::OnStaleRegistered( int thread_id, int request_id, - const ServiceWorkerRegistrationObjectInfo& info) { + const ServiceWorkerRegistrationObjectInfo& info, + const ServiceWorkerVersionAttributes& attrs) { + SendServiceWorkerObjectDestroyed(thread_safe_sender_.get(), + attrs.installing.handle_id); + SendServiceWorkerObjectDestroyed(thread_safe_sender_.get(), + attrs.waiting.handle_id); + SendServiceWorkerObjectDestroyed(thread_safe_sender_.get(), + attrs.active.handle_id); SendRegistrationObjectDestroyed(thread_safe_sender_.get(), info.handle_id); } @@ -90,13 +97,13 @@ void ServiceWorkerMessageFilter::OnStaleSetVersionAttributes( int provider_id, int registration_handle_id, int changed_mask, - const ServiceWorkerVersionAttributes& attributes) { + const ServiceWorkerVersionAttributes& attrs) { SendServiceWorkerObjectDestroyed(thread_safe_sender_.get(), - attributes.installing.handle_id); + attrs.installing.handle_id); SendServiceWorkerObjectDestroyed(thread_safe_sender_.get(), - attributes.waiting.handle_id); + attrs.waiting.handle_id); SendServiceWorkerObjectDestroyed(thread_safe_sender_.get(), - attributes.active.handle_id); + attrs.active.handle_id); SendRegistrationObjectDestroyed(thread_safe_sender_.get(), registration_handle_id); } diff --git a/content/child/service_worker/service_worker_message_filter.h b/content/child/service_worker/service_worker_message_filter.h index 40a67bca280f..dfe0ad7ab066 100644 --- a/content/child/service_worker/service_worker_message_filter.h +++ b/content/child/service_worker/service_worker_message_filter.h @@ -38,13 +38,14 @@ class CONTENT_EXPORT ServiceWorkerMessageFilter void OnStaleRegistered( int thread_id, int request_id, - const ServiceWorkerRegistrationObjectInfo& info); + const ServiceWorkerRegistrationObjectInfo& info, + const ServiceWorkerVersionAttributes& attrs); void OnStaleSetVersionAttributes( int thread_id, int provider_id, int registration_handle_id, int changed_mask, - const ServiceWorkerVersionAttributes& attributes); + const ServiceWorkerVersionAttributes& attrs); void OnStaleSetControllerServiceWorker( int thread_id, int provider_id, diff --git a/content/child/service_worker/web_service_worker_impl.cc b/content/child/service_worker/web_service_worker_impl.cc index e76822362ed3..dcd69ca8a902 100644 --- a/content/child/service_worker/web_service_worker_impl.cc +++ b/content/child/service_worker/web_service_worker_impl.cc @@ -43,8 +43,7 @@ WebServiceWorkerImpl::~WebServiceWorkerImpl() { void WebServiceWorkerImpl::OnStateChanged( blink::WebServiceWorkerState new_state) { - DCHECK(proxy_); - if (proxy_->isReady()) + if (proxy_ && proxy_->isReady()) CommitState(new_state); else queued_states_.push_back(new_state); diff --git a/content/common/service_worker/service_worker_messages.h b/content/common/service_worker/service_worker_messages.h index 1362f0952c58..42943e5ecf52 100644 --- a/content/common/service_worker/service_worker_messages.h +++ b/content/common/service_worker/service_worker_messages.h @@ -185,10 +185,11 @@ IPC_MESSAGE_ROUTED1(ServiceWorkerHostMsg_CacheStorageKeys, // on the correct thread. // Response to ServiceWorkerMsg_RegisterServiceWorker. -IPC_MESSAGE_CONTROL3(ServiceWorkerMsg_ServiceWorkerRegistered, +IPC_MESSAGE_CONTROL4(ServiceWorkerMsg_ServiceWorkerRegistered, int /* thread_id */, int /* request_id */, - content::ServiceWorkerRegistrationObjectInfo) + content::ServiceWorkerRegistrationObjectInfo, + content::ServiceWorkerVersionAttributes) // Response to ServiceWorkerMsg_UnregisterServiceWorker. IPC_MESSAGE_CONTROL2(ServiceWorkerMsg_ServiceWorkerUnregistered, @@ -217,6 +218,12 @@ IPC_MESSAGE_CONTROL5(ServiceWorkerMsg_SetVersionAttributes, int /* changed_mask */, content::ServiceWorkerVersionAttributes) +// Informs the child process that new ServiceWorker enters the installation +// phase. +IPC_MESSAGE_CONTROL2(ServiceWorkerMsg_UpdateFound, + int /* thread_id */, + content::ServiceWorkerRegistrationObjectInfo) + // Tells the child process to set the controller ServiceWorker for the given // provider. IPC_MESSAGE_CONTROL3(ServiceWorkerMsg_SetControllerServiceWorker, -- 2.11.4.GIT