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

navigator.serviceWorker.ready depends subtly on timing of registration, some calls get "stuck" #357

Closed
dominiccooney opened this issue Jul 4, 2014 · 12 comments

Comments

@dominiccooney
Copy link

The "ready" algorithm step 2.3 finds a registration, then waits for it to have an activeWorker (step 4.) This means that if there is interleaved registrations and access to navigator.serviceWorker.ready, some of those ready Promises can get "stuck" observing defunct registrations.

This seems contrary to the use cases I understand for "ready" which are to get a Service Worker that is ready to handle messages/requests/etc., because in some cases those .ready invocations go into a black hole.

@jungkees
Copy link
Collaborator

jungkees commented Jul 7, 2014

A document may have its Service Worker registration, represented by a [[Registration]] object in the spec, during its lifetime. Once the document obtains a [[Registration]] object, the only chance it goes defunct is the unregister() call. And as such, multiple registration attempts nothing but update the active worker within the same [[Registration]] object.

I thought of the behavior of the .ready getter simply as:

  • It checks at its execution time if the associated [[Registration]]'s active worker exists
  • If so, resolve with the value; If not, wait until it gets the value

Am I misunderstanding your concern here?

@dominiccooney
Copy link
Author

If you look at ready step 2.3 it does [[MatchScope]] to read the [[ScopeToRegistrationMap]] and produce a registration object.

Other calls to register with different scopes, but whose scopes also match the document, may write into the [[ScopeToRegistrationMap]]. So the ready property may watch different registrations depending on when it is invoked (because step 2.3/[[MatchScope]] may return a different, new registration.)

Because only one Service Worker can be "active" for a document, some of these registrations will "lose" and one of them will "win". Scripts that accessed navigator.serviceWorker.ready and unluckily started watching a "loser" will never get resolved (neither will they get rejected, because ready does not reject either.)

I think you want to consider how Issue 365 affects ready, too.

@jungkees
Copy link
Collaborator

Yeah, I'll revisit ready as such and ask review.

@dominiccooney
Copy link
Author

Implementer feedback:

Blink has updated its implementation of ready; it used to resolve with the "current" ServiceWorker; now it resolves with the active worker.

Further, it ignores specific registrations and resolves when the matching registration (in terms of [[MatchScope]]) has an active worker. In this way none of them get "stuck" observing a registration. This also makes reusing a Promise over multiple property accesses feasible in more cases.

@nikhilm
Copy link
Contributor

nikhilm commented Jul 24, 2014

@coonsta What do you mean by "ignores specific registrations"?

@KenjiBaheux
Copy link
Collaborator

@coonsta friendly ping for nikhilm's question.

@nikhilm
Copy link
Contributor

nikhilm commented Aug 24, 2014

So ready() continues to be confusing. Is the pattern of using it going to be

if (!navigator.serviceWorker.controller) {
  // We are not controlled
  navigator.serviceWorker.ready.then(function(swr) {
    swr.pushRegistrationManager.doStuff();
  });
} else {
  // Wait! There isn't a way to get the ServiceWorkerRegistration that is controlling this page, which is probably what the page wants to do
}

Using the [[MatchScope]] algorithm means that even if the page is controlled, the registration returned by ready may not be the one controlling the page. But what is the page waiting for then? Sorry if I'm misunderstanding, but the use cases for ready confuse me. Maybe a non-normative section in the spec to document this would be nice too.

For now Gecko is going to only have one Promise ever, and once that resolves, that's it. It will be trivial to follow Blink's path if this issue is resolved so.

@jakearchibald @sicking

@jungkees
Copy link
Collaborator

@coonsta What do you mean by "ignores specific registrations"?

I suppose @coonsta just meant the same thing as:

Using the [[MatchScope]] algorithm means that even if the page is controlled, the registration returned by ready may not be the one controlling the page.

I think .ready does not have to do with .controller. If a page started without a controller, the only way it gets one without reload itself is InstallEvent.replace(). Even for this case, we have navigator.serviceWorker.oncontrollerchange. The expected behavior of .ready that I've thought of is aligned to what @coonsta explained:

it resolves when the matching registration (in terms of [[MatchScope]]) has an active worker.

That is,

  • If there's no registration covering the URL space of the page at all, then it waits until a new registration process gets to [[Activate]] 1.7. In this case, the page will not be controlled before the subsequent navigation.
  • If there's a matching ([[MatchScope]]) registration with an active worker, then it resolves with the registration. Otherwise (if it has no active worker yet), it waits until it gets one.
    • Note that this matching registration can be different from the registration that currently controls the page.

It seems this behavior would cover the following use case better:

navigator.serviceWorker.ready.then(function(swr) {
  swr.pushRegistrationManager.doStuff();
});

If .ready resolves with the registration for the current controller, the above snippet will attach the push manager to the current page's registration even if the subsequent navigation to the current page URL will be using a new registration.

@nhiroki
Copy link
Contributor

nhiroki commented Sep 9, 2014

@jungkees Would it be possible to reflect your previous comment and so on to the ready algorithm? Especially, I'm interested in the timing to resolve the ready promise ([[Activate]] 1.7. ?). I'm going to update the .ready implementation in Blink if necessary (http://crbug.com/399533).

@jungkees
Copy link
Collaborator

Just updated the algorithm: ffaa0fb. The idea behind the algorithm has not been changed. Please let me know if it's different from what has been expected.

@KenjiBaheux
Copy link
Collaborator

@nhiroki could you confirm that the change doesn't impact Blink (I believe so)

@nhiroki
Copy link
Contributor

nhiroki commented Sep 18, 2014

@jungkees Thank you for updating! Yes, that's what I expected.

@KenjiBaheux Confirmed. This was fixed in Blink (http://crbug.com/399533).

jdlrobson added a commit to jdlrobson/pushipedia that referenced this issue Aug 21, 2015
This small powerful change passes the service worker registration
directly to the WikiWorker.

To avoid massive indents and make my code easier to read I add a
getRegisteredWorker function - but there's no need for it to be async :)

A new WikiWorker gets attached to the button and this clears the way for
me not having to work in global scope.

See w3c/ServiceWorker#357 to avoid
confusion.
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

5 participants