Closed Bug 1266433 Opened 3 years ago Closed 3 years ago

Always notify observers about push subscription changes

Categories

(Core :: DOM: Push Notifications, defect)

47 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: janx, Assigned: lina)

References

Details

(Whiteboard: btpp-active)

Attachments

(5 files)

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
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?
Flags: needinfo?(kcambridge)
Kit, assigning to you as requested on IRC.
Assignee: nobody → kcambridge
Flags: needinfo?(kcambridge)
Status: NEW → ASSIGNED
Whiteboard: btpp-active
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.
Flags: needinfo?(kcambridge)
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?
Attachment #8744003 - Flags: review?(janx)
Attachment #8744003 - Flags: review?(dd.mozilla)
Attachment #8744003 - Flags: feedback+
Attachment #8744004 - Flags: review?(janx)
Attachment #8744004 - Flags: review?(dd.mozilla)
Attachment #8744004 - Flags: feedback+
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.
Flags: needinfo?(kcambridge)
Flags: needinfo?(janx)
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/
Attachment #8744003 - Attachment description: MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. r?janx → MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r?dragana
Attachment #8744004 - Attachment description: MozReview Request: Bug 1266433 - Send an observer notification when a service worker unsubscribes from Push. r?janx → MozReview Request: Bug 1266433 - Send an observer notification when a service worker unsubscribes from Push. f=janx r?dragana
Attachment #8744003 - Flags: feedback+
Attachment #8744004 - Flags: feedback+
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/
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
Review commit: https://reviewboard.mozilla.org/r/48655/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48655/
Attachment #8744004 - Attachment description: MozReview Request: Bug 1266433 - Send an observer notification when a service worker unsubscribes from Push. f=janx r?dragana → MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r?dragana
Attachment #8744662 - Flags: review?(dd.mozilla)
Attachment #8744663 - Flags: review?(dd.mozilla)
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/
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 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
Attachment #8744003 - Flags: review?(dd.mozilla) → review+
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
Attachment #8744004 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8744662 [details]
MozReview Request: Bug 1266433 - Clean up `nsIPushNotifier` static casts. r=dragana

https://reviewboard.mozilla.org/r/48655/#review45499
Attachment #8744662 - Flags: review?(dd.mozilla) → review+
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
Attachment #8744663 - Flags: review?(dd.mozilla) → review+
Review commit: https://reviewboard.mozilla.org/r/48829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48829/
Attachment #8744003 - Attachment description: MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r?dragana → MozReview Request: Bug 1266433 - Send Push observer notifications to parent and content processes. f=janx r=dragana
Attachment #8744004 - Attachment description: MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r?dragana → MozReview Request: Bug 1266433 - Send an observer notification when a push subscription is lost. f=janx r=dragana
Attachment #8744662 - Attachment description: MozReview Request: Bug 1266433 - Clean up `nsIPushNotifier` static casts. r?dragana → MozReview Request: Bug 1266433 - Clean up `nsIPushNotifier` static casts. r=dragana
Attachment #8744663 - Attachment description: MozReview Request: Bug 1266433 - Indicate push subscriptions created by privileged code. r?dragana → MozReview Request: Bug 1266433 - Indicate push subscriptions created by privileged code. r=dragana
Attachment #8745096 - Flags: review?(markh)
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 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 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 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/
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.
Attachment #8745096 - Flags: review?(markh) → review+
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
Target Milestone: mozilla48 → mozilla49
Blocks: 1269385
The implemented solution works for me. Thanks!
Flags: needinfo?(janx)
You need to log in before you can comment on or make changes to this bug.