New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
service worker: Improve WPT tests for async respondWith/waitUntil. #15852
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already reviewed downstream.
2347b25
to
1116df4
Compare
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] w3c/ServiceWorker#1213 [2] w3c/ServiceWorker#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}
1116df4
to
1647da2
Compare
Travis CI is failing because the fetch-event-async-respond-with flakily crashes on Chrome which is expected. The Chromium CL included put Skip expectations in TestExpectations so Chrome doesn't run the test. The real fix to Chrome is in in the next CL. |
The CL to fix crashing Chrome landed at https://chromium-review.googlesource.com/c/chromium/src/+/1523207 |
Thanks for checking and explaining the failure, @mattto! I'll go ahead and admin merge this. |
Thanks @foolip ! |
…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] w3c/ServiceWorker#1213 [2] w3c/ServiceWorker#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}
See discussion at [1] and [2].
This makes the following changes.
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
Changes the expectation for:
addEventListener('message', event => {
Promise.resolve().then(event.waitUntil(p));
});
From throws to not throws, for the same reasoning as above.
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.
To test what 3. originally intended, a new test is
added which makes waitPromise a promise that does not immediately
resolve.
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] w3c/ServiceWorker#1213
[2] w3c/ServiceWorker#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}