1266433 - Always notify observers about push subscription changes

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Jan Keromnes [:janx]]

In about:debugging, we would like to show Push subscription-related information (bug 1188981), like for example the endpoint URL. To keep this information up-to-date, we would like a way to be notified when a push subscription changes.

It seems we can subscribe to such an event with:

    Services.obs.addObserver(this, PushService.subscriptionChangeTopic, false);

However, this only works in the case of a system principal or in tests, due to `PushNotifier::ShouldNotifyObservers` [0].

- Could we maybe remove `ShouldNotifyObservers`, and make `PushNotifier` always send this event?

- Also, it seems that this event is not sent if a service worker calls `pushSubscription.unsubscribe()`. Can we make it do that?

[0] http://mxr.mozilla.org/mozilla-central/source/dom/push/PushNotifier.cpp#303

Comment 1

[Jan Keromnes [:janx]]

Side-note: Being notified about push messages with the code below could also be useful.

    Services.obs.addObserver(this, PushService.pushTopic, false);

Kit, who would be a good person to address this?

Comment 2

[Jan Keromnes [:janx]]

Kit, assigning to you as requested on IRC.

Comment 3

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r=dragana

Review commit: https://reviewboard.mozilla.org/r/48235/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48235/

Comment 4

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r=dragana

Review commit: https://reviewboard.mozilla.org/r/48237/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48237/

Comment 5

[Jan Keromnes [:janx]]

Thank you Kit for addressing this so quickly!

I'm not the right person to fully review these patches, so I will forward them to :dragana.

However, I did try them. Both "push-message" and "push-subscription-change" events now seem to be forwarded in some case. \o/

However:

- "push-message" is not triggered when clicking on "Push" button in about:debugging > Workers (the Service Worker has to be running, and "Push" calls `ServiceWorkerManager.sendPushEvent(originAttributes, registration.scope)` in the same child process as the live worker)

- "push-subscription-change" is not triggered when going to about:preferences > Content > Notifications > Choose (remove a subscription, e.g. "goroost.com")

- "push-subscription-change" is not always triggered when switching between "Allow" and "Block" in push website's Security Icon > Permissions > Receive Notifications.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8744003 [details]
MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r=dragana

I don't feel comfortable reviewing these platform changes. Dragana, could you please have a look?

Comment 7

[Lina Butler [:lina]]

No problem; I hope this helps! Thanks for looking it over.

- I didn't change 'ServiceWorkerManager.sendPushMessage'; only 'nsIPushNotifier' fires 'nsIObserverService' notifications. These patches should make it possible to use the notifier from either process and have it work correctly, though. You could change the "Push" button to use 'nsIPushNotifier.notifyPush{WithData}', or I can look into modifying 'ServiceWorkerManager'.

- 'push-subscription-change' is odd: it only gets triggered when the permission is changed from "block" to "allow" (not "always ask", because service workers can't ask for permission). Otherwise, the worker would just try to subscribe again, and fail because permission isn't granted.

- That last one sounds like a bug. :-( Would you mind posting an example of where it doesn't fire?

For case 2, I think we can fire a different observer notification for you to use in Dev Tools. Maybe we can rename "push-unsubscribe" to "push-subscription-lost", and pass a reason as the subject.

Actually, now that I think about it...'nsIPushErrorReporter' already reports these reasons to the server for the Dev Dashboard. We can remove 'notifyUnsubscribe' and just alert XPCOM observers using the same path.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8744003 [details]
MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48235/diff/1-2/

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8744004 [details]
MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48237/diff/1-2/

Comment 10

[Lina Butler [:lina]]

I kept the "push-unsubscribe" notification, and set its subject to an `nsISupportsPRUint16` that's one of these: https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/dom/interfaces/push/nsIPushErrorReporter.idl#27-29

Comment 11

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1266433 - Clean up `nsIPushNotifier` static casts. r=dragana

Review commit: https://reviewboard.mozilla.org/r/48655/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48655/

Comment 12

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1266433 - Indicate push subscriptions created by privileged code. r=dragana

Review commit: https://reviewboard.mozilla.org/r/48657/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48657/

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8744004 [details]
MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48237/diff/2-3/

Comment 14

[Lina Butler [:lina]]

Thinking some more (maybe too much) about this...

- I think "push-subscription-lost" is a better name. "push-unsubscribe" isn't really accurate because Firefox also "expires" push subscriptions that exceed their quota, or have their permission revoked.

This distinction isn't important for service workers, because they can't resubscribe until `pushsubscriptionchange`, anyway. For privileged code, I think the difference is "you need to resubscribe because the server dropped this subscription" vs. "Firefox dropped this subscription for reasons".

Chrome code that uses Push (FxA, Sync, Hello, etc.) needs to handle the first case, but the second one is more informational for Dev Tools.

- We should add an `isSystemSubscription` attribute to nsIPushSubscription, too, so that your PushSubscriptionActor can ignore updates for chrome subscriptions.

Comment 15

[Dragana Damjanovic [:dragana]]

Comment on attachment 8744003 [details]
MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r=dragana

https://reviewboard.mozilla.org/r/48235/#review45493

looks good

Comment 16

[Dragana Damjanovic [:dragana]]

Comment on attachment 8744004 [details]
MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r=dragana

https://reviewboard.mozilla.org/r/48237/#review45497

Comment 17

[Dragana Damjanovic [:dragana]]

Comment on attachment 8744662 [details]
MozReview Request: Bug 1266433 - Clean up `nsIPushNotifier` static casts. r=dragana

https://reviewboard.mozilla.org/r/48655/#review45499

Comment 18

[Dragana Damjanovic [:dragana]]

Comment on attachment 8744663 [details]
MozReview Request: Bug 1266433 - Indicate push subscriptions created by privileged code. r=dragana

https://reviewboard.mozilla.org/r/48657/#review45501

Comment 19

[Lina Butler [:lina]]

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

Comment 20

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1266433 - Only load `FxAccountsPush` in the parent process. r?markh

Review commit: https://reviewboard.mozilla.org/r/48829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48829/

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8744003 [details]
MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48235/diff/2-3/

Comment 22

[Lina Butler [:lina]]

Comment on attachment 8744004 [details]
MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48237/diff/3-4/

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8744662 [details]
MozReview Request: Bug 1266433 - Clean up `nsIPushNotifier` static casts. r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48655/diff/1-2/

Comment 24

[Lina Butler [:lina]]

Comment on attachment 8744663 [details]
MozReview Request: Bug 1266433 - Indicate push subscriptions created by privileged code. r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48657/diff/1-2/

Comment 25

[Lina Butler [:lina]]

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

Comment 26

[Lina Butler [:lina]]

Mark, some context for you: we're now broadcasting push observer notifications to both processes, so the last patch makes sure we don't try to initialize FxAccountsPush in the child.

Another approach is to change `DoNotifyObservers()` so that it only creates the service in the parent. I used the process selector because the component manager already supports them.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8745096 [details]
MozReview Request: Bug 1266433 - Only load `FxAccountsPush` in the parent process. r?markh

https://reviewboard.mozilla.org/r/48829/#review45701

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c42b3c27892
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0ebf7dbe88
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79aac60e54b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe0436ac454
https://hg.mozilla.org/integration/mozilla-inbound/rev/a83e9aa58e02
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f40cd90686

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c42b3c27892
https://hg.mozilla.org/mozilla-central/rev/ec0ebf7dbe88
https://hg.mozilla.org/mozilla-central/rev/e79aac60e54b
https://hg.mozilla.org/mozilla-central/rev/bbe0436ac454
https://hg.mozilla.org/mozilla-central/rev/a83e9aa58e02
https://hg.mozilla.org/mozilla-central/rev/a6f40cd90686

Comment 30

[Jan Keromnes [:janx]]

The implemented solution works for me. Thanks!