[Service Worker] Fix service worker activates too early when installi…

…ng (youtube#547) (youtube#564) b/234788479 (cherry picked from commit 51d20e3) Co-authored-by: Sherry Zhou <[email protected]>
kaidokert · Jun 8, 2023 · c9b8ee5 · c9b8ee5
1 parent 24c63b3
commit c9b8ee5
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 48 deletions.
diff --git a/cobalt/layout_tests/testdata/web-platform-tests/service-workers/web_platform_tests.txt b/cobalt/layout_tests/testdata/web-platform-tests/service-workers/web_platform_tests.txt
@@ -1,19 +1,26 @@
 # Service Worker API tests
 
+service-worker/activation-after-registration.https.html, PASS
+service-worker/activate-event-after-install-state-change.https.html, PASS
 service-worker/clients-matchall-on-evaluation.https.html, PASS
 service-worker/fetch-event-add-async.https.html, PASS
+service-worker/import-scripts-cross-origin.https.html, PASS
 service-worker/import-scripts-resource-map.https.html, PASS
 service-worker/import-scripts-updated-flag.https.html, PASS
 service-worker/register-default-scope.https.html, PASS
 service-worker/registration-basic.https.html, PASS
 service-worker/registration-security-error.https.html, PASS
 service-worker/registration-script-url.https.html, PASS
 service-worker/rejections.https.html, PASS
+service-worker/serviceworkerobject-scripturl.https.html, PASS
 service-worker/service-worker-csp-default.https.html, PASS
 service-worker/service-worker-csp-connect.https.html, PASS
 service-worker/service-worker-csp-script.https.html, PASS
+service-worker/service-worker-header.https.html, PASS
 service-worker/Service-Worker-Allowed-header.https.html, PASS
 service-worker/skip-waiting-without-client.https.html, PASS
+service-worker/state.https.html, PASS
+service-worker/synced-state.https.html, PASS
 service-worker/uncontrolled-page.https.html, PASS
 service-worker/unregister.https.html, PASS
 service-worker/update-no-cache-request-headers.https.html, PASS
@@ -28,17 +35,10 @@ service-worker/update-missing-import-scripts.https.html, DISABLE
 service-worker/import-scripts-mime-types.https.html, DISABLE
 
 # b/234788479 Implement waiting for update worker state tasks in Install algorithm.
-service-worker/activation-after-registration.https.html, DISABLE
-service-worker/activate-event-after-install-state-change.https.html, DISABLE
-service-worker/import-scripts-cross-origin.https.html, DISABLE
 service-worker/import-scripts-redirect.https.html, DISABLE
 service-worker/multiple-update.https.html, DISABLE
 service-worker/register-wait-forever-in-install-worker.https.html, DISABLE
 service-worker/registration-service-worker-attributes.https.html, DISABLE
-service-worker/service-worker-header.https.html, DISABLE
-service-worker/state.https.html, DISABLE
-service-worker/synced-state.https.html, DISABLE
-service-worker/serviceworkerobject-scripturl.https.html, DISABLE
 
 # "Module" type of dedicated worker is supported in Cobalt
 service-worker/dedicated-worker-service-worker-interception.https.html, DISABLE

diff --git a/cobalt/worker/service_worker_jobs.cc b/cobalt/worker/service_worker_jobs.cc
@@ -1678,9 +1678,9 @@ void ServiceWorkerJobs::UpdateRegistrationState(
       // 2.2. For each registrationObject in registrationObjects:
       for (auto& context : web_context_registrations_) {
         // 2.2.1. Queue a task to...
-        context->message_loop()->task_runner()->PostTask(
+        context->message_loop()->task_runner()->PostBlockingTask(
             FROM_HERE,
-            base::BindOnce(
+            base::Bind(
                 [](web::Context* context,
                    ServiceWorkerRegistrationObject* registration) {
                   // 2.2.1. ... set the installing attribute of
@@ -1708,9 +1708,9 @@ void ServiceWorkerJobs::UpdateRegistrationState(
       // 3.2. For each registrationObject in registrationObjects:
       for (auto& context : web_context_registrations_) {
         // 3.2.1. Queue a task to...
-        context->message_loop()->task_runner()->PostTask(
+        context->message_loop()->task_runner()->PostBlockingTask(
             FROM_HERE,
-            base::BindOnce(
+            base::Bind(
                 [](web::Context* context,
                    ServiceWorkerRegistrationObject* registration) {
                   // 3.2.1. ... set the waiting attribute of registrationObject
@@ -1736,9 +1736,9 @@ void ServiceWorkerJobs::UpdateRegistrationState(
       // 4.2. For each registrationObject in registrationObjects:
       for (auto& context : web_context_registrations_) {
         // 4.2.1. Queue a task to...
-        context->message_loop()->task_runner()->PostTask(
+        context->message_loop()->task_runner()->PostBlockingTask(
             FROM_HERE,
-            base::BindOnce(
+            base::Bind(
                 [](web::Context* context,
                    ServiceWorkerRegistrationObject* registration) {
                   // 4.2.1. ... set the active attribute of registrationObject
@@ -1786,34 +1786,31 @@ void ServiceWorkerJobs::UpdateWorkerState(ServiceWorkerObject* worker,
       // 4. ... queue a task on
       //    settingsObject's responsible event loop in the DOM manipulation task
       //    source to run the following steps:
-      context->message_loop()->task_runner()->PostTask(
-          FROM_HERE,
-          base::BindOnce(
-              [](web::Context* context, ServiceWorkerObject* worker,
-                 ServiceWorkerState state) {
-                DCHECK_EQ(context->message_loop(),
-                          base::MessageLoop::current());
-                // 4.1. Let objectMap be settingsObject's service worker object
-                //      map.
-                // 4.2. If objectMap[worker] does not exist, then abort these
-                //      steps.
-                // 4.3. Let  workerObj be objectMap[worker].
-                auto worker_obj = context->LookupServiceWorker(worker);
-                if (worker_obj) {
-                  // 4.4. Set workerObj's state to state.
-                  worker_obj->set_state(state);
-                  // 4.5. Fire an event named statechange at workerObj.
-                  context->message_loop()->task_runner()->PostTask(
-                      FROM_HERE,
-                      base::BindOnce(
-                          [](scoped_refptr<ServiceWorker> worker_obj) {
-                            worker_obj->DispatchEvent(
-                                new web::Event(base::Tokens::statechange()));
-                          },
-                          worker_obj));
-                }
-              },
-              context, base::Unretained(worker), state));
+      context->message_loop()->task_runner()->PostBlockingTask(
+          FROM_HERE, base::Bind(
+                         [](web::Context* context, ServiceWorkerObject* worker,
+                            ServiceWorkerState state) {
+                           DCHECK_EQ(context->message_loop(),
+                                     base::MessageLoop::current());
+                           // 4.1. Let objectMap be settingsObject's service
+                           // worker object
+                           //      map.
+                           // 4.2. If objectMap[worker] does not exist, then
+                           // abort these
+                           //      steps.
+                           // 4.3. Let  workerObj be objectMap[worker].
+                           auto worker_obj =
+                               context->LookupServiceWorker(worker);
+                           if (worker_obj) {
+                             // 4.4. Set workerObj's state to state.
+                             worker_obj->set_state(state);
+                             // 4.5. Fire an event named statechange at
+                             // workerObj.
+                             worker_obj->DispatchEvent(
+                                 new web::Event(base::Tokens::statechange()));
+                           }
+                         },
+                         context, base::Unretained(worker), state));
     }
   }
 }

diff --git a/third_party/web_platform_tests/service-workers/service-worker/resources/test-helpers.sub.js b/third_party/web_platform_tests/service-workers/service-worker/resources/test-helpers.sub.js
@@ -86,9 +86,7 @@ function wait_for_update(test, registration) {
   return new Promise(test.step_func(function(resolve) {
       var handler = test.step_func(function() {
         registration.removeEventListener('updatefound', handler);
-        // b/234788479 Implement waiting for update worker state tasks in
-        // Install algorithm, otherwise the worker is activated too early
-        resolve(get_newest_worker(registration));
+        resolve(registration.installing);
       });
       registration.addEventListener('updatefound', handler);
     }));

diff --git a/..._platform_tests/service-workers/service-worker/update-no-cache-request-headers.https.html b/..._platform_tests/service-workers/service-worker/update-no-cache-request-headers.https.html
@@ -32,10 +32,7 @@
   // Do an update.
   await registration.update();
 
-  // Ask the new worker what the request headers were.
-  // b/234788479 Implement waiting for update worker state tasks in
-  // Install algorithm, otherwise the worker is activated too early
-  const newWorker = get_newest_worker(registration);
+  const newWorker = registration.installing;
   const sawMessage = new Promise((resolve) => {
     navigator.serviceWorker.onmessage = (event) => {
       resolve(event.data);