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}

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(

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<AttributeChangeLogEntry> attribute_change_log_;
   std::vector<StateChangeLogEntry> 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) {

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(

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(

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();
 

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<ServiceWorkerHandle> 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<ServiceWorkerHandle> handle =
-        ServiceWorkerHandle::Create(context_,
-                                    dispatcher_host_,
-                                    kDocumentMainThreadId,
-                                    provider_id_,
-                                    version);
-    info = handle->GetObjectInfo();
-    dispatcher_host_->RegisterServiceWorkerHandle(handle.Pass());
-  }
-  return info;
-}
-
 }  // namespace content

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<ServiceWorkerContextCore> context_;
   ServiceWorkerDispatcherHost* dispatcher_host_;
   const int provider_id_;

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();

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();
   }
 }
 

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);

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);
 }

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,

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);

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,