1185716 - Unregistering a service worker should drop its push subscription

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect, P2)

Description

[Lina Butler [:lina]]

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.

Comment 1

[Andrew Overholt [:overholt]]

We're trying to get rid of about:serviceworkers.  Should we move this to a future devtools request instead?

Comment 2

[Andrew Overholt [:overholt]]

Something under bug 943220 or bug 1188675?

Comment 3

[Lina Butler [:lina]]

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!

Comment 4

[Lina Butler [:lina]]

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.

Comment 5

[Andrew Overholt [:overholt]]

That looks right but Catalin would know better than I would.

Comment 6

[Cătălin Badea (:catalinb)]

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?

Comment 7

[Lina Butler [:lina]]

Thanks, Catalin! I think we should remove the push subscription immediately, even if clients are still using the worker, but Martin can confirm.

Comment 8

[Martin Thomson [:mt:]]

Yes, I don't see any point in retaining the subscription.  Firing off a DELETE request shouldn't hurt that much.

Comment 9

[Lina Butler [:lina]]

Attachment: 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

Comment 10

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d6e6c4ea42e

Comment 11

[Cătălin Badea (:catalinb)]

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?

Comment 12

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8291c66fe4c7

Comment 13

[Lina Butler [:lina]]

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.

Comment 14

[Andrea Marchesini [:baku]]

> :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?

Comment 15

[Lina Butler [:lina]]

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.

Comment 16

[Lina Butler [:lina]]

I meant "bug 1183245 or bug 1258596."

Comment 17

[Lina Butler [:lina]]

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.

Comment 19

[Lina Butler [:lina]]

Attachment: 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

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 21

[Lina Butler [:lina]]

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.

Comment 22

[Lina Butler [:lina]]

Attachment: unsubscribeOnUnregister.patch

Incorporated review feedback; carrying r+ forward.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1738cb0de145

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1738cb0de145