Navigation Menu

Skip to content
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

Merged
merged 1 commit into from Mar 18, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Mar 15, 2019

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}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1524393 branch 4 times, most recently from 2347b25 to 1116df4 Compare March 18, 2019 01:55
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}
@mfalken
Copy link
Member

mfalken commented Mar 18, 2019

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.

@mfalken
Copy link
Member

mfalken commented Mar 18, 2019

The CL to fix crashing Chrome landed at https://chromium-review.googlesource.com/c/chromium/src/+/1523207

@foolip
Copy link
Member

foolip commented Mar 18, 2019

Thanks for checking and explaining the failure, @mattto! I'll go ahead and admin merge this.

@foolip foolip merged commit cecb3eb into master Mar 18, 2019
@foolip foolip deleted the chromium-export-cl-1524393 branch March 18, 2019 15:30
@mfalken
Copy link
Member

mfalken commented Mar 19, 2019

Thanks @foolip !

marcoscaceres pushed a commit that referenced this pull request Jul 23, 2019
…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}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants