1252290 - Provide a way for browser code to reliably receive push notifications without impacting browser startup time

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

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

Currently browser code that wishes to receive push notifications must listen for observer notifications with a topic of "push-message". For reliable receipt of such messages, the browser code must register an observer before the push service initializes, or the notifications may be sent before a listener is set up.

Currently the push service initializes very soon after startup. However, lots of browser code explicitly attempts to avoid early initialization so that Firefox startup time is not impacted. As more browser code starts to leverage push, we are likely to find more and more code that is forced to initialize early, just to avoid missing push notifications. We should attempt to address this by providing a facility for the push service itself to force this initialization only when a relevant notification (ie, push message, subscription change, etc) needs to be delivered.

A straw-man:
* browser code would register an xpcom component with the category manager (eg, category=”push”, entry={push_scope}.
* Before the push manager notifies observers, it attempt to instantiate an xpcom service from the category manager. Under the assumption that referring to already instantiated services is fast, and failing to find a matching service is fast, it might be possible to do this on every message (ie, not bother with tracking if the service has already attempted this instantiation)
* The implementation of the xpcom service must attach listeners as it initializes. Thus there would be no new interfaces that need implementation - the initialization of the service is assumed to do all necessary work.

Comment 1

[Andrew Overholt [:overholt]]

William, what do you think?

Comment 2

[Chris Karlof [:ckarlof]]

Andrew, a little more context: this work is important for our 2016H1 Sync and FxA initiatives. One of our goals is to reduce end-to-end latency of data syncing and Push is how we're going to do it. Addressing this issue is part of making that a quality experience. Please let me know if you'd like further information.

Comment 3

[Lina Butler [:lina]]

Implementation-wise, we could outfit `PushNotifier.cpp` to use the category manager, instead of broadcasting via the observer service (similar to https://dxr.mozilla.org/mozilla-central/source/embedding/components/appstartup/nsAppStartupNotifier.cpp). As long as the consumer implemented `nsIObserver`, I think we could just call its `Observe` method, without requiring its constructor to listen for the two events. A nice side effect is that we wouldn't need to pass the scope as the data param.

Earlier, I was thinking we could automatically create a subscription for the scope listed in the manifest, if it doesn't exist. But that seems too magical, and the consumer still needs to send the endpoint URL back to its server...so that doesn't give us anything.

This is all assuming the category manager is the right approach, of course. :-)

Comment 4

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

(In reply to Kit Cambridge [:kitcambridge] from comment #3)
> Implementation-wise, we could outfit `PushNotifier.cpp` to use the category
> manager, instead of broadcasting via the observer service (similar to
> https://dxr.mozilla.org/mozilla-central/source/embedding/components/
> appstartup/nsAppStartupNotifier.cpp). As long as the consumer implemented
> `nsIObserver`, I think we could just call its `Observe` method, without
> requiring its constructor to listen for the two events.

That sounds reasonable - although I wonder if it adds much value? I assume we'd want to support the observer service as well (ie, to not force push consumers into this xpcom-service based model) so it seems we'd just be duplicating c++ code (ie, sending the notification directly to a service and to the observer service) all to save exactly addObserver call by the service.

> A nice side effect
> is that we wouldn't need to pass the scope as the data param.

That would probably prevent one service handling multiple scopes? I think it's possible a single service would register against multiple categories (ie, against multiple scopes)

> This is all assuming the category manager is the right approach, of course.
> :-)

I think we should just run a patch up the flagpole and see who salutes :)

Comment 5

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #4)
> That sounds reasonable - although I wonder if it adds much value? I assume
> we'd want to support the observer service as well (ie, to not force push
> consumers into this xpcom-service based model) so it seems we'd just be
> duplicating c++ code (ie, sending the notification directly to a service and
> to the observer service) all to save exactly addObserver call by the service.

Oh, I see. I was thinking we'd replace the observers and force consumers into the service model...but that does make it awkward, particularly for C++ callers. Please ignore everything I said in my comment. :-)

Comment 6

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

Attachment: 0001-Bug-1252290-experiment-with-creating-an-xpcom-servic.patch

I think a change like this is all the push service itself would need to do. It obviously needs tests, which I'd be happy to work on if we agree this general approach is OK.

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8726094 [details] [diff] [review]
0001-Bug-1252290-experiment-with-creating-an-xpcom-servic.patch

Review of attachment 8726094 [details] [diff] [review]:
-----------------------------------------------------------------

Nice and simple; I like it!

::: dom/push/PushNotifier.cpp
@@ +197,5 @@
> +  return DoNotifyObservers(nullptr, "push-subscription-change", aScope);
> +}
> +
> +nsresult
> +PushNotifier::DoNotifyObservers(nsISupports *subject, const char *topic,

Nit: How do you feel about just `NotifyObservers`?

@@ +214,5 @@
> +    nsresult rv = catMan->GetCategoryEntry("push",
> +                                           PromiseFlatCString(aScope).get(),
> +                                           getter_Copies(contractId));
> +    if (NS_SUCCEEDED(rv)) {
> +      do_GetService(contractId);

Is there any chance this call could be optimized out? I wonder if we could use the `do_GetService(contractId, &rv)` form, and warn if the constructor throws. But maybe that's totally unnecessary.

A comment that we're initializing the service as a side effect, and that its constructor should add the observers, would be lovely.

Comment 8

[William Chen [:wchen]]

Comment on attachment 8726094 [details] [diff] [review]
0001-Bug-1252290-experiment-with-creating-an-xpcom-servic.patch

Review of attachment 8726094 [details] [diff] [review]:
-----------------------------------------------------------------

Approach looks good to me.

Comment 9

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

(In reply to William Chen [:wchen] from comment #8)
> Approach looks good to me.

Thanks William.

Comment 10

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

Attachment: 0001-Bug-1252290-load-xpcom-services-registered-with-the-.patch

Same basic patch, but using the new constants for the topics and including tests.

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8726529 [details] [diff] [review]
0001-Bug-1252290-load-xpcom-services-registered-with-the-.patch

Review of attachment 8726529 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks, Mark!

::: dom/push/PushNotifier.cpp
@@ +198,5 @@
> +}
> +
> +nsresult
> +PushNotifier::DoNotifyObservers(nsISupports *subject, const char *topic,
> +                  const nsACString& aScope)

Style nit: aArgs, indentation.

Comment 12

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

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fd095cbb65ad

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd095cbb65ad