Inconsistencies due to when clients are created

[jakearchibald]

https://jakearchibald.github.io/isserviceworkerready/demos/clients-count/

1. Visit the above
2. Hit shift + refresh
3. Hit refresh

Chrome says 1, Firefox says 0.

The Firefox behaviour seems right to me, but unless I'm reading it wrong, the spec isn't clear around this. This might be because of earlier confusion around when clients are created.

From https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-documents:

Set request's client to the source browsing context's active document's Window object's environment settings object

If I'm reading this correctly, the client is the window being navigated, not a new client ready for the new page.

From https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#on-fetch-request-algorithm 12.1:

If client is not a secure context, return null.

This seems to assume that the client is a new client ready for the new page, but doesn't this mean that SW will fail if you're navigating from an unrelated insecure page?

[jakearchibald]

Related Chrome bug https://bugs.chromium.org/p/chromium/issues/detail?id=514148

[wanderview]

I think this is a Firefox bug and chrome is more correct.

The current spec says to control windows. And it says to associate the service worker with the window (i.e. control it) before firing the FetchEvent. So there should be an existing controlled client when a navigation FetchEvent is fired.

The problem in Firefox is that we control documents, not windows. The document does not exist before the navigation, so we don't have a controlling client yet. There is a bug open for this, but there was some discussion at the last f2f that we might want to change to controlling documents in the spec. So we have not moved on this.

https://bugzilla.mozilla.org/show_bug.cgi?id=1237498

In regards to this spec language:

If client is not a secure context, return null.

I think the spec is trying to check for a non-secure parent window. It probably needs to be re-worded since it should use the navigation URL security and parent window security to make this determination.

[jakearchibald]

We should talk about this at the f2f. I think the Firefox behaviour makes more sense, especially given what we do with fetchEvent.clientId.

[wanderview]

If we decide to keep the "client is Window" behavior currently in spec, then I think we could revisit providing a value in fetchEvent.clientId for navigations. My objections there were largely based on my misconception about clients being documents.

[jakearchibald]

This issue is very related to #811

[jakearchibald]

F2F:

• A reserved client is created for a navigation
• fetchEvent.clientId - represents the client that initiated the request
• fetchEvent.reservedClientId - represents the client that has been created for potentially-client-creating request, which may not be used in the event of a redirect, content-disposition, network error, or other response rejection
• fetchEvent.targetClientId - the id of the client that the new client will replace if it's used
• clients.matchAll() will not return reserved clients
• clients.matchAll({ includeReserved: true }) will return reserved clients
• Reserved clients will have url about:blank
• Reserved clients will inherit visual properties from their target client (which may be a cross origin client, or new tab)
• client.used will be false for reserved clients
• clients.get will return a reserved client
• postMessage on a reserved client will be buffered

Some of the naming is up for bikeshedding, specifically:

• "reserved", in all the properties it's used
• targetClientId or replacesClientId - intent is to know which client
• client.used

[jakearchibald]

openedWindow.location.href = 'blah' - what client initiates this request? The openedWindow or the window which executed the code?

[jakearchibald]

targetClientId will be the same as reservedClientId if the link is being opened in a new window.

For worker requests targetClientId will be the same as reservedClientId

[wanderview]

@jakearchibald Can we mark this one decided? I thought we decided it in the meeting.

[wanderview]

Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1264177

[mfalken]

Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=514148

[mkruisselbrink]

Since we didn't explicitly discuss this, I wonder if what we came up with also sufficiently addresses worker clients. For workers their settings object (client?) is created before the worker is fetched, and that worker is then used as the client for the actual request to fetch the worker.

So would that be an exception to the fetchEvent.clientId represents the client that initiated the request rule? Or since it's not a navigation (but it is a new controlled client) would we only have a clientId and have both reservedClientId and targetClientId be null/undefined?

[jungkees]

For workers their settings object (client?) is created before the worker is fetched, and that worker is then used as the client for the actual request to fetch the worker.

I think this is not true. The fetch in run a worker is invoked with outside settings which is the source client.

[jungkees]

I outlined the redefinition of the service worker client concept: https://gist.github.com/jungkees/03d4473ae47bd222539ab9cf5c9afd31

Hooks to HTML's navigation and run a worker will be needed (with some changes to whatwg/html#322).

In the gist proposal, I suggested we have a single .potentialClientId (with its state defined) over having both .reservedClientId and .targetClientId.

Please take a look.

/cc @annevk @jakearchibald @slightlyoff @mkruisselbrink @wanderview @ehsan

[annevk]

I don't really understand the motivation. It says a service worker can outlive a client, and then it goes on about how a service worker exists before a client is created.

How does this work in browsers today?

Supposedly we always know when we are creating a new client, so we could generate an ID that we then later use once the client is created. Is that what you're trying to accomplish here?

[jungkees]

so we could generate an ID that we then later use once the client is created.

This is what I suggested.

1. Navigation is triggered.
2. [Navigation] Invoke fetch.
3. [Fetch] Invoke Handle Fetch.
4. [Handle Fetch] Create (potential) target client information
5. [Navigation] Create (target) Window/Document
6. [Navigation] Set Window/Document's client information to the value created in 4.

In order to do that, the step 4 and 6 above should be added. And for this, I wanted to have some internal slots for id and active worker on a fetch algorithm instance or on a request.

How does this work in browsers today?

Blink creates some corresponding state (object) when fetch occurs, and the properties in that object are later associated with the the document/worker. So, largely the steps I suggest here is what the implementation does now.

[annevk]

I think it would be better if we created the ID between step 1 and 2 and pass that along in the request. After all, navigate ends up creating the document which is presumably where this information is stored eventually.

[annevk]

(And a similar setup for workers.)

[jungkees]

it would be better if we created the ID between step 1 and 2 and pass that along in the request.

This sounds fine to me.

How about the matched active worker? Handle fetch should be the best place for this I think. Any better idea?

[annevk]

That would mean it would have to somehow make its way back up the chain (e.g., by sticking on a response), which seems a little ugly. What is needed to determine the active worker?

[jungkees]

Right. Determining the active worker is done in Handle Fetch step 12.3. That is, the fetch is for a non-subresource request and the scope of the request url matches one of the registrations.

[annevk]

Okay, so the worry is about some kind of race condition if we match twice on the URL? I think you have mentioned that earlier.

The operations performed on client there don't make sense, right? Client doesn't exist yet for non-subresource requests.

I think here too it makes more sense if we end up dictating what service worker to use. Rather than letting "Handle Fetch" figure it out somehow and tell us about it later what it came up with.

It also seems plausible we might have more such cases in the future where you'd want to fetch something using a particular service worker.

One way to do this would be to change the "skip-service-worker flag" of request into a "service worker" setting. Which is one of "none", "client", or a service worker instance of sorts.

[wanderview]

Note, we're unlikely to get any review here from google folks while google I/O is going on this week.

[jungkees]

@annevk I'm looking at more of how the implementations currently work. Blink finds matched registration in navigation algorithm equivalent phase and get its active worker right before dispatching the fetch event (in handle fetch algorithm equivalent phase). Your suggestion seems okay to me now. Let me try to examine a bit more next week. @wanderview Google folks will be able to have a look by then. On the spec side, Anne and I can discuss, and on the API side, more discussion would be needed I guess.

[jungkees]

I think we should rather set the matched service worker registration to a request rather than the active worker. The active worker can change (by waiting worker being promoted) between the step in navigation and the step later in Handle Fetch if the registration has had both the active worker and the waiting worker.

Also, in the same reason, the active worker that will be attached to the client (document or other worker) should be determined when the registration's active worker for the fetch event is determined (in Handle Fetch). So, it seems to me that the active worker should be passed back to the caller within a response. Or can we create some outer object in navigate that has the active worker slot and pass the reference to fetch/Handle Fetch and change its state in the Handle Fetch?

[annevk]

That seems like an unfortunate setup since in general fetch doesn't need to know about this, only for navigation.

[jungkees]

Status note:

• Addressed the following (8b483b0):
  • Use of request's reserved client (an environment or an environment settings object created during a navigation or a worker creation respectively in HTML) and request's target client id to expose fetchEvent.reservedClientId/targetClientId to fetch event handlers
  • Match a service worker registration and attach its active worker, if available, to the reserved client
• Updated clients.get(id): 4ce01b5
• Updated clients.matchAll(options): 3893c35
• Updated clients.claim(): a279060
• Added client.reserved: d88e979 - Defining the execution ready flag in HTML which .reserved uses is being tracked in Extend environment concept whatwg/html#2004

[jungkees]

The issue has been addressed by adding/modifying the relevant concepts and steps in SW, HTML (navigate, run a worker, etc.), and Fetch. (See #870 (comment) for the related changes.)

Closing.