1236933 - FetchEvent.clientId should be null for non-subresource network requests

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This was discussed in the spec and we decided that the clientId should be null for non-subresource requests:

  https://github.com/slightlyoff/ServiceWorker/commit/872e51b9f06d9872d925a2877f02ca765194ef78

The reasoning here is that there is no service worker controlled client in these cases.  The document resulting from the FetchEvent may be controlled, but it does not exist yet.

In gecko, however, we seem to populate the clientId with the source window that initiated the navigation.  This is wrong as it may not be controlled by the service worker at all.

See:

  https://github.com/slightlyoff/ServiceWorker/issues/808

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Ehsan, do you have time to look at this?  I think we should fix it quickly if we can and possibly uplift.  We don't want people to start relying on clientId for navigations.

Comment 2

[(no longer active)]

I can change our behavior, sure, but does this really make sense?

To me the reasoning behind this change sounds like "this is hard to implement".  We can already find client IDs for non-controlled clients, so I don't understand why the fact that we don't know whether the document is going to be controlled should have any bearing on anything here.  :/  Not supporting clientId on these requests will make it a pain for Web developers if they actually want the clientId for that client (which they can get anyways, but with extra pain.)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #2)
> I can change our behavior, sure, but does this really make sense?
> 
> To me the reasoning behind this change sounds like "this is hard to
> implement".  We can already find client IDs for non-controlled clients, so I
> don't understand why the fact that we don't know whether the document is
> going to be controlled should have any bearing on anything here.  :/  Not
> supporting clientId on these requests will make it a pain for Web developers
> if they actually want the clientId for that client (which they can get
> anyways, but with extra pain.)

Because FetchEvent.clientId is typically the controlled client.  Its a way for the service worker to communicate with the controlled document.

I think it will be more confusing for developers to have the client be the controlled document for some FetchEvents and some other random not-controlled or controlled-by-another-sw page in other FetchEvents.

Also, consider things like opening the page from a homescreen icon or passing the URL on the command line.  What is the "client" in those cases?

I think its more consistent to say there is no client for non-subresource FetchEvents.  Its also easier to reverse this decision later if developers show they expect the non-controlled client to be present.  We can't safely drop the non-controlled clientId for null, though.

In any case, you should raise a spec issue if you strongly disagree.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Also consider that you cannot get the client otherwise for cross-origin navigations, etc.  The clients.matchAll() algorithm only provides access to same-origin clients as far as I can tell.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Also, Jungkees raises a problem with using the source clientId in his original IRC question here:

  http://logs.glob.uno/?c=freenode%23whatwg&s=11+Nov+2015&e=11+Nov+2015&h=clientId#c974050

If a user clicks on a link, the FetchEvent.clientId would be pointing at a document that is in the process of being destroyed or moved into the bfcache.

Comment 6

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #3)
> (In reply to :Ehsan Akhgari from comment #2)
> > I can change our behavior, sure, but does this really make sense?
> > 
> > To me the reasoning behind this change sounds like "this is hard to
> > implement".  We can already find client IDs for non-controlled clients, so I
> > don't understand why the fact that we don't know whether the document is
> > going to be controlled should have any bearing on anything here.  :/  Not
> > supporting clientId on these requests will make it a pain for Web developers
> > if they actually want the clientId for that client (which they can get
> > anyways, but with extra pain.)
> 
> Because FetchEvent.clientId is typically the controlled client.  Its a way
> for the service worker to communicate with the controlled document.

Err, why?  Non-controlled documents can also have a clientId and can be communicated with just fine, no?

> I think it will be more confusing for developers to have the client be the
> controlled document for some FetchEvents and some other random
> not-controlled or controlled-by-another-sw page in other FetchEvents.

I don't understand how the controlled-ness matters here.  IIRC clientId was added instead of FetchEvent.client in order to let the SW communicate with the respective client, no?

> Also, consider things like opening the page from a homescreen icon or
> passing the URL on the command line.  What is the "client" in those cases?

I'm probably missing something huge.  At least in Gecko, Client has nothing to do with opener, so how the document has been opened doesn't change anything about the Client that represents it.

> I think its more consistent to say there is no client for non-subresource
> FetchEvents.  Its also easier to reverse this decision later if developers
> show they expect the non-controlled client to be present.  We can't safely
> drop the non-controlled clientId for null, though.
> 
> In any case, you should raise a spec issue if you strongly disagree.

I just think that I'm missing the reasoning, but if I can't convince you I have no hopes of convincing anyone else.  :-)  I'll reiterate what I am thinking here:

* Clients can be controlled or non-controlled, and a clientId for a controlled document is no different than one for a non-controlled document (or a document controlled by another SW) from the perspective of anything in the spec.
* It is useful to expose the clientId of the request's client on FetchEvent.  The same use cases apply regardless of whether the FetchEvent is for a subresource or not.
* Even if we hide FetchEvent.clientId, the service worker will observe that clientId for other resources on that client, therefore we have only made accessing the clientId more painful, not impossible.

Based on the above, I think we should make the clientId accessible on all FetchEvents.  What in the above am I getting wrong?

(In reply to Ben Kelly [:bkelly] from comment #4)
> Also consider that you cannot get the client otherwise for cross-origin
> navigations, etc.  The clients.matchAll() algorithm only provides access to
> same-origin clients as far as I can tell.

Sure, but since non-subresource cross origin requests never get intercepted, there is no FetchEvent for them in the first place, so they're a non-issue as far as clientId is concerned.

(In reply to Ben Kelly [:bkelly] from comment #5)
> Also, Jungkees raises a problem with using the source clientId in his
> original IRC question here:
> 
>  
> http://logs.glob.uno/
> ?c=freenode%23whatwg&s=11+Nov+2015&e=11+Nov+2015&h=clientId#c974050
> 
> If a user clicks on a link, the FetchEvent.clientId would be pointing at a
> document that is in the process of being destroyed or moved into the bfcache.

Hmm, this (and now that I reread it, comment 0 too) seems to suggest that FetchEvent.clientId for a non-subresource fetch is set to the clientId of the "previous document" (aka the document from which the top-level fetch is initiated) which is not at all what the spec used to say, so I don't think this is an issue.  The old spec said that FetchEvent.clientId in that case will be the clientId corresponding to the new environment settings object that has been created for the newly fetched document.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #6)
> Hmm, this (and now that I reread it, comment 0 too) seems to suggest that
> FetchEvent.clientId for a non-subresource fetch is set to the clientId of
> the "previous document" (aka the document from which the top-level fetch is
> initiated) which is not at all what the spec used to say, so I don't think
> this is an issue.  The old spec said that FetchEvent.clientId in that case
> will be the clientId corresponding to the new environment settings object
> that has been created for the newly fetched document.

The problem with using the target document of the navigation is that it may never exist.  The service worker can return an error resulting in no document.  This "client" doesn't really exist at the time the FetchEvent is processed.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

IRC is failing me at the moment.

I'm personally unconvinced we need the complexity of exposing a clientId to an unloaded dochshell right now.  The spec agrees with me at the moment.  If you feel otherwise, please go write a spec issue so we can have the discussion there.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Or comment in the existing spec issue here:

  https://github.com/slightlyoff/ServiceWorker/issues/808

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Ehsan clarified in IRC that while we expose FetchEvent.clientId, the corresponding clients.get(evt.clientId) for a navigation will return null during the FetchEvent handler.  I think this satisfies my concerns.  We need to sync with the spec, though.

Comment 11

[(no longer active)]

self-needinfo to not drop the ball here...

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

The spec conversation proposes keeping .clientId null for non-subresource FetchEvent, but maybe adding a separate .potentialClientId.  This would let code distinguish between clients that were closed and clients that haven't opened yet.

https://github.com/slightlyoff/ServiceWorker/issues/808#issuecomment-169625196

Ehsan, what do you think?

Comment 13

[(no longer active)]

That's fine by me.  I'll submit a patch to return null in this case for now.

Comment 14

[(no longer active)]

Attachment: Return null from FetchEvent.clientId for non-subresource network requests

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8705798 [details] [diff] [review]
Return null from FetchEvent.clientId for non-subresource network requests

Review of attachment 8705798 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d47e55ac4177

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d47e55ac4177