Closed
Bug 1185716
Opened 9 years ago
Closed 9 years ago
Unregistering a service worker should drop its push subscription
Categories
(Core :: DOM: Notifications, defect, P2)
Core
DOM: Notifications
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
References
(Depends on 1 open bug)
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
lina
:
review+
|
Details | Diff | Splinter Review |
The "Unregister" button removes the service worker registration, but keeps the push subscription. The only way to drop the subscription is to call `pushSubscription.unsubscribe()`, which is clunky when developing locally. We should make this button drop both the registration and push subscription, assuming one exists.
Assignee | ||
Updated•9 years ago
|
Summary: about:serviceworkers: "Unregister" should drop push subscriptions → Unregistering a service worker should drop its push subscription
Updated•9 years ago
|
Priority: -- → P2
Comment 1•9 years ago
|
||
We're trying to get rid of about:serviceworkers. Should we move this to a future devtools request instead?
Assignee | ||
Comment 3•9 years ago
|
||
Actually, I think this should be handled by `ServiceWorkerRegistration.unregister()`. The Push spec says, "when a service worker registration is unregistered, any associated push subscription MUST be deactivated." I'll add this to our backlog. Thanks, Andrew!
Flags: needinfo?(kcambridge)
Assignee | ||
Comment 4•9 years ago
|
||
Andrew, is https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2104-2199 the right place to add this? We can use `nsIPushClient.unregister` to drop the subscription, but I don't know where to plug it in.
Flags: needinfo?(overholt)
Comment 5•9 years ago
|
||
That looks right but Catalin would know better than I would.
Flags: needinfo?(overholt) → needinfo?(catalin.badea392)
Comment 6•9 years ago
|
||
ServiceWorkerUnregisterJob::Unregister would be the right place, yes. Note that if there are clients using the registration, we just set the uninstall flag and don't remove it right away. Should the push subscription be removed it that case?
Flags: needinfo?(catalin.badea392)
Assignee | ||
Comment 7•9 years ago
|
||
Thanks, Catalin! I think we should remove the push subscription immediately, even if clients are still using the worker, but Martin can confirm.
Flags: needinfo?(martin.thomson)
Comment 8•9 years ago
|
||
Yes, I don't see any point in retaining the subscription. Firing off a DELETE request shouldn't hurt that much.
Flags: needinfo?(martin.thomson)
Assignee | ||
Comment 9•9 years ago
|
||
Bug 1185716 - Unregistering a service worker should drop its push subscription. r?catalinb
Attachment #8683776 -
Flags: review?(catalin.badea392)
Assignee | ||
Comment 10•9 years ago
|
||
Updated•9 years ago
|
Attachment #8683776 -
Flags: review?(catalin.badea392) → review+
Comment 11•9 years ago
|
||
Comment on attachment 8683776 [details]
MozReview Request: Bug 1185716 - Unregistering a service worker should drop its push subscription. r?catalinb
https://reviewboard.mozilla.org/r/24419/#review22029
lgtm
::: dom/workers/ServiceWorkerManager.cpp:2242
(Diff revision 1)
> + RemovePushSubscription(bool aSuccess)
Can this be renamed to something that makes it more obvious that it also resolves the unregister promise?
Assignee | ||
Comment 12•9 years ago
|
||
Assignee: nobody → kcambridge
Assignee | ||
Comment 13•9 years ago
|
||
The oranges on Try are caused by the `nsIQuotaManagerService.reset()` calls in test_cache_{orphaned_body, orphaned_cache, restart, shrink}.html, which invalidate the Push IDB database. Trying to operate on that causes this line in `IndexedDBHelper.newTxn` to throw (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/base/IndexedDBHelper.jsm#138), and never call `successCb` or `failureCb`. Since this patch unconditionally calls `nsIPushService.unsubscribe()` on unregister, the test hangs.
:bkelly mentioned we drop all service worker registrations on quota eviction, but I can't find the code that does that. Any ideas, :baku?
I'm also wondering if it makes sense for the quota manager to notify push (or broadcast an observer notification), so that it can recover after invalidation.
Flags: needinfo?(amarchesini)
Comment 14•9 years ago
|
||
> :bkelly mentioned we drop all service worker registrations on quota
> eviction, but I can't find the code that does that. Any ideas, :baku?
Actually, we don't do that yet. Do you mind to file a bug and assign it to me?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 15•9 years ago
|
||
Another alternative is to remove IndexedDB from Push and use flat files, which is what https://wiki.mozilla.org/Performance/Avoid_SQLite_In_Your_Next_Firefox_Feature recommends. That would solve the issues with invalidation and IDB schema changes when switching between release channels...but it also seems excessive.
Un-assigning myself for now, since this is blocked on either bug 1246642 or bug 1258596. Once we decide what to do about IDB, we can land this patch.
Assignee: kcambridge → nobody
Assignee | ||
Comment 16•9 years ago
|
||
I meant "bug 1183245 or bug 1258596."
Assignee | ||
Comment 17•9 years ago
|
||
It occurred to me that bug 1264710 might "fix" this. Resetting the quota manager will still break the IDB database, but at least it'll catch the exception and reject instead of hanging.
Push should still recover from quota invalidation, but this may be a good short-term workaround to get the tests passing.
Assignee | ||
Comment 19•9 years ago
|
||
Looks like this does the trick. The cache tests now pass for me locally.
Ben, I see you refactored ServiceWorkerUnregisterJob a while back, so flagging you for additional review to make sure I did this right.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eec143548d14
Assignee: nobody → kcambridge
Attachment #8683776 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8748317 -
Flags: review?(catalin.badea392)
Attachment #8748317 -
Flags: review?(bkelly)
Assignee | ||
Updated•9 years ago
|
Whiteboard: btpp-active
Comment 20•9 years ago
|
||
Comment on attachment 8748317 [details] [diff] [review]
unsubscribeOnUnregister.patch
Review of attachment 8748317 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good. r=me with comments addressed.
Note, I think the push spec should probably clarify when the push subscription is removed. There are two things that can happen when you call unregister():
1) No client is using the registration and its actually removed.
2) A client is using the registration and the registration remains with its "uninstalling flag" set. The registration may be resurrected later by a register() call.
This patch removes the subscription for case (1). Are we sure it should remain for case (2)? Might be worth a spec issue to discuss.
For now, though, I think this patch is a step in the right direction no matter what. Lets get this landed.
::: dom/workers/ServiceWorkerManager.cpp
@@ +15,5 @@
> #include "nsIHttpChannelInternal.h"
> #include "nsIHttpHeaderVisitor.h"
> #include "nsINetworkInterceptController.h"
> #include "nsIMutableArray.h"
> +#include "nsIPushService.h"
It seems this should not be necessary now that its in a different file?
::: dom/workers/ServiceWorkerUnregisterJob.cpp
@@ +13,5 @@
> namespace workers {
>
> +namespace {
> +
> +class PushUnsubscribeCallback final : public nsIUnsubscribeResultCallback
Please make this ServiceWorkerUnregisterJob::PushUnsubscribeCallback so that it can call private methods on the the job class. I'd rather not make Unregister public.
@@ +35,5 @@
> + return NS_OK;
> + }
> +
> +private:
> + virtual ~PushUnsubscribeCallback()
This does not need to be virtual since the class is final and your Release() method is defined in this class. I think the preference is to not make it virtual in these cases.
::: dom/workers/ServiceWorkerUnregisterJob.h
@@ +23,5 @@
> bool
> GetResult() const;
>
> + void
> + Unregister();
Please make this private and the callback class nested within ServiceWorkerUnregisterJob.
Attachment #8748317 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 21•9 years ago
|
||
Thanks, Ben!
(In reply to Ben Kelly [:bkelly] from comment #20)
> Note, I think the push spec should probably clarify when the push
> subscription is removed.
Agreed. Let's discuss in https://github.com/w3c/push-api/issues/190.
Assignee | ||
Comment 22•9 years ago
|
||
Incorporated review feedback; carrying r+ forward.
Attachment #8748317 -
Attachment is obsolete: true
Attachment #8748317 -
Flags: review?(catalin.badea392)
Attachment #8748438 -
Flags: review+
Comment 23•9 years ago
|
||
Comment 24•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•