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

resurrected service workers don't fire an activate event which may break expectations #1204

Closed
wanderview opened this issue Oct 6, 2017 · 12 comments · Fixed by #1415
Closed
Assignees
Milestone

Comments

@wanderview
Copy link
Member

wanderview commented Oct 6, 2017

Consider a service worker that looks like this:

self.addEventListener('activate', evt => evt.waitUntil(clients.claim()));

And then a main page that waits for a window to be controlled:

navigator.serviceWorker.register('sw.js');
await waitForControlled(existingWindow);

This works fine if the SW is installed fresh. If the registration resurrects a service worker that was previously unregistered, then this code will break. The activate event will not fire and the claim() will not occur.

Should we consider firing another activate event or some other new event when the service worker is resurrected like this? It seems that the intent of restoring the SW to the active slot is that it should be equivalent to registering the new worker.

@n8schloss
Copy link

I think that makes sense, this is a hard edge case for many developers to reason about. I do think that if we fire 'activate' again that we probably want to fire 'install' again too. I could see developers getting confused about 'activate' firing in this case but not 'install'.

@NekR
Copy link

NekR commented Oct 6, 2017

If the registration resurrects a service worker that was previously unregistered, then this code will break.

What does this mean exactly? How does this happen?

@wanderview
Copy link
Member Author

What does this mean exactly? How does this happen?

When you call unregister() on a registration with an active worker that is still controlling clients, then the worker doesn't die immediately. It gets an uninstalling flag and will normally go away once the last client closes. If someone calls register() with the same params before that happens, though, then SW is "resurrected". We just remove the uninstalling flag and let it keep on living. While the uninstalling flag is set, though, the worker misses out on controlling clients, doesn't show in getRegistrations(), etc.

I think that makes sense, this is a hard edge case for many developers to reason about. I do think that if we fire 'activate' again that we probably want to fire 'install' again too. I could see developers getting confused about 'activate' firing in this case but not 'install'.

Well, if we are going to do that, maybe we should just get rid of resurrection completely and make a new installing worker in this case. Once a worker is doomed as uninstalling it would never come back.

Also, right now we have an invariant that SW firing the install event are reg.installing. That wouldn't hold for what you are proposing here and could break things. (Unlikely, though.)

@NekR
Copy link

NekR commented Oct 7, 2017

From JS perspective is sounds like ServiceWorker is being removed completely and then added again. I mean, that's certainly unexpected, especially if that worker doesn't control anything anymore (if I got it right) and isn't showing in any API to be alive.

I is how I think it's working (I haven't thought much uninstalling process before):

  • SW registered
  • Pages loaded (3 for example)
  • SW is unregistered in tabs 3
  • Browser calls redundant for active worker
  • Worker doesn't go away (as you said), but keeps controlling current pages
  • Redundant worker doesn't receive control of new tabs
  • When another tabs calls register(), even with same params, new SW is created and starts normal live cycle events
  • On active, it new SW claims all current tabs and steals them from redundant worker

Much like an update, but with a redundant (irony) step of unregistration.

@jakearchibald
Copy link
Contributor

I'm unsure about the problem in the OP. You'll have a similar problem if existingWindow was shift-reloaded.

In cases where I want claiming to happen like this, I post a message to the service worker asking it to claim all. This is how https://jakearchibald.github.io/isserviceworkerready/demos/claim/ works.

That said…

maybe we should just get rid of resurrection completely

Resurrection has never sat well with me. I support getting rid of it if we can.

@wanderview
Copy link
Member Author

I used the message+claim approach to fix the test I found running into this. So yea, that does work. I just don't see it as a common pattern (but I'm a c++ person so maybe not surprising).

I do think its a bit different from shift-reload. In that case the window does not actually end up controlled. With the resurrection case you end up controlled, but never told you are controlled.

Note, this is very similar to #1198. That one was a bit easier to solve, though, because we can safely just resolve the promise.

@jakearchibald
Copy link
Contributor

In terms of dropping resurrection: we should just remove the registration from the map. If .register is called for the same scope, we create a new registration and add it to the map.

@mfalken
Copy link
Member

mfalken commented Nov 7, 2017

Sounds good to me to remove resurrection. It causes complexity in the code and I doubt sites are relying on it.

I was briefly worried you'd get into situations where getRegistration() can return a registration that doesn't have your controller in it, but I think that's already possible today. For example, you can unregister then register a new registration with a longer matching scope.

When making the spec change for this, we'd want to update and review unregister-then-register.https.html and unregister-then-register-new-script.https.html.

@jakearchibald
Copy link
Contributor

F2F: resurrection event instead? It's a hook for developers to read about.

Currently in the spec, if you unregister the SW, then reregister with a different URL, if that fetch fails the registration is resurrected. New clients will be controlled by the reg while the script is fetching.

resurrection must happen on the active worker, regardless of script URL changes.

We're not really happy with any of these ideas.

Action: @jakearchibald to write a blog post asking developer on expected behaviour.

@jungkees
Copy link
Collaborator

We move this feature to V2 to have more discussion on the desired behavior.

@jakearchibald
Copy link
Contributor

F2F: Kill it. Update tests.

@jakearchibald
Copy link
Contributor

PR #1415

jakearchibald added a commit that referenced this issue Jun 6, 2019
* Remove repetition in respondWith

* Kill resurrection (fixes #1204)

* Better id

* Addressing feedback

* Oops
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@jakearchibald @NekR @wanderview @jungkees @mfalken @n8schloss and others