863598 - SimplePush: Make PushService a real service (jsm)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

PushService is currently implemented as a component.

1) This causes the JS file to be loaded in every process that starts on B2G. PushService is about 40k and its imports add more text. Converting to a jsm loaded only in shell.js will result in memory reduction.
2) This is required for PushService to be able to use the AlarmService in its current state. (Filing follow up).

@dougt:
I am not inclined to agree about PushService leaks on desktop. On a recent m-c checkout, there were no increased leaks due to conversion to PushService. In addition nothing stands out specific to push in shutdown leak logs. Can you reproduce again?

Comment 1

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

Landing this will NOT affect firefox23 desktop since https://hg.mozilla.org/mozilla-central/rev/fc8267682725
disabled Push on non-b2g platforms.

Comment 2

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

Attachment: Make PushService a module

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 739510 [details] [diff] [review]
Make PushService a module

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

::: dom/push/src/PushService.js
@@ +427,5 @@
> +
> +    Services.obs.addObserver(this, "profile-change-teardown", false);
> +    Services.obs.addObserver(this, "network-interface-state-changed",
> +                             false);
> +    Services.obs.addObserver(this, "webapps-uninstall", false);

do we no longer have to worry about any of this?

Comment 4

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

Attachment: Make PushService a module - patch for b2g18

Comment 5

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

Turns out that PushService as a component is also created, and app-startup called, but it seems like gecko content processes don't receive final-ui-startup, even though they receive app-startup, which stops multiple push services from being created.

Comment 6

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

Attachment: Make PushService a module

Moved some of the shutdown code from bug 863599 here for more coherence.

Carrying r=dougt forward.

Comment 7

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

http://hg.mozilla.org/projects/birch/rev/ea6b923829e1

Comment 8

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

Attachment: Make PushService a module - patch for b2g18

Fix up changes against current b2g18.

Comment 9

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

Desktop does leak.
https://tbpl.mozilla.org/?tree=Try&rev=9023d1b1a8e0

No reason to backout since push is disabled in m-c, but need to track this down.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

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

Comment on attachment 744468 [details] [diff] [review]
Make PushService a module - patch for b2g18

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 822712
User impact if declined: 
No user impact. Reduces memory consumption and prevents per process loading of the Push Service. Required change to use Alarms API in Push (see bug 863732).

Testing completed: 
Yes. Operating with change on local devices for 2 weeks.

Risk to taking this patch (and alternatives if risky): 
Very low.

String or UUID changes made by this patch:
Removes a JS component.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/de3c1a9e486e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

And a bustage fix since b2g18 still packages PushService with the browser too.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/15cc47992c6e