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

Only specific APIs should skip the fetch event when called within a service worker #303

Closed
mvano opened this Issue May 12, 2016 · 79 comments

Comments

9 participants
@mvano
Copy link

mvano commented May 12, 2016

Currently it is not clear whether resource fetches for notifications (e.g. icons) shown from a service worker should trigger the service worker onfetch handler. As a use case it makes sense to enable offlining these resources.

My understanding is that fetches initiated directly from a service worker are not passed through the service worker onfetch handler, which seems reasonable. However, what about the fetches triggered indirectly by actions initiated in the service worker, like showing a notification?

CC @annevk @jakearchibald @beverloo @slightlyoff

@beverloo

This comment has been minimized.

Copy link
Member

beverloo commented May 12, 2016

+1, notification resources should be cacheable by a Service Worker regardless where a notification was shown from. Today this is a failure case for people using push on flaky connections, in a future having a job scheduler/timers there is no guarantee that the device will be online at all when a notification is to be shown.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 12, 2016

How many APIs in a service worker can cause a fetch?

  1. fetch() (we cannot let that be captured by the same service worker)
  2. openWindow() (is captured, though not necessarily by the same service worker, depends on the URL)
  3. showNotification() (currently not captured, despite being able to fetch multiple resources that should probably work offline)
  4. ?
@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 22, 2016

@jungkees @wanderview @jakearchibald you might want to discuss this at the SW F2F?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 22, 2016

@mvano it might be better to file this at https://github.com/slightlyoff/ServiceWorker/issues/new perhaps? Or at least track it in both places.

@annevk annevk added the needsinfo label Jul 22, 2016

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 22, 2016

I don't see anything in the notifications API about setting the skip-service-worker flag:

https://notifications.spec.whatwg.org/#fetch-steps

So I think per spec this should be intercepted. This isn't all that different from Client.navigate() which might also trigger an onFetch.

So I think it should be intercepted. I'm not sure what behavior is currently implemented, though. It would be nice to have a test case.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 22, 2016

Actually, I take that back. Something like foreign fetch would allow these to be intercepted. Currently I don't think the notification image is loaded in a controlled Client, so its not intercepted.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 22, 2016

An alternative here would be to let the Notification API accept Response objects instead of URLs. Then the SW push event handler could specify resources it already has offlined without a separate fetch event.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 22, 2016

Foreign fetch only works across origins. Same-origin is not intercepted since the request client is the service worker itself.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 22, 2016

@annevk What is the "origin" of the notification popup? Is it even defined as a window or Client? AFAICT these requests made from within the browser without a window defining a source origin. No?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 22, 2016

The origin of a notification is the origin of the global it was created in. But I was talking about the notification's resources, not the notification itself.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 22, 2016

@annevk The spec seems a bit vague right now. Notification API says to "fetch notification’s icon URL". It links, however, to a spec text in fetch that depends on having a request:

https://fetch.spec.whatwg.org/#concept-fetch

How does the conversion from URL to request occur here?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 22, 2016

@wanderview that seems like something that should be fixed, but in principle a notification is no different from <img>.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 22, 2016

Well, it matters when I'm trying to figure out what the Client should be to determine if its controlled or not.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 22, 2016

When created in a service worker the client can be nothing but that service worker.

@jungkees

This comment has been minimized.

Copy link
Contributor

jungkees commented Jul 25, 2016

The use case sounds reasonable here, and I'd expect there would be more such needs in future.

Basically, I don't think we want to change the base rule that requests initiated from a service worker won't go through a service worker by default. So, I believe we need an optional parameter to fetch that'd override this behavior.

@annevk, maybe we'll need to discuss more on this in line with the whatwg/html#322 where we try to move the active worker definition to Document/WorkerGlobalScope. I remember you sort of proposed replacing the skip-service-worker flag with using the active worker internal slot of a request's client or something like that?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 25, 2016

Yeah, my idea was that client-creating requests would specify the service worker they want to use and we'd have a "client" default value that gets it from the client. And a "none" value to bypass the whole thing.

@jungkees

This comment has been minimized.

Copy link
Contributor

jungkees commented Jul 25, 2016

Coming back to #303 (comment), the problem to solve is which service worker this request will use. As the request in this particular case (e.g. icon) is a subresource request, and the client (the service worker) has no controller, we can't decide which service worker to use. Matching a service worker registration for a subresource request seems not quite right either to me.

@jungkees

This comment has been minimized.

Copy link
Contributor

jungkees commented Jul 25, 2016

I think #303 (comment) is a good option?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 25, 2016

Yeah, changing the API is an option and might be the best really. We have been discussing some kind of IconSet object that might help with this depending on the characteristics. Although I suppose that would not solve it for sounds and such.

You could use blob URLs today of course...

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 25, 2016

You could use blob URLs today of course...

I thought we removed createObjectURL() and revokeObjectURL() from service workers because the life cycle for the URL objects was difficult to define in a SW.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 25, 2016

Ah, data URLs to the rescue! So yeah, we should fix this 😟

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jul 25, 2016

I think we should take this opportunity to experiment with setting a Response instead of a URL. This is something we want more generally, so it would be helpful to try it out on a small API to start.

@beverloo

This comment has been minimized.

Copy link
Member

beverloo commented Jul 25, 2016

For notifications, how would that interact with something like IconSet, where the UA will choose the most appropriate resource to fetch? You'd need Response objects for all options...

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 25, 2016

Yeah. Or we have some API with which the browser can resolve ahead of time somehow…

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 25, 2016

Other APIs to think of here: workers, shared workers, and any kind of async module loading API we may get in future.

The simplest solution may be to special-case fetch in terms of skipping the fetch event. Others go through the fetch event. Let's discuss it at the f2f.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 26, 2016

That doesn't sound too bad actually.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 29, 2016

F2F:

  • We need to check worker loading in fetch - does it use clients correctly.
  • There's also the font loading API, CSP reporting
  • EventSource too, but that shouldn't go through the fetch event from a SW, as it creates a loop as it reconnects
  • We'd also need to "skip service worker" for cache.addAll and importScripts (as well as fetch)
  • This is a lot more complicated than I thought
  • Seems much simpler to allow notification to take a response/file for the icon, but this gets tough when it gets to iconsets. Although getting 10 responses doesn't mean we have to read their body into memory
  • We could create a new client type for the notification, but how long does this client live? As long as the notification is there
  • Decision: special-case notification so it triggers a fetch event in the active worker of this registration, client will be null
@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Aug 1, 2016

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Dec 9, 2016

@wanderview how do you feel about #303 (comment)? If you're happy with it, I'll spec it.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 10, 2016

I'm still concerned about allowing FetchEvent without a Client source we can refer to. The truth is there is a thing triggering the request, but we are just choosing to hide it from script for some reason.

I mean, I'd be happy with something as simple as a "opaque client", "simple client", or something that doesn't allow postMessage, focus, etc. But the SW could compare its ID, etc.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Dec 10, 2016

The client would be the serviceWorker in this case. It makes sense to expose those I think.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Dec 12, 2016

@wanderview does it solve your concerns if we add a new client type of "serviceworker"?

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 12, 2016

does it solve your concerns if we add a new client type of "serviceworker"?

Well, it seems we should do that for foreign fetch anyway. So that is ok.

What protections will we have to prevent a SW from keeping itself alive infinitely via fetch events if it can generate its own fetch event? We must provide some keep-alive budget so that work can be done, but we don't want this to be a way to persist the SW in memory for long periods.

@mkruisselbrink

This comment has been minimized.

Copy link
Collaborator

mkruisselbrink commented Dec 12, 2016

What protections will we have to prevent a SW from keeping itself alive infinitely via fetch events if it can generate its own fetch event? We must provide some keep-alive budget so that work can be done, but we don't want this to be a way to persist the SW in memory for long periods.

Probably the same thing we do to prevent a SW from keeping itself alive indefinitely via message events (w3c/ServiceWorker#980)? What chrome currently implements is to limit the timeout of events that are triggered by a service worker to the remaining timeout of whatever event the worker was processing when triggering the new event.

Note that if we allow service workers to start shared/dedicated workers, we'd need to apply the same logic to fetch events triggered by those workers.

@mkruisselbrink

This comment has been minimized.

Copy link
Collaborator

mkruisselbrink commented Dec 12, 2016

does it solve your concerns if we add a new client type of "serviceworker"?

Well, it seems we should do that for foreign fetch anyway. So that is ok.

There has been discussions about whether foreign fetch should expose client IDs (w3c/ServiceWorker#1017), but I'm not aware of any proposals to have actual Client objects you can look up for these IDs. So not sure we really need a new client type for that use case. It do think it seems reasonable to add such a client type for this.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 12, 2016

Note that if we allow service workers to start shared/dedicated workers, we'd need to apply the same logic to fetch events triggered by those workers.

True. I guess we have to cross this bridge in order support nested workers.

Ok, sounds good to me.

@jakearchibald jakearchibald changed the title Resource fetches for notifications shown from a service worker should trigger onfetch Only specific APIs should skip the fetch event when called within a service worker Dec 13, 2016

@slightlyoff

This comment has been minimized.

Copy link

slightlyoff commented Dec 16, 2016

Dumb question! Is it now possible to call showNotification() (e.g., w/ a tag) from inside onfetch which is triggered by showNotification() (perhaps initially called within an onpush) to keep a loop going?

@jungkees

This comment has been minimized.

Copy link
Contributor

jungkees commented Dec 16, 2016

@slightlyoff, I understand.. you were talking about this:

self.addEventListener('push', e => {
  registration.showNotification('Seeing this again?', {icon: 'same-scope/atlocal.png'});
};

self.addEventListener('fetch', e => {
  registration.showNotification('Seeing this again?', {icon: 'same-scope/atlocal.png'});
};

Not sure if we considered this point (or missed it) that the APIs that intend to follow the proposed default behavior (i.e. enter SW again) can create a loop in this way. Should each API deal with this by having some internal state in the spec algorithm to avoid a loop?

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 16, 2016

I had thought of this, but for some reason I also thought showNotification() was only available from push events.

Should each API deal with this by having some internal state in the spec algorithm to avoid a loop?

It would be really hard to trace this through async callbacks. Consider a fetch handler that did a setTimeout() and then a showNotification().

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Dec 16, 2016

I had also thought of this & mentioned it in #303 (comment) - is this solution not good enough?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Dec 16, 2016

What about CSP and NEL reports? Or things that don't necessarily have a client nor should go through a service worker, such as OSCP? Do we need to set the enum for those to "none"?

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Dec 16, 2016

@annevk I thought we were happy with reports going through the service worker? But yeah, anything which doesn't want to go through any SW should set the enum to "none".

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Dec 16, 2016

@jakearchibald yeah, reports should go through, but might not have a client I think. Forgot exactly how they are setup. From what I remember we wanted them to go through the service worker that was associated with their URL, not the one associated with some document.

OSCP wouldn't have a client either and I guess we'd need to define that somewhere since their specification is in IETF land.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 16, 2016

@jakearchibald Do you mean in that comment that we can get a loop, but its ok because we'll kill it at after its been running for 5 minutes?

@sleevi

This comment has been minimized.

Copy link

sleevi commented Dec 16, 2016

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Dec 16, 2016

@wanderview yeah, we should probably spec how a serviceWorker gets some time to remain open, and how two service workers (or a worker created by a serviceWorker) cannot give each other more time than they have themselves.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 16, 2016

I guess it feels a bit of a foot gun to me. Devs have to understand which API call might possibly have self-interception and carefully avoid fetch loops.

I am also reminded about my other annoyance with this change. We used to be able to say FetchEvent was triggered by either navigation or a request from a controlled client. We now have a third condition which just feels weird.

Anway, even if we made the notification a Client, we would still have the same problem here. I think we could even get this kind of loop by a fetch event handler doing clients.openWindow() triggering another FetchEvent, etc.

So I guess the timer thing is ok.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Dec 16, 2016

@sleevi fair, didn't realize support was limited to Fx. Isn't there some fetching related to certificates though that everyone implements and needs to bypass service workers?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Dec 16, 2016

We used to be able to say FetchEvent was triggered by either navigation or a request from a controlled client.

This wasn't true for reports back then either (unless you bucket them with a navigation somehow, but then this would be similar to that in a way).

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 16, 2016

Anway, even if we made the notification a Client, we would still have the same problem here. I think we could even get this kind of loop by a fetch event handler doing clients.openWindow() triggering another FetchEvent, etc.

Oh, this openWindow() loop doesn't happen because we only allow it via "user interaction".

@sleevi

This comment has been minimized.

Copy link

sleevi commented Dec 16, 2016

@annevk annevk closed this in #435 Feb 14, 2017

annevk added a commit that referenced this issue Feb 14, 2017

Replace skip-service-worker flag with service-workers mode
This allows certain fetches within service workers to trigger fetch events. It also makes interception of redirects by foreign fetch possible.

Tests: web-platform-tests/wpt#4518.

Related service worker PR: w3c/ServiceWorker#1025.

Fixes #303 and fixes #362.
@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Feb 14, 2017

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Feb 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment