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 ServiceWorkerContainer.getRegistrations() check the uninstalling flag #943

Closed
wanderview opened this issue Aug 4, 2016 · 13 comments
Milestone

Comments

@wanderview
Copy link
Member

Currently we set the uninstalling flag when an unregister job runs but we can't remove the registration yet. For example, its still controlling a client. This flag is then used to hide the registration when someone calls navigator.serviceWorker.getRegistration().

We don't, however, check the uninstalling flag in navigator.serviceWorker.getRegistrations(). So an uninstalling registration will show up there.

Is this intentional? It seems very confusing to me that the registration for your scope might show up in getRegistrations(), but not getRegistration(). I would expect them to have similar logic.

Note, an uninstalling registration will still be exposed via self.registration in the service worker thread itself.

@wanderview wanderview added this to the Version 1 milestone Aug 4, 2016
@wanderview
Copy link
Member Author

I'm going to implement hiding uninstalling registrations for now.

@wanderview
Copy link
Member Author

Jake gave me a vague head nod in IRC, so I'm going to land this in gecko with a new wpt test case.

@jungkees
Copy link
Collaborator

jungkees commented Aug 9, 2016

I agree to add checking uninstalling flag to getRegistrations(). To me, setting the uninstalling flag of a registration means it won't be valid but just continue to control the existing clients until they unload. So, I think self.registration should return undefined for consistency. WDYT?

One occasion we might have to consider is when the uninstalling registration is being reclaimed by re-register() before being cleared up. In this case, getRegistration(), getRegistrations() and self.registration in the same global will start to return the registration again. I think it seems reasonable. Does anyone have any concern about this behavior?

@wanderview
Copy link
Member Author

I'm not sure its worth clearing self.registration. Code can always keep one of these DOM objects alive just by doing self.myReg = self.registration.

We could expose a self.registration.uninstalling flag, though.

@jungkees
Copy link
Collaborator

I'm not sure its worth clearing self.registration. Code can always keep one of these DOM objects alive just by doing self.myReg = self.registration.

Yeah, clearing self.registration seems not necessary as you mentioned though this is also true for getRegistration(url) in a document where the subsequent calls to the same method after the uninstalling flag is set won't return the registration. But the latter case is more natural as we don't want to match a SW of the uninstalling registration to a new client.

Let's keep the current behavior for self.registration.

We could expose a self.registration.uninstalling flag, though.

I think this is not necessary until we get some requirements from devs.

@rubys
Copy link
Member

rubys commented Aug 10, 2016

I think this is not necessary until we get some requirements from devs.

I guess I qualify as a dev. I posted the following recently: https://lists.w3.org/Archives/Public/public-webapps/2016JulSep/0016.html - see point #5.

I would like EITHER navigator.serviceWorker.getRegistrations() to not return registrations that are in the process of being uninstalled, OR for there be a way to check if a registration that was returned is in the process of being uninstalled.

Fair enough?

@jungkees
Copy link
Collaborator

Sorry to have missed the point 5 there. Thanks for reminding me. Indeed, the OP is exactly around that requirement, and I agree to change it as suggested: ".getRegistrations() to not return registrations that are in the process of being uninstalled."

I don't have a strong feeling about adding self.registration.uninstalling though. If it is the case it has a good use case even after changing the getRegistrations() behavior as above, please let me know.

@jungkees
Copy link
Collaborator

Let's talk about self.registration.uninstalling in a new thread if it is required. Thanks.

@jungkees
Copy link
Collaborator

/cc @wanderview @mattto @nhiroki @aliams

@wanderview
Copy link
Member Author

LGTM. I'm ambivalent to the uninstalling flag.

@aliams
Copy link
Contributor

aliams commented Aug 10, 2016

I agree to change it as suggested: ".getRegistrations() to not return registrations that are in the process of being uninstalled."

@jungkees, that sounds good to me.

Let's talk about self.registration.uninstalling in a new thread if it is required.

I'm not sure I understand the use case of exposing an uninstalling flag via self.registration.uninstalling. Can you clarify please?

@jungkees
Copy link
Collaborator

I'm not sure I understand the use case of exposing an uninstalling flag via self.registration.uninstalling.

I haven't come across any use case yet. I guess @wanderview might have thought it would help in case we need some way to prevent further operations on a registration when it's being uninstalled. However, I don't see any issue with the current behavior: when called on an uninstalling registration, registration.update() rejects, and registration.unregister() resolves with true if it's still waiting to be cleared or resolves with false if it's already cleared, which seem reasonable to me.

@wanderview
Copy link
Member Author

One use case would be short circuiting logic if you see self.registration.uninstalling is true. For example, you might not perform some expensive operation since you know the result will be thrown away soon.

But in general I don't think we really need this yet.

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

No branches or pull requests

4 participants