From 7897f9d5beff624590d2fc254d2a7eca6e2b1e8f Mon Sep 17 00:00:00 2001 From: jugglinmike Date: Fri, 21 Sep 2018 18:12:00 -0400 Subject: [PATCH] [service-workers] Return value in cleanup (#13163) Previously, many tests un-registered service workers using the testharness.js API `add_cleanup` without returning the promise which tracked the operation. While this ensured the operation was started at the completion of the test, it did not guarantee that the operation would be completed before the test was considered "complete." In automation scenarios where many tests are executed in rapid succession, this enabled a race condition: the navigation could interrupt the un-registration process. In order to account for the possibility of registrations that persisted from previous test failures, the tests included "setup" code to defensively un-register such workers. The `Test#add_cleanup` method was recently extended to support asynchronous "clean up" operations [1]. Use that API to ensure tests are not considered "complete" until after service worker un-registration is done. [1] https://github.com/web-platform-tests/wpt/pull/8748 --- .../fetch-request-xhr-sync.https.html | 2 +- .../interfaces-window.https.html | 2 +- .../onactivate-script-error.https.html | 2 +- .../service-worker/postmessage.https.html | 18 +++--------------- ...ration-service-worker-attributes.https.html | 2 +- .../resources/worker-testharness.js | 2 +- .../skip-waiting-using-registration.https.html | 2 +- ...iting-without-using-registration.https.html | 2 +- .../service-worker/synced-state.https.html | 2 +- 9 files changed, 11 insertions(+), 23 deletions(-) diff --git a/service-workers/service-worker/fetch-request-xhr-sync.https.html b/service-workers/service-worker/fetch-request-xhr-sync.https.html index f1e4fecc1b35ba..ec27fb898343e7 100644 --- a/service-workers/service-worker/fetch-request-xhr-sync.https.html +++ b/service-workers/service-worker/fetch-request-xhr-sync.https.html @@ -14,7 +14,7 @@ return service_worker_unregister_and_register(t, url, scope) .then(function(registration) { t.add_cleanup(function() { - registration.unregister(); + return registration.unregister(); }); return wait_for_state(t, registration.installing, 'activated'); diff --git a/service-workers/service-worker/interfaces-window.https.html b/service-workers/service-worker/interfaces-window.https.html index 85d7f6467ee989..2c131b3c93a600 100644 --- a/service-workers/service-worker/interfaces-window.https.html +++ b/service-workers/service-worker/interfaces-window.https.html @@ -40,7 +40,7 @@ t, 'resources/empty-worker.js', scope) .then(function(registration) { t.add_cleanup(function() { - registration.unregister(); + return registration.unregister(); }); window.registrationInstance = registration; diff --git a/service-workers/service-worker/onactivate-script-error.https.html b/service-workers/service-worker/onactivate-script-error.https.html index 6c16357f3cc5ab..f5e80bb9a456bc 100644 --- a/service-workers/service-worker/onactivate-script-error.https.html +++ b/service-workers/service-worker/onactivate-script-error.https.html @@ -35,7 +35,7 @@ registration = r; t.add_cleanup(function() { - r.unregister(); + return r.unregister(); }); return wait_for_install(registration.installing); diff --git a/service-workers/service-worker/postmessage.https.html b/service-workers/service-worker/postmessage.https.html index 3d73afe9796114..b1f3342a8c14e7 100644 --- a/service-workers/service-worker/postmessage.https.html +++ b/service-workers/service-worker/postmessage.https.html @@ -13,11 +13,7 @@ return service_worker_unregister_and_register(t, script, scope) .then(r => { - // TODO: return the Promise created by `r.unregister`once - // `testharness.js` has been updated to honor thenables returned by - // cleanup functions. - // See https://github.com/web-platform-tests/wpt/pull/8748 - t.add_cleanup(() => { r.unregister(); }); + t.add_cleanup(() => r.unregister()); registration = r; worker = registration.installing; @@ -66,11 +62,7 @@ return service_worker_unregister_and_register(t, script, scope) .then(r => { - // TODO: return the Promise created by `r.unregister`once - // `testharness.js` has been updated to honor thenables returned by - // cleanup functions. - // See https://github.com/web-platform-tests/wpt/pull/8748 - t.add_cleanup(() => { r.unregister(); }); + t.add_cleanup(() => r.unregister()); var ab = text_encoder.encode(message); assert_equals(ab.byteLength, message.length); @@ -110,11 +102,7 @@ return service_worker_unregister_and_register(t, script, scope) .then(r => { - // TODO: return the Promise created by `r.unregister`once - // `testharness.js` has been updated to honor thenables returned by - // cleanup functions. - // See https://github.com/web-platform-tests/wpt/pull/8748 - t.add_cleanup(() => { r.unregister(); }); + t.add_cleanup(() => r.unregister()); var channel = new MessageChannel; port = channel.port1; diff --git a/service-workers/service-worker/registration-service-worker-attributes.https.html b/service-workers/service-worker/registration-service-worker-attributes.https.html index 8fd566dadc62a7..f7b52d5ddced18 100644 --- a/service-workers/service-worker/registration-service-worker-attributes.https.html +++ b/service-workers/service-worker/registration-service-worker-attributes.https.html @@ -14,7 +14,7 @@ return service_worker_unregister_and_register(t, worker_url, scope) .then(function(r) { t.add_cleanup(function() { - r.unregister(); + return r.unregister(); }); registration = r; newest_worker = registration.installing; diff --git a/service-workers/service-worker/resources/worker-testharness.js b/service-workers/service-worker/resources/worker-testharness.js index fdf5868e33e2fe..73e97be1eaa44b 100644 --- a/service-workers/service-worker/resources/worker-testharness.js +++ b/service-workers/service-worker/resources/worker-testharness.js @@ -20,7 +20,7 @@ importScripts('/resources/testharness.js'); var cache_name = self.location.pathname + '/' + uniquifier; test.add_cleanup(function() { - self.caches.delete(cache_name); + return self.caches.delete(cache_name); }); return self.caches.delete(cache_name) diff --git a/service-workers/service-worker/skip-waiting-using-registration.https.html b/service-workers/service-worker/skip-waiting-using-registration.https.html index a9c398a260676d..67838acff46698 100644 --- a/service-workers/service-worker/skip-waiting-using-registration.https.html +++ b/service-workers/service-worker/skip-waiting-using-registration.https.html @@ -51,7 +51,7 @@ .then(function(registration) { sw_registration = registration; t.add_cleanup(function() { - registration.unregister(); + return registration.unregister(); }); return saw_controllerchanged; }) diff --git a/service-workers/service-worker/skip-waiting-without-using-registration.https.html b/service-workers/service-worker/skip-waiting-without-using-registration.https.html index dbe2bde78d3c0b..705fe8355e125c 100644 --- a/service-workers/service-worker/skip-waiting-without-using-registration.https.html +++ b/service-workers/service-worker/skip-waiting-without-using-registration.https.html @@ -27,7 +27,7 @@ .then(function(registration) { sw_registration = registration; t.add_cleanup(function() { - registration.unregister(); + return registration.unregister(); }); return wait_for_state(t, registration.installing, 'activated'); }) diff --git a/service-workers/service-worker/synced-state.https.html b/service-workers/service-worker/synced-state.https.html index c4a9b6655cb066..0e9f63a9a2e528 100644 --- a/service-workers/service-worker/synced-state.https.html +++ b/service-workers/service-worker/synced-state.https.html @@ -33,7 +33,7 @@ worker = registration.installing; t.add_cleanup(function() { - r.unregister(); + return r.unregister(); }); return nextChange(worker);