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

clarify that manually constructed ExtendableEvent objects may not extend lifetime via waitUntil() #1040

Closed
wanderview opened this issue Dec 22, 2016 · 10 comments

Comments

@wanderview
Copy link
Member

Currently its possible to do this in the spec:

let e = new ExtendableEvent('foo');
e.waitUntil(new Promise());

By default the extensions allowed flag is set, so this does not throw an InvalidStateError.

We should be clear, though, that this does not actually extend the lifetime of the SW. Currently the spec is written in a way that suggests that this might imbue some keep-alive budget to the SW.

@wanderview wanderview added this to the Version 1 milestone Dec 22, 2016
@wanderview
Copy link
Member Author

I would also be happy to make manually constructed ExtendableEvent objects unset extensions allowed flag.

@wanderview
Copy link
Member Author

I think we also used to have something that prevented waitUntil() outside of a dispatch, but I don't see that any more. Thats a minor issue, though, since the the script can call self.dispatchEvent().

@wanderview
Copy link
Member Author

FWIW, we are going to throw InvalidStateError in this case. Its easier to open this up to not throw if we need that behavior for some reason.

@stefhak
Copy link

stefhak commented Dec 23, 2016

To put this in context, I do, to emulate background sync on Firefox, periodically check if navigator.onLine is true, and when it is I fire an extendable event:

let e = new ExtendableEvent('sync');
e.tag = 'some_tag';
if (navigator.onLine)
    self.dispatchEvent(e);

My event handler looks like:

self.addEventListener('sync', (evt) => {
    if (evt.tag == 'some_tag') {
        evt.waitUntil(doStuff());
    }
});

where evt.waitUntil is taken from some Background sync tutorial. I understand it would not have any effect with my emulated background sync, but will it with the real background sync? Is that specified anywhere?

@jakearchibald
Copy link
Contributor

I think #980 (comment) would fix this too.

@jakearchibald
Copy link
Contributor

To discuss: Should we spec #980 (comment), and is this v1?

@wanderview
Copy link
Member Author

Even if we spec the solution in #980, I don't think it should apply to manually constructed events.

@jakearchibald
Copy link
Contributor

jakearchibald commented Apr 4, 2017

F2F: When we spec #980, the example in the OP shouldn't allow waitUntil, as if promises have been previously passed & all settled.

@jungkees
Copy link
Collaborator

jungkees commented Apr 4, 2017

I think we may want to use event.isTrusted to check if the event is a synthetic object instead of checking pending promises count here. @annevk, can we ensure with .isTrusted == false if an event is a synthetic event?

@annevk
Copy link
Member

annevk commented Apr 4, 2017

What is mentioned in OP should already throw since the dispatch flag is not set.

But yeah, it seems reasonable to also check the isTrusted attribute (internal slot at some point). Not entirely convinced that's the best way, but that should work.

jungkees added a commit that referenced this issue Jul 11, 2017
This change adds a guard to waitUntil() that disallows the extension of
the event that is not dispatched by the user agent.

Fixes #1040.
jungkees added a commit to jungkees/web-platform-tests that referenced this issue Jul 11, 2017
This moves the state check point of the script created event from the
outside into the event dispatch task. Before this change, it could be
possible to pass the test without checking the isTrusted state when
"the pending promises count is zero and the dispatch flag is unset"
condition was met.

Spec issue: w3c/ServiceWorker#1040.
Spec PR: w3c/ServiceWorker#1166.
jakearchibald pushed a commit to web-platform-tests/wpt that referenced this issue Sep 1, 2017
…sk (#6517)

* Check the state of the script created event inside of the dispatch task

This moves the state check point of the script created event from the
outside into the event dispatch task. Before this change, it could be
possible to pass the test without checking the isTrusted state when
"the pending promises count is zero and the dispatch flag is unset"
condition was met.

Spec issue: w3c/ServiceWorker#1040.
Spec PR: w3c/ServiceWorker#1166.

* Dispatch event synchronously and reuse sync_waituntil

* Change for a retry after 502 server error
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this issue Nov 8, 2017
…sk (web-platform-tests#6517)

* Check the state of the script created event inside of the dispatch task

This moves the state check point of the script created event from the
outside into the event dispatch task. Before this change, it could be
possible to pass the test without checking the isTrusted state when
"the pending promises count is zero and the dispatch flag is unset"
condition was met.

Spec issue: w3c/ServiceWorker#1040.
Spec PR: w3c/ServiceWorker#1166.

* Dispatch event synchronously and reuse sync_waituntil

* Change for a retry after 502 server error
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this issue Nov 16, 2017
…sk (web-platform-tests#6517)

* Check the state of the script created event inside of the dispatch task

This moves the state check point of the script created event from the
outside into the event dispatch task. Before this change, it could be
possible to pass the test without checking the isTrusted state when
"the pending promises count is zero and the dispatch flag is unset"
condition was met.

Spec issue: w3c/ServiceWorker#1040.
Spec PR: w3c/ServiceWorker#1166.

* Dispatch event synchronously and reuse sync_waituntil

* Change for a retry after 502 server error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants