Unregistering a service worker should drop its push subscription

RESOLVED FIXED in Firefox 49

Status

()

P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

(Depends on: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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

3 years ago
Summary: about:serviceworkers: "Unregister" should drop push subscriptions → Unregistering a service worker should drop its push subscription

Updated

3 years ago
Priority: -- → P2
We're trying to get rid of about:serviceworkers.  Should we move this to a future devtools request instead?
Something under bug 943220 or bug 1188675?
Flags: needinfo?(kcambridge)
(Assignee)

Comment 3

3 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

3 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)
That looks right but Catalin would know better than I would.
Flags: needinfo?(overholt) → needinfo?(catalin.badea392)
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

3 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)
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

3 years ago
Created attachment 8683776 [details]
MozReview Request: Bug 1185716 - Unregistering a service worker should drop its push subscription. r?catalinb

Bug 1185716 - Unregistering a service worker should drop its push subscription. r?catalinb
Attachment #8683776 - Flags: review?(catalin.badea392)
Attachment #8683776 - Flags: review?(catalin.badea392) → review+
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 13

3 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)
> :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)

Updated

3 years ago
Depends on: 1246642
(Assignee)

Updated

3 years ago
Depends on: 1183245
(Assignee)

Comment 15

3 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
See Also: → bug 1258596
(Assignee)

Comment 16

3 years ago
I meant "bug 1183245 or bug 1258596."
(Assignee)

Comment 17

2 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)

Updated

2 years ago
Duplicate of this bug: 1269767
(Assignee)

Comment 19

2 years ago
Created attachment 8748317 [details] [diff] [review]
unsubscribeOnUnregister.patch

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

2 years ago
Whiteboard: btpp-active
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

2 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

2 years ago
Created attachment 8748438 [details] [diff] [review]
unsubscribeOnUnregister.patch

Incorporated review feedback; carrying r+ forward.
Attachment #8748317 - Attachment is obsolete: true
Attachment #8748317 - Flags: review?(catalin.badea392)
Attachment #8748438 - Flags: review+

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1738cb0de145
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1273948
You need to log in before you can comment on or make changes to this bug.