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?

Kit, assigning to you as requested on IRC.

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

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

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

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 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 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/

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

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

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 on attachment 8744662 [details] MozReview Request: Bug 1266433 - Clean up `nsIPushNotifier` static casts. r=dragana https://reviewboard.mozilla.org/r/48655/#review45499

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

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

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.

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

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

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

The implemented solution works for me. Thanks!