Bug 1536619 [wpt PR 15852] - service worker: Improve WPT tests for async respondWith/waitUntil., a=testonly

Automatic update from web-platform-tests service worker: Improve WPT tests for async respondWith/waitUntil. (#15852) See discussion at [1] and [2]. This makes the following changes. 1. Adds a test for: self.addEventListener('fetch', e => { Promise.resolve().then(() => { e.respondWith(new Response('hi')); }); }); This should not throw because respondWith() is called while the event dispatch flag is still set. The microtask checkpoint is in "Cleanup After Running Scripts" here: https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script This is called from step 16.2 here: https://heycam.github.io/webidl/#call-a-user-objects-operation Which in turn is called from the DOM spec's "Inner Invoke" to call event targets: https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke 2. Changes the expectation for: addEventListener('message', event => { Promise.resolve().then(event.waitUntil(p)); }); From throws to not throws, for the same reasoning as above. 3. Changes the expectation for: addEventListener('message', event => { waitPromise = Promise.resolve(); event.waitUntil(waitPromise); waitPromise.then(() => { Promise.resolve().then(() => {event.waitUntil();}); }); }); From throws to not throws. This is subtle. Because all the promises are just Promise.resolve(), the event dispatch flag is still set by the time the second waitUntil() is called. 4. To test what 3. originally intended, a new test is added which makes waitPromise a promise that does not immediately resolve. 5. Changes the expectation for: addEventListener(‘fetch’, event => { response = Promise.resolve(new Response('RESP')); event.respondWith(response); response.then(() => { Promise.resolve().then(() => {event.waitUntil();}); }) }); Again this is because the promises used resolve immediately, so the event dispatch flag is still set. Similarly, a new test is added to cover the original intent. These WPT changes appear to match the behavior of Safari and Edge while diverging from Chrome and (partially) Firefox. [1] https://github.com/w3c/ServiceWorker/issues/1213 [2] https://github.com/w3c/ServiceWorker/issues/1394 Bug: 942414 Change-Id: I9a4a56d71d3919ed614ff78df2bdc6cc0251dadd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1524393 Commit-Queue: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Ben Kelly <wanderview@chromium.org> Cr-Commit-Position: refs/heads/master@{#641514} -- wpt-commits: cecb3eba4dae3d795876f7b4be71bd49afa03356 wpt-pr: 15852
2019-04-15 14:34:45 +00:00 · 2019-04-15 14:34:45 +00:00 · 43a34a8adf
--- a/testing/web-platform/tests/service-workers/service-worker/extendable-event-async-waituntil.https.html
+++ b/testing/web-platform/tests/service-workers/service-worker/extendable-event-async-waituntil.https.html
@ -1,4 +1,5 @@
 <!DOCTYPE html>
+<meta name="timeout" content="long">
 <script src="/resources/testharness.js"></script>
 <script src="resources/testharness-helpers.js"></script>
 <script src="/resources/testharnessreport.js"></script>
@ -56,21 +57,25 @@ function msg_event_test(scope, test) {
 }

 promise_test(msg_event_test.bind(this, 'no-current-extension-different-task'),
-  'Test calling waitUntil in a different task without an existing extension throws');
+  'Test calling waitUntil in a task at the end of the event handler without an existing extension throws');

 promise_test(msg_event_test.bind(this, 'no-current-extension-different-microtask'),
-  'Test calling waitUntil in a different microtask without an existing extension throws');
+  'Test calling waitUntil in a microtask at the end of the event handler without an existing extension suceeds');

 promise_test(msg_event_test.bind(this, 'current-extension-different-task'),
-  'Test calling waitUntil in a different task with an existing extension succeeds');
+  'Test calling waitUntil in a different task an existing extension succeeds');

-promise_test(msg_event_test.bind(this, 'current-extension-expired-same-microtask-turn'),
-  'Test calling waitUntil with an existing extension promise handler succeeds');
+promise_test(msg_event_test.bind(this, 'during-event-dispatch-current-extension-expired-same-microtask-turn'),
+  'Test calling waitUntil at the end of an existing extension promise handler succeeds (event is still being dispatched)');

-// The promise handler will queue a new microtask after the check for new
-// extensions was performed.
-promise_test(msg_event_test.bind(this, 'current-extension-expired-same-microtask-turn-extra'),
-  'Test calling waitUntil at the end of the microtask turn throws');
+promise_test(msg_event_test.bind(this, 'during-event-dispatch-current-extension-expired-same-microtask-turn-extra'),
+  'Test calling waitUntil in a microtask at the end of an existing extension promise handler succeeds (event is still being dispatched)');
+
+promise_test(msg_event_test.bind(this, 'after-event-dispatch-current-extension-expired-same-microtask-turn'),
+  'Test calling waitUntil in an existing extension promise handler succeeds (event is not being dispatched)');
+
+promise_test(msg_event_test.bind(this, 'after-event-dispatch-current-extension-expired-same-microtask-turn-extra'),
+  'Test calling waitUntil in a microtask at the end of an existing extension promise handler throws (event is not being dispatched)');

 promise_test(msg_event_test.bind(this, 'current-extension-expired-different-task'),
  'Test calling waitUntil after the current extension expired in a different task fails');
@ -80,24 +85,36 @@ promise_test(msg_event_test.bind(this, 'script-extendable-event'),

 promise_test(function(t) {
    var testBody = function(worker) {
-      return with_iframe('./resources/pending-respondwith-async-waituntil/dummy.html');
+      return with_iframe('./resources/pending-respondwith-async-waituntil');
    }
    return runTest(t, 'pending-respondwith-async-waituntil', testBody);
  }, 'Test calling waitUntil asynchronously with pending respondWith promise.');

 promise_test(function(t) {
    var testBody = function(worker) {
-      return with_iframe('./resources/respondwith-microtask-sync-waituntil/dummy.html');
+      return with_iframe('./resources/during-event-dispatch-respondwith-microtask-sync-waituntil');
    }
-    return runTest(t, 'respondwith-microtask-sync-waituntil', testBody);
-  }, 'Test calling waitUntil synchronously inside microtask of respondWith promise.');
+    return runTest(t, 'during-event-dispatch-respondwith-microtask-sync-waituntil', testBody);
+  }, 'Test calling waitUntil synchronously inside microtask of respondWith promise (event is being dispatched).');

 promise_test(function(t) {
    var testBody = function(worker) {
-      return with_iframe('./resources/respondwith-microtask-async-waituntil/dummy.html');
+      return with_iframe('./resources/during-event-dispatch-respondwith-microtask-async-waituntil');
    }
-    return runTest(t, 'respondwith-microtask-async-waituntil', testBody);
-  }, 'Test calling waitUntil asynchronously inside microtask of respondWith promise.');
+    return runTest(t, 'during-event-dispatch-respondwith-microtask-async-waituntil', testBody);
+  }, 'Test calling waitUntil asynchronously inside microtask of respondWith promise (event is being dispatched).');

+promise_test(function(t) {
+    var testBody = function(worker) {
+      return with_iframe('./resources/after-event-dispatch-respondwith-microtask-sync-waituntil');
+    }
+    return runTest(t, 'after-event-dispatch-respondwith-microtask-sync-waituntil', testBody);
+  }, 'Test calling waitUntil synchronously inside microtask of respondWith promise (event is not being dispatched).');

+promise_test(function(t) {
+    var testBody = function(worker) {
+      return with_iframe('./resources/after-event-dispatch-respondwith-microtask-async-waituntil');
+    }
+    return runTest(t, 'after-event-dispatch-respondwith-microtask-async-waituntil', testBody);
+  }, 'Test calling waitUntil asynchronously inside microtask of respondWith promise (event is not being dispatched).');
 </script>
--- a/testing/web-platform/tests/service-workers/service-worker/fetch-event-async-respond-with.https.html
+++ b/testing/web-platform/tests/service-workers/service-worker/fetch-event-async-respond-with.https.html
@ -1,36 +1,61 @@
 <!DOCTYPE html>
+<html>
+<title>respondWith cannot be called asynchronously</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="resources/test-helpers.sub.js"></script>
 <script>
-promise_test(function(t) {
-    var script = 'resources/fetch-event-async-respond-with-worker.js';
-    var scope = 'resources/simple.html';
+// This file has tests that call respondWith() asynchronously.

-    return service_worker_unregister_and_register(t, script, scope)
-      .then(function(registration) {
-          t.add_cleanup(function() {
-              return service_worker_unregister(t, scope);
+let frame;
+let worker;
+const script = 'resources/fetch-event-async-respond-with-worker.js';
+const scope = 'resources/simple.html';
+
+// Global setup: this must be the first promise_test.
+promise_test(async (t) => {
+  const registration =
+      await service_worker_unregister_and_register(t, script, scope);
+  worker = registration.installing;
+  await wait_for_state(t, worker, 'activated');
+  frame = await with_iframe(scope);
+}, 'global setup');
+
+// Does one test case. It fetches |url|. The service worker gets a fetch event
+// for |url| and attempts to call respondWith() asynchronously. It reports back
+// to the test whether an exception was thrown.
+async function do_test(url) {
+  // Send a message to tell the worker a new test case is starting.
+  const sawMessage = new Promise(resolve => {
+    navigator.serviceWorker.onmessage = (event) => {
+      resolve(event.data);
+    };
+    worker.postMessage('');
  });

-          return wait_for_state(t, registration.installing, 'activated');
-        })
-      .then(function() {
-          return with_iframe(scope);
-        })
-      .then(function(frame) {
-          add_completion_callback(function() { frame.remove(); });
-          var channel = new MessageChannel();
-          var saw_message = new Promise(function(resolve) {
-              channel.port1.onmessage = function(e) { resolve(e.data); }
-            });
-          var worker = frame.contentWindow.navigator.serviceWorker.controller;
+  // Start a fetch.
+  frame.contentWindow.fetch(url);

-          worker.postMessage({port: channel.port2}, [channel.port2]);
-          return saw_message;
-        })
-      .then(function(message) {
-          assert_equals(message, 'PASS');
-        })
-  }, 'Calling respondWith asynchronously throws an exception');
+  // Receive the test result from the service worker.
+  return await sawMessage;
+};
+
+promise_test(async (t) => {
+  const result = await do_test('respondWith-in-task');
+  assert_true(result.didThrow, 'should throw');
+  assert_equals(result.error, 'InvalidStateError');
+}, 'respondWith in a task throws InvalidStateError');
+
+promise_test(async (t) => {
+  const result = await do_test('respondWith-in-microtask');
+  assert_equals(result.didThrow, false, 'should not throw');
+}, 'respondWith in a microtask does not throw');
+
+// Global cleanup: the final promise_test.
+promise_test(async (t) => {
+  if (frame)
+    frame.remove();
+  await service_worker_unregister(t, scope);
+}, 'global cleanup');
 </script>
+</html>
--- a/testing/web-platform/tests/service-workers/service-worker/resources/extendable-event-async-waituntil.js
+++ b/testing/web-platform/tests/service-workers/service-worker/resources/extendable-event-async-waituntil.js
@ -1,4 +1,12 @@
-// controlled by 'init'/'done' messages.
+// This worker calls waitUntil() and respondWith() asynchronously and
+// reports back to the test whether they threw.
+//
+// These test cases are confusing. Bear in mind that the event is active
+// (calling waitUntil() is allowed) if:
+// * The pending promise count is not 0, or
+// * The event dispatch flag is set.
+
+// Controlled by 'init'/'done' messages.
 var resolveLockPromise;
 var port;

@ -14,34 +22,72 @@ self.addEventListener('message', function(event) {
      case 'done':
        resolveLockPromise();
        break;
+
+      // Throws because waitUntil() is called in a task after event dispatch
+      // finishes.
      case 'no-current-extension-different-task':
        async_task_waituntil(event).then(reportResultExpecting('InvalidStateError'));
        break;
+
+      // OK because waitUntil() is called in a microtask that runs after the
+      // event handler runs, while the event dispatch flag is still set.
      case 'no-current-extension-different-microtask':
-        async_microtask_waituntil(event).then(reportResultExpecting('InvalidStateError'));
+        async_microtask_waituntil(event).then(reportResultExpecting('OK'));
        break;
+
+      // OK because the second waitUntil() is called while the first waitUntil()
+      // promise is still pending.
      case 'current-extension-different-task':
        event.waitUntil(new Promise((res) => { resolveTestPromise = res; }));
        async_task_waituntil(event).then(reportResultExpecting('OK')).then(resolveTestPromise);
        break;
-      case 'current-extension-expired-same-microtask-turn':
+
+      // OK because all promises involved resolve "immediately", so the second
+      // waitUntil() is called during the microtask checkpoint at the end of
+      // event dispatching, when the event dispatch flag is still set.
+      case 'during-event-dispatch-current-extension-expired-same-microtask-turn':
        waitPromise = Promise.resolve();
        event.waitUntil(waitPromise);
        waitPromise.then(() => { return sync_waituntil(event); })
          .then(reportResultExpecting('OK'))
        break;
-      case 'current-extension-expired-same-microtask-turn-extra':
-        // The promise handler queues a new microtask *after* the check for new
-        // extensions was performed.
+
+      // OK for the same reason as above.
+      case 'during-event-dispatch-current-extension-expired-same-microtask-turn-extra':
        waitPromise = Promise.resolve();
        event.waitUntil(waitPromise);
+        waitPromise.then(() => { return async_microtask_waituntil(event); })
+          .then(reportResultExpecting('OK'))
+        break;
+
+
+      // OK because the pending promise count is decremented in a microtask
+      // queued upon fulfillment of the first waitUntil() promise, so the second
+      // waitUntil() is called while the pending promise count is still
+      // positive.
+      case 'after-event-dispatch-current-extension-expired-same-microtask-turn':
+        waitPromise = makeNewTaskPromise();
+        event.waitUntil(waitPromise);
+        waitPromise.then(() => { return sync_waituntil(event); })
+          .then(reportResultExpecting('OK'))
+        break;
+
+      // Throws because the second waitUntil() is called after the pending
+      // promise count was decremented to 0.
+      case 'after-event-dispatch-current-extension-expired-same-microtask-turn-extra':
+        waitPromise = makeNewTaskPromise();
+        event.waitUntil(waitPromise);
        waitPromise.then(() => { return async_microtask_waituntil(event); })
          .then(reportResultExpecting('InvalidStateError'))
        break;
+
+      // Throws because the second waitUntil() is called in a new task, after
+      // first waitUntil() promise settled and the event dispatch flag is unset.
      case 'current-extension-expired-different-task':
        event.waitUntil(Promise.resolve());
        async_task_waituntil(event).then(reportResultExpecting('InvalidStateError'));
        break;
+
      case 'script-extendable-event':
        self.dispatchEvent(new ExtendableEvent('nontrustedevent'));
        break;
@ -51,23 +97,60 @@ self.addEventListener('message', function(event) {
  });

 self.addEventListener('fetch', function(event) {
-    if (event.request.url.indexOf('pending-respondwith-async-waituntil') != -1) {
+  const path = new URL(event.request.url).pathname;
+  const step = path.substring(path.lastIndexOf('/') + 1);
+  let response;
+  switch (step) {
+    // OK because waitUntil() is called while the respondWith() promise is still
+    // unsettled, so the pending promise count is positive.
+    case 'pending-respondwith-async-waituntil':
      var resolveFetch;
-      let response = new Promise((res) => { resolveFetch = res; });
+      response = new Promise((res) => { resolveFetch = res; });
      event.respondWith(response);
      async_task_waituntil(event)
        .then(reportResultExpecting('OK'))
        .then(() => { resolveFetch(new Response('OK')); });
-    } else if (event.request.url.indexOf('respondwith-microtask-sync-waituntil') != -1) {
+      break;
+
+    // OK because all promises involved resolve "immediately", so waitUntil() is
+    // called during the microtask checkpoint at the end of event dispatching,
+    // when the event dispatch flag is still set.
+    case 'during-event-dispatch-respondwith-microtask-sync-waituntil':
      response = Promise.resolve(new Response('RESP'));
      event.respondWith(response);
      response.then(() => { return sync_waituntil(event); })
-        .then(reportResultExpecting('OK'))
-    } else if (event.request.url.indexOf('respondwith-microtask-async-waituntil') != -1) {
+        .then(reportResultExpecting('OK'));
+      break;
+
+    // OK because all promises involved resolve "immediately", so waitUntil() is
+    // called during the microtask checkpoint at the end of event dispatching,
+    // when the event dispatch flag is still set.
+    case 'during-event-dispatch-respondwith-microtask-async-waituntil':
      response = Promise.resolve(new Response('RESP'));
      event.respondWith(response);
+      response.then(() => { return async_microtask_waituntil(event); })
+        .then(reportResultExpecting('OK'));
+      break;
+
+    // OK because the pending promise count is decremented in a microtask queued
+    // upon fulfillment of the respondWith() promise, so waitUntil() is called
+    // while the pending promise count is still positive.
+    case 'after-event-dispatch-respondwith-microtask-sync-waituntil':
+      response = makeNewTaskPromise().then(() => {return new Response('RESP');});
+      event.respondWith(response);
+      response.then(() => { return sync_waituntil(event); })
+        .then(reportResultExpecting('OK'));
+      break;
+
+
+    // Throws because waitUntil() is called after the pending promise count was
+    // decremented to 0.
+    case 'after-event-dispatch-respondwith-microtask-async-waituntil':
+      response = makeNewTaskPromise().then(() => {return new Response('RESP');});
+      event.respondWith(response);
      response.then(() => { return async_microtask_waituntil(event); })
        .then(reportResultExpecting('InvalidStateError'))
+      break;
  }
 });

@ -118,3 +201,10 @@ function async_task_waituntil(event) {
    }, 0);
  });
 }
+
+// Returns a promise that settles in a separate task.
+function makeNewTaskPromise() {
+  return new Promise(resolve => {
+    setTimeout(resolve, 0);
+  });
+}
--- a/testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-async-respond-with-worker.js
+++ b/testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-async-respond-with-worker.js
@ -1,19 +1,56 @@
-var result;
+// This worker attempts to call respondWith() asynchronously after the
+// fetch event handler finished. It reports back to the test whether
+// an exception was thrown.

-self.addEventListener('message', function(event) {
-    event.data.port.postMessage(result);
+// These get reset at the start of a test case.
+let reportResult;
+let resultPromise;
+
+// The test page sends a message to tell us that a new test case is starting.
+// We expect a fetch event after this.
+self.addEventListener('message', (event) => {
+  resultPromise = new Promise((resolve) => {
+    reportResult = resolve;
  });

-self.addEventListener('fetch', function(event) {
-    setTimeout(function() {
+  // Keep the worker alive until the test case finishes, and report
+  // back the result to the test page.
+  event.waitUntil(resultPromise.then(result => {
+    event.source.postMessage(result);
+  }));
+});
+
+// Calls respondWith() and reports back whether an exception occurred.
+function tryRespondWith(event) {
  try {
    event.respondWith(new Response());
-          result = 'FAIL: did not throw';
+    reportResult({didThrow: false});
  } catch (error) {
-          if (error.name == 'InvalidStateError')
-            result = 'PASS';
-          else
-            result = 'FAIL: Unexpected exception: ' + error;
+    reportResult({didThrow: true, error: error.name});
  }
+}
+
+function respondWithInTask(event) {
+  setTimeout(() => {
+    tryRespondWith(event);
  }, 0);
+}
+
+function respondWithInMicrotask(event) {
+  Promise.resolve().then(() => {
+    tryRespondWith(event);
+  });
+}
+
+self.addEventListener('fetch', function(event) {
+  const path = new URL(event.request.url).pathname;
+  const test = path.substring(path.lastIndexOf('/') + 1);
+
+  // If this is a test case, try respondWith() and report back to the test page
+  // the result.
+  if (test == 'respondWith-in-task') {
+    respondWithInTask(event);
+  } else if (test == 'respondWith-in-microtask') {
+    respondWithInMicrotask(event);
+  }
 });