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

should clients.claim() control reserved Clients? #1090

Closed
wanderview opened this issue Mar 21, 2017 · 17 comments
Closed

should clients.claim() control reserved Clients? #1090

wanderview opened this issue Mar 21, 2017 · 17 comments

Comments

@wanderview
Copy link
Member

Should it be possible for clients.claim() to take control of a reserved Client? I think the answer is probably "no" since the Client won't have a creation URL until its execution ready. I just want to verify that everyone agrees, though.

@jungkees
Copy link
Collaborator

A corner case that I'm concerned about is when the reserved client's active service worker is handling the main resource fetch while a new active worker is claiming. (The reserved client is considered being controlled already.) If this reserved client is excluded from clients.claim() of a new active worker, only this reserved client will have a different controller (likely a redundant worker) from other clients that are under the same registration's scope.

That said, I think reserved clients should be considered the same as other clients for clients.claim(). But this would make us require the reserved client's creation URL to be set to the request URL which you didn't support through #1034.

@wanderview
Copy link
Member Author

A corner case that I'm concerned about is when the reserved client's active service worker is handling the main resource fetch while a new active worker is claiming. (The reserved client is considered being controlled already.) If this reserved client is excluded from clients.claim() of a new active worker, only this reserved client will have a different controller (likely a redundant worker) from other clients that are under the same registration's scope.

I think that is a different case. The new SW will not move to active if there is a controlled Client. It can use skipWaiting() to automatically take control of already controlled Clients. As you point out, those algorithms should probably apply to reserved Clients.

As I understand it clients.claim() is only about taking control of Clients that are not already controlled.

Its unclear to me what the intent is if the Client is controlled by a different service worker registration due to overlapping scopes. @jakearchibald, what did you intend there?

@jungkees
Copy link
Collaborator

I think that is a different case. The new SW will not move to active if there is a controlled Client.

skipWaiting() makes it ignore that condition.

It can use skipWaiting() to automatically take control of already controlled Clients. As you point out, those algorithms should probably apply to reserved Clients.

As I understand it clients.claim() is only about taking control of Clients that are not already controlled.

skipWaiting() gives a chance for the installed service worker to promote to active even if there are controlled clients. 'clients.claim()` basically replaces the controller of the clients that are under the scope of the service worker's registration if those clients' controller wasn't the same one. So I thought claim() should apply to reserved clients.

Its unclear to me what the intent is if the Client is controlled by a different service worker registration due to overlapping scopes. @jakearchibald, what did you intend there?

clients.claim() targets those clients that have a matching registration and that registrations is the same as the claiming service worker's registration. So the registration with the most specific matching scope is taken when a client is under multiple overlapping scopes. clients.claim() step 3.1.3 and 3.1.4 are that part of the algorithm.

@wanderview
Copy link
Member Author

Well, if the only way to call clients.claim() is if your active, that means there is no Client controlled by another service worker for that registration. So the steps in clients.claim() can only potentially replace a controller on a reserved client controlled by a different registration with an overlapping scope.

@jungkees
Copy link
Collaborator

The "corner case" scenario that I brought in the OP is:
A registration R has both the active worker SW1 and the waiting worker SW2, and the instruction sequence occur as follows.

  1. The reserved client gets a controller SW1 through Handle Fetch step 14.6, and SW1 is serving the fetch event for the reserved client's main resource.
  2. SW2 executes skipWaiting() so SW2 becomes the active worker and SW1 gets to redundant state.
  3. SW2 executes claim().

@wanderview
Copy link
Member Author

SW2 executes skipWaiting() so SW2 becomes the active worker and SW1 gets to redundant state.

I'm saying this is the bug in that case, not the claim(). The Client should not be allowed to be controlled by a redundant worker. If skipWaiting() is called then the reserved Client must be controlled by the new worker.

@jungkees
Copy link
Collaborator

I think the reserved client's handle fetch steps and SW2's skipWaiting() can race. Do you mean it won't be possible that skipWaiting() in SW2 is executed after the reserved client has already got the controller SW1?

@wanderview
Copy link
Member Author

I mean that skipWaiting() must update the controller on the reserved Client. Either the reserved Client gets SW1 and then upgraded to SW2 on skipWaiting(), or it comes in a bit later and just gets SW2.

Why do you think the skipWaiting() would not operate on the reserved Client? It should not depend on the URL of the Client, but simply replace the Clients controlled by SW1. That should be a known list.

@jungkees
Copy link
Collaborator

I found the point where we misunderstood each other.

I mean that skipWaiting() must update the controller on the reserved Client.

To my understanding skipWaiting() doesn't update the controller of any client. What it does is just promoting it to active. And it's the claim()'s role that forcibly applies the updated controller to the clients without involving a navigation.

@wanderview
Copy link
Member Author

To my understanding skipWaiting() doesn't update the controller of any client. What it does is just promoting it to active.

This used to be the case when Clients had an associated registration and it was assumed the active one was the controller. Even still, that should work for reserved clients.

That's probably wrong, now, though. The spec at some point was changed such that the Client has a specific service worker:

https://html.spec.whatwg.org/multipage/webappapis.html#concept-environment-active-service-worker

The skipWaiting() algorithm now needs to go update all those values.

Either way, though, its the same affect. Clients->Registration->Active or Clients->ServiceWorker where ServiceWorker is updated to equal Active.

@jungkees
Copy link
Collaborator

jungkees commented Mar 23, 2017

Here's a bit of history: #586. We separated installEvent.replace() out to self.skipWaiting() and clients.claim(). replace() had been designed to do both automatically back then.

That's probably wrong, now, though. The spec at some point was changed such that the Client has a specific service worker:

I changed this part of the spec. It's nothing but moving the active service worker internal slot from the previous concept of service worker client (an environment settings object) to environment because we needed to attach a controller before the actual environment settings object/global object/document are created.

The skipWaiting() algorithm now needs to go update all those values.

We separated it out from installEvent.replace() to not update the values automatically. I'll need to find more history why we thought it should be. I recall it was driven by some devs requirement though.

@wanderview
Copy link
Member Author

I changed this part of the spec. It's nothing but moving the active service worker internal slot from the previous concept of service worker client (an environment settings object) to environment because we needed to attach a controller before the actual environment settings object/global object/document are created.

I think even further back the controlled windows has ServiceWorkerRegistration objects associated. Gecko was written to match and I wrote a bug to switch to the same model used by the spec:

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

But as comment 3 points out there, though, this is handled down in the Activate algorithm. Its now step 7.1 here:

https://w3c.github.io/ServiceWorker/#activation-algorithm

7. For each service worker client client who is using registration:
  1. Set client’s active worker to registration’s active worker.

I believe a controlled reserved client should count as "using registration" in step 7 here.

@jungkees
Copy link
Collaborator

Ah.. yes. The controlled clients are covered by skipWaiting()-Activate step7.1 as you pointed! Reserved clients should count. Please ignore the scenario that I was concerned about. Sorry for my confusion.

That clarified, clients.claim()'s main role is to cover the case where a client initially called the registration function and the case when a registration with more specific matching scope is registered. Coming back to OP, I'm not so sure, but for the latter case clients.claim() should take control of reserved clients?

@wanderview
Copy link
Member Author

That clarified, clients.claim()'s main role is to cover the case where a client initially called the registration function and the case when a registration with more specific matching scope is registered. Coming back to OP, I'm not so sure, but for the latter case clients.claim() should take control of reserved clients?

So my concern is I'm having a really hard time figuring out how to implement claiming reserved clients. I can't use scope matching reliably because we don't really have the creation URL yet. Also, the code calling claim() is probably running in another process, so trying to search all browser processes for in-flight network requests with a match reserved Client is quite hard (and racy).

I would really like to just say claim() does not operate on reserved clients. In many ways this is similar to what we do today. Today if a navigation starts just after a claim(), the resulting client may end up controlled by another reservation or uncontrolled if loaded via hard-refresh, etc.

@jungkees
Copy link
Collaborator

jungkees commented Mar 24, 2017

I understand your concern. I'll figure out how to make this part of the spec more implementable next week. And also whatwg/html#2455 and whatwg/html#2456.

@jakearchibald
Copy link
Contributor

F2F: clients.claim() doesn't claim reserved clients

@jungkees
Copy link
Collaborator

We'll follow up on the necessary work for this decision in #1245 as V2. (V1 won't expose any state that represents reserved clients.)

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

3 participants