871372 - back out bug 863599, find a better way to initialize the push service

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

See bug 863599 comment 14.

A few bad things:
a) PushService.jsm isn't actually used in browser.js, so there's no good reason to pollute that scope with the module's symbols. There are better places to trigger one-time browser-specific code (e.g. nsBrowserGlue)
b) implicit initialization as a side effect of import() is frowned upon for modules
c) it seems odd to me that this service is now only initialized for Firefox desktop

The best solution would be to put the initialization of the service somewhere in gecko. If you really need c), that's best controlled by a pref.

An intermediate solution that would be cleaner but still not ideal would be putting explicit initialization (e.g. a call to PushService.init()) in nsBrowserGlue (e.g. in its onWindowsRestored).

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Gavin,

Concerning point (c), PushService is also initialized on b2g (b2g/chrome/content/shell.js), but not on Android. In that context would it make sense to move it to Gecko (where in Gecko would the load occur?) based on the pref.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Also, which is a better way to unload a module. Have it register for profile-change-teardown or explicitly call uninit() in nsBrowserGlue._onProfileShutdown()?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to Nikhil Marathe from comment #1)
> Concerning point (c), PushService is also initialized on b2g
> (b2g/chrome/content/shell.js), but not on Android. In that context would it
> make sense to move it to Gecko (where in Gecko would the load occur?) based
> on the pref.

Yes.

(In reply to Nikhil Marathe from comment #2)
> Also, which is a better way to unload a module. Have it register for
> profile-change-teardown or explicitly call uninit() in
> nsBrowserGlue._onProfileShutdown()?

Why do you need to unload it explicitly?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> (In reply to Nikhil Marathe from comment #1)
> > Concerning point (c), PushService is also initialized on b2g
> > (b2g/chrome/content/shell.js), but not on Android. In that context would it
> > make sense to move it to Gecko (where in Gecko would the load occur?) based
> > on the pref.
> 
> Yes.

Would you happen to which file to load it in?

> 
> (In reply to Nikhil Marathe from comment #2)
> > Also, which is a better way to unload a module. Have it register for
> > profile-change-teardown or explicitly call uninit() in
> > nsBrowserGlue._onProfileShutdown()?
> 
> Why do you need to unload it explicitly?

I'm currently trying to fix some CC issues in some components that should eliminate this need.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Nikhil Marathe from comment #4)
> Would you happen to which file to load it in?

I'm not sure, it depends a lot on when you want it to be initializedg. I assume you just want "some time not too long after startup", but you otherwise want to avoid interfering with app startup. We don't have very clean ways to do that (bug 692420 was meant to investigate adding some).

You could write a small xpcom component registered to observe app-startup (like NavigatorPropertyHelper.js). On app-startup, the component could then register for final-ui-startup or (some other topic) and load/initialize the PushService then. It would be nice to add this to an existing such (C++) component, to avoid the overhead of an additional JS XPCOM component, but I don't know which would be best.

(PushService already has a final-ui-startup observer that attempts to call init(), but it seems to not be useful, given the call to PushService.init() in the top level.)

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> (In reply to Nikhil Marathe from comment #4)
> > Would you happen to which file to load it in?
> 
> I'm not sure, it depends a lot on when you want it to be initializedg. I
> assume you just want "some time not too long after startup", but you
> otherwise want to avoid interfering with app startup. We don't have very
> clean ways to do that (bug 692420 was meant to investigate adding some).
> 
> You could write a small xpcom component registered to observe app-startup
> (like NavigatorPropertyHelper.js). On app-startup, the component could then
> register for final-ui-startup or (some other topic) and load/initialize the
> PushService then. It would be nice to add this to an existing such (C++)
> component, to avoid the overhead of an additional JS XPCOM component, but I
> don't know which would be best.

All of PushService used to be a component, but this caused it to be launched in every process in B2G, which was not a good idea :) A Fx Desktop specific app-startup component that starts the PushService sounds like a good idea. I'll try it.

Is there anything wrong in loading PushService in nsBrowserGlue _onProfileStartup, after 'browser-ui-startup-complete' is dispatched.

> 
> (PushService already has a final-ui-startup observer that attempts to call
> init(), but it seems to not be useful, given the call to PushService.init()
> in the top level.)

This was legacy code and has been removed in a patch that will land soon.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Nikhil Marathe from comment #6)
> All of PushService used to be a component, but this caused it to be launched
> in every process in B2G, which was not a good idea :) A Fx Desktop specific
> app-startup component that starts the PushService sounds like a good idea.
> I'll try it.

The component would be used for startup only, you wouldn't actually need to retrieve it from content processes. The goal is to have a method of initializing the service that isn't specific to a particular app, so an Fx Desktop specific component isn't ideal.

> Is there anything wrong in loading PushService in nsBrowserGlue
> _onProfileStartup, after 'browser-ui-startup-complete' is dispatched.

As I mentioned in comment 0, this would certainly be better than the browser.js change, but still not ideal (because it continues to be app-specific for something that should be considered part of the platform).

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Use a component to load PushService.

This uses a component to load the service. Patch works on B2G too.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 752451 [details] [diff] [review]
Use a component to load PushService.

>diff --git a/dom/push/src/PushService.jsm b/dom/push/src/PushService.jsm

>+      case "xpcom-shutdown":
>+        this.uninit();

Can you add a comment (ideally a "FIXME" with a reference to a bug number) that explains why this is necessary?

>diff --git a/dom/push/src/StartPushService.js b/dom/push/src/StartPushService.js

>+this.StartPushService = function StartPushService() {
>+  this.wrappedJSObject = this;

You don't actually need this, do you?

>diff --git a/dom/push/src/StartPushService.manifest b/dom/push/src/StartPushService.manifest

>+# StartPushService.js (initialization on startup)
>+component {6672a34d-babb-41a2-9412-2819dbcc8d1c} StartPushService.js
>+contract @mozilla.org/dom/push/service;1 {6672a34d-babb-41a2-9412-2819dbcc8d1c}
>+category app-startup StartPushService @mozilla.org/dom/push/service;1

No need to add a separate manifest, you could just put this in the existing Push.manifest.

There is (or at least used to be) non-trivial memory overhead to adding another JS component (particularly one loaded at startup), so it's a shame that there isn't some other existing app-startup component that this could be added to.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #9)
> There is (or at least used to be) non-trivial memory overhead to adding
> another JS component (particularly one loaded at startup)

So it would be good for you to measure the memory impact this has, particularly for b2g.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 752451 [details] [diff] [review]
Use a component to load PushService.

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

StartPushService.js

nit: I do not like having file names that being with verbs.  Can this just be called PushService?

Do we not what to start this on mobile or on metro?

::: dom/push/src/StartPushService.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

This is kind of sexy, but not sure if the readability hit is worth it.

@@ +8,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +this.StartPushService = function StartPushService() {
> +  this.wrappedJSObject = this;

Why do you need this?

@@ +23,5 @@
> +      case "app-startup":
> +        Services.obs.addObserver(this, "final-ui-startup", true);
> +        break;
> +      case "final-ui-startup":
> +        // Startup PushService.jsm

Unneeded comment.

@@ +24,5 @@
> +        Services.obs.addObserver(this, "final-ui-startup", true);
> +        break;
> +      case "final-ui-startup":
> +        // Startup PushService.jsm
> +        if (!Services.prefs.getBoolPref("services.push.enabled"))

lets stick with the DOM coding style:

if (1) {
  stmt;
}

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Pushed with changes noted in comment 9 and comment 11.

This change means that Push is now included in Firefox Desktop builds, but prevented from starting by a pref (services.push.enabled). Bug 857464 tracks enabling.

The component to start the service is now called PushServiceLauncher.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7d9177c7ee

Comment 14

[Chris Peterson [:cpeterson]]

Comment on attachment 752451 [details] [diff] [review]
Use a component to load PushService.

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

::: dom/push/src/StartPushService.js
@@ +22,5 @@
> +    switch (topic) {
> +      case "app-startup":
> +        Services.obs.addObserver(this, "final-ui-startup", true);
> +        break;
> +      case "final-ui-startup":

Can you unregister your "app-startup" and "final-ui-startup" observers here? Their work is done.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Landed in https://hg.mozilla.org/mozilla-central/rev/ac7d9177c7ee

Don't know why this wasn't closed