Nerfing all events on all event targets in a serviceworker after initial script evaluation seems odd

[bzbarsky]

As of today, https://dom.spec.whatwg.org/commit-snapshots/8eb68e48b3f613663425120edc5039d83b3a9de8/#dom-eventtarget-addeventlistener says:

If context object’s relevant global object is a ServiceWorkerGlobalScope object and its associated service worker’s script resource’s has ever been evaluated flag is set, then throw a TypeError. [SERVICE-WORKERS]

Why are we doing this for all event targets? Why are we even doing it for all events? This makes no sense.

[bzbarsky]

Looks like this was introduced in #155

// cc @jungkees

[bzbarsky]

I mean, there's the question of why we're doing it at all too, instead of handling this like upload events on XHR.

[jungkees]

there's the question of why we're doing it at all too

It had started by w3c/ServiceWorker#718 (comment) to deterministically detect whether no event handler's registered for a given event (to reduce unnecessary SW start-up delay in such cases.) Over a discussion, we'd setteled on the current behavior. (TL;DR see w3c/ServiceWorker#718 (comment) and below.)

Why are we doing this for all event targets?

I think this was a miss. This should be only for the events on ServiceWorkerGlobalScope not for all the EventTargets.

Why are we even doing it for all events?

Given that it should be only for ServiceWorkerGlobalScope, SW lifecycle events plus all other functional events including fetch, push and future extesions are within the scope of this behavior.

handling this like upload events on XHR.

Could you give me a pointer or explain a bit more on this?

[annevk]

Upload events are based on a flag whose state is determined once fetching starts. I think service workers has that already in place. We check if there are any event listeners registered and store those someplace.

[bzbarsky]

Over a discussion, we'd setteled on the current behavior

I don't really see a discussion. I see @jakearchibald asserting that this should be the behavior and no one questioning it, or thinking about how it ties in to other specifications. For example, it introduced inconsistent behavior between addEventListener("fetch", ...) and setting onfetch...

SW lifecycle events plus all other functional events including fetch, push and future extesions are within the scope of this behavior

OK, but you're doing this for all events, including random made-up ones the service worker script itself decides to fire. I don't see why this is at all reasonable.

[jungkees]

I don't really see a discussion.

Basically this: w3c/ServiceWorker#718. The goal was to deterministically detect if a SW opted in to certain functional events. We didn't want to override addEventListener for this, was trying to introduce an API to disable fetch, etc. But finally determined to store the set of event types the SW script deterministically declares during the initial execution - only possible by a successful update through the first run in Update algorithm step 9.6. And we added the condition to addEventListener() that checks has ever been evaluated flag. We need to fix this to check it only on ServiecWorkerGlobalScope as mentioned.

it introduced inconsistent behavior between addEventListener("fetch", ...) and setting onfetch...

This is w3c/ServiceWorker#1004. onfetch needs the same behavior.

OK, but you're doing this for all events, including random made-up ones the service worker script itself decides to fire. I don't see why this is at all reasonable.

Synthetic events do not spin off SW. Note that only SW lifecycle events and functional events run SW. fetch event dispatch is defined by SW's Handle Fetch and other functional events should extend SW spec and dispatch events by invoking Handle Functional Event. (Also see https://w3c.github.io/ServiceWorker/#extensibility.)

[annevk]

@jungkees in theory you can do self.dispatchEvent(new Event("blah")) though, except now that would only work during the initial run, which is a little limiting.

[bzbarsky]

Basically this: w3c/ServiceWorker#718.

That has precisely 0 discussion about the throwing behavior.

And we added the condition to addEventListener() that checks has ever been evaluated flag.

Right, but why? This isn't how the precedent in the platform for this (XHR upload events) works....

This is w3c/ServiceWorker#1004.

Yes, I'm well aware. My discussion about this stuff with @catalinb is what led to this issue being filed.

For what it's worth, I expect that the behavior for onfetch will be crazy no matter what. For example, say the worker script does:

self.onfetch = ()=>{};
self.onfetch = null;

during initial script execution and then after initial execution does:

self.onfetch = ()=>{};

Should that last set throw? Why would it? It doesn't add an event listener, note.

Synthetic events do not spin off SW.

I have no idea what "spin off" means here.

Note that only SW lifecycle events and functional events run SW.

Again, no idea what "run" means here.

As @annevk notes, scripts can create and dispatch arbitrary events at a ServiceWorkerGlobalScope. And I mean arbitrary. With arbitrary data on them, for their own use. CustomEvent is exposed in SW. I don't see why we should prevent scripts from doing this. And we shouldn't prevent them.

[wanderview]

I thought we were only going to do this throwing behavior for fetch event. Why do we need to do it for any other event? Everything else has an API to explicitly opt in to the event.

[jungkees]

I have no idea what "spin off" means here. Again, no idea what "run" means here.

That is Run Service Worker the only call sites of which are SW Update/Install/Active, Handle Fetch, Handle Functional Event, and postMessage to SW.

This isn't how the precedent in the platform for this (XHR upload events) works...

As @annevk noted, SW already does this (but with the has ever been evaluated flag constraint): Run Service Worker step 15.

That has precisely 0 discussion about the throwing behavior.

So this kind of usages is totally feasible

if (Math.random() > .9) { xhr.upload.onloadstart = handlerFunc; }

xhr.send(body);

under the uncertainty whether the upload events will be fired.

But in the case of SW to meet the original goal of optimizing the behavior (i.e., not starting the SW in the first place), the constraint was needed.

Should that last set throw? Why would it? It doesn't add an event listener, note.

Yes, it throws in the current behavior. It's a matter of whether we'd consider this as a valid constraint for the optimization for SW or not.

I thought we were only going to do this throwing behavior for fetch event. Why do we need to do it for any other event? Everything else has an API to explicitly opt in to the event.

I think it was discussed in the f2f lead to this comment: w3c/ServiceWorker#718 (comment). This applies the benefit of not starting the SW that has no event listener for the event regardless of whether the app opted in for the event.

@jungkees in theory you can do self.dispatchEvent(new Event("blah")) though, except now that would only work during the initial run, which is a little limiting.

Right. I'm not sure if there's a reasonable way to not throw in the case of CustomEvents. If so, that seems to be an option here.

[bzbarsky]

That is Run Service Worker the only call sites of which are SW Update/Install/Active, Handle Fetch, Handle Functional Event, and postMessage to SW.

OK, so Run Service Worker doesn't fire random events other than some small whitelist? Fine, but so what? ;)

So this kind of usages is totally feasible

Yes. The event will be fired if there were upload listeners set before send().

the constraint was needed

I don't see why. Your XHR example would work exactly the same whether XHR had this constraint or not, note...

Yes, it throws in the current behavior.

I don't know what you meant by "current behavior", but it doesn't throw in Gecko, and it doesn't throw per the current spec. Where does it throw? What causes it to throw there?

I'm not sure if there's a reasonable way to not throw in the case of CustomEvents.

Well, you could start by only throwing, if you're going to throw at all, for a specific whitelist of event names that are reserved by the SW spec for its own purposes, instead of blanket-throwing on everything in sight.

[jungkees]

I don't see why. Your XHR example would work exactly the same whether XHR had this constraint or not, note...

You're right. I picked up a wrong example indeed. Proper examples would be cases where the event listers are set up within other event handlers or other asyc callbacks. The point is, considering SW's lifetime, setting up event handlers in async tasks wouldn't guarantee they will be run.

I don't know what you meant by "current behavior"

Sorry for the confusion. I meant the behavior that addEventListener throws.

Well, you could start by only throwing, if you're going to throw at all, for a specific whitelist of event names that are reserved by the SW spec for its own purposes, instead of blanket-throwing on everything in sight.

That makes sense. For the exact spec language, I'd want to improve the definition of functional events so it can be used to reference the SW events with future extensions inclusive.

[annevk]

@jungkees should we really use the precedent of the fetch event for new events? I think it would be better for new things to be opt-in. It also sounds like nobody really knows the rationale for the current setup and nobody implements it, so maybe we should just revisit it and only have the "has ever been evaluated flag" and don't modify addEventListener() (or make it only throw when you pass in fetch, but that's still a little weird as it would ban synthetic events with that name).

[jungkees]

@annevk, other events are already opt-in. As answered to @wanderview's question, I thought checking the "set of event types to handle" in Handle Functional Event step 4 is generally beneficial by avoiding SW start-up delay for scripts that don't (deterministically) register any listener for the event.

I'm not against revisiting the design. Indeed we (SW folks including you and me) had discussed this and settled on the current setup (I admit I missed things when having spec'd them as pointed in this issue) together, so I'd like to hear more comments from the folks before we move on. /cc @slightlyoff, @jakearchibald, @wanderview, @aliams @hober

FYI, here's the minute of the f2f where we discussed this issue: https://lists.w3.org/Archives/Public/public-webapps/2016JanMar/0096.html.

#718: Proposal: Optimized No-Fetch Service Workers
     * Resolved: check which event listeners have been registered via addEventListener and optimize based on that - additionally, addEventListener should throw an error if it’s called after the initial execution of the script (e.g. in an event handler) 

[jungkees]

Note: the problem posed in the OP of w3c/ServiceWorker#718 (comment) is also being addressed by w3c/ServiceWorker#920 effort. We might want to consider that point too.

[jakearchibald]

There are two intents here:

• Avoid executing the service worker to fire an event that isn't being listened to
• Prevent the user from creating an event listener that's likely to be lost before it's called

Eg

self.addEventListener('install', () => {
  self.addEventListener('activate', () => {
    console.log('hello');
  });
});

The above is dangerous, as the listener to "activate" will be lost if the service worker closes between dispatching "install" and "activate".

I agree that we shouldn't be doing this for all targets as @bzbarsky says. For instance we want devs to be able to create a worker during a fetch event, and this rule prevents adding the message handler.

For addEventListener on ServiceWorkerGlobalScope I'm struggling to see why throwing isn't the correct thing to do. Sure, it prevents you from lazy-adding a custom listener for manually-dispatched custom events you know will run before service worker termination, but the answer there is just to add the listener in the initial execution right?

[bzbarsky]

The listener you add may depend on the result of information you get asynchronously (e.g. via fetch()), no?

[jakearchibald]

@bzbarsky if you set a global listener from the result of something async, it'll be lost when the service worker next closes. I can't think of cases where you'd want to add a global listener async.

[bzbarsky]

it'll be lost when the service worker next closes.

Sure, but when it starts up again you'd set it up again, no? I mean, how is this different from setting up a listener directly in the initial script the service worker runs? It's just a difference between a function call and an async function.

[annevk]

@bzbarsky but the listener would never be invoked, since if it's not added before installation finishes, it won't be a reason to start the service worker.

[bzbarsky]

I'm talking about the situation where the worker installs a fetch listener and also installs a listener for myFunkyEventIFireMyself, and does the latter async.

[annevk]

@jakearchibald @jungkees @wanderview did this behavior get implemented?

I think I'm okay with what @bzbarsky argues for. That we investigate which listeners were added at the end of some task. We record their event names as being able to start the service worker and we leave everything else as-is. So you can continue to use custom events, including custom events named fetch, without getting exceptions.

I think we should probably also only do this for some of the existing event names, such as fetch, and require explicit opt-in going forward.

Thoughts?

[jakearchibald]

self.addEventListener('push', e => {
  self.onfetch = e => {
    // A push event will add this fetch listener.
  };
});

If we consider the above service worker "has a fetch listener", this would happen:

1. Controlled document makes a fetch.
2. We start up the service worker, because it has a fetch listener.
3. We despatch the 'fetch' event.
4. There is no listener.

[jungkees]

Oh.. right. I had lost the context indeed. It gets us go back to the question 2 and 3 of #371 (comment).

[jungkees]

2. Can we remove the exception at this point.
3. Should we limit this practice somehow. E.g., require an opt-in other than an event listener for new "functional events".

I've thought about this. I don't think we can avoid throwing here. Functional events themselves are the very starting point of the execution of service workers, and the existence of the event listeners being the criterion for the optimization sounds reasonable to me. I think we should do this for all the functional events. For custom events, they can't start off a service worker in the first place, but we can just let them work as usual within the lifetime of a service worker.

We'd still have to change it to allow only ServiceWorkerGlobalScope as the event target though (instead of all the event target objects in the global).

[annevk]

Why can't we avoid throwing?

[jungkees]

Given that we didn't want to make the fetch opt out, I can't think of any better design choice than relying on the event listeners set during the initial execution.

[annevk]

Sure, but why does that mean we need to throw if you add another listener later?

[jakearchibald]

@annevk what's wrong with throwing in this case?

The problem with the current spec is it throws if the global is ServiceWorkerGlobal, whereas it should only throw if the context is ServiceWorkerGlobal.

If throwing is bad, we should definitely display a console warning.

[jungkees]

Adding another listener during the initial execution won't throw. The point is we are closing the door at the end of the top-level evaluation to not allow any indeterministic additions.

[annevk]

Throwing is bad because it breaks the expectations of the event model, as bz argued early on in this thread. It's unclear that we should forbid someone adding a listener for their own synthetic "push" event at any point within the lifetime of the global.

[jakearchibald]

If that's the sticking point then I'm happy with us switching to a console warning for listeners attached to the global that service worker uses as lifecycle or functional events.

When it comes to functional events, we shouldn't spin up the service worker if it isn't in the set of event types to handle.

[annevk]

Okay, great. @bzbarsky I assume that works for you too?

I guess what remains here are tests. But this means that in the DOM, we'll remove these lines:

If eventTarget’s relevant global object is a ServiceWorkerGlobalScope object and its associated service worker’s script resource’s has ever been evaluated flag is set, then throw a TypeError.

If the context object’s relevant global object is a ServiceWorkerGlobalScope object and its associated service worker’s script resource’s has ever been evaluated flag is set, then throw a TypeError.

or alternatively rephrase them with the suggestion to say something in the error console (and also only do it if the context object is the global object). Unfortunately whatwg/console#57 is still open.

[bzbarsky]

I'm not clear on what exactly is being proposed, so I'm not sure whether it works for me.

Is the proposal that we take out all special handling of events on ServiceWorkerGlobalScope apart from possibly having some console warnings? How does that handle the original motivating use cases for this behavior?

[annevk]

@bzbarsky basically web developers will only spot potential mistakes in the console. The service workers specification will still have special handling in that it observes listeners at a specific point in time. At some point I'll add a "legacy has listener" specification entry point for that, to make it clear that's a thing that exists (and we don't quite like).

[bzbarsky]

That works for me in terms of the core event processing bits, sure.

[jungkees]

or alternatively rephrase them with the suggestion to say something in the error console (and also only do it if the context object is the global object). Unfortunately whatwg/console#57 is still open.

@annevk, stating that in the spec would help. And a few more details about SW behavior:

When it comes to functional events, we shouldn't spin up the service worker if it isn't in the set of event types to handle.

To make it clear, what do we want to do with the lifecycle events and the message event? I'm not thinking of adding the lifecycle events (install and activate) to this rule as Install and Activate steps change the service worker's state regardless of the existence of the event listeners.

If that's the sticking point then I'm happy with us switching to a console warning for listeners attached to the global that service worker uses as lifecycle or functional events.

We may want to do this only for functional events I guess as Install and Activate are not checking the set of event types to handle.

[annevk]

To make it clear, what do we want to do with the lifecycle events and the message event?

I think it would be okay to check for message and the lifecycle events as part of the overall legacy. The main thing I care about is that new features don't end up relying on it.

[annevk]

If we want to log something in the console, we should probably only do it if event's type is one of "fetch", "message", ... right? What's the complete list there? (And even that can be a little sketchy, as it might be a legitimate late addition due to synthetic events, but I guess in the majority of cases it'll be a legitimate problem.)

[jungkees]

If we want to log something in the console, we should probably only do it if event's type is one of "fetch", "message", ... right? What's the complete list there?

I think we can use the definition of the functional event. And I can probably add a reference to the complete list of functional events from there. The service worker API page of MDN seems to be a good place we can maintain the list. What do you think?

(And even that can be a little sketchy, as it might be a legitimate late addition due to synthetic events, but I guess in the majority of cases it'll be a legitimate problem.)

So, I think the warning message should point out the fact that the late addition for functional events doesn't run the service worker even though it was a legitimate call.

[annevk]

MDN is not a good normative reference. If you want to maintain a registry, inline it. What about the message event? Is that a functional event?

[jungkees]

Inlining it sounds good to me.

I don't think the message event is a functional event. The message event doesn't check the set of event types to handle either.