SimplePush: Make PushService a real service (jsm)



6 years ago
6 years ago


(Reporter: nsm, Assigned: nsm)


Dependency tree / graph

Firefox Tracking Flags

(firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)


(Whiteboard: [fixed-in-birch])


(2 attachments, 2 obsolete attachments)

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).

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?
tracking-b2g18: --- → ?
Landing this will NOT affect firefox23 desktop since
disabled Push on non-b2g platforms.
No longer blocks: 863599
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?
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.
status-b2g18: --- → affected
tracking-b2g18: ? → +
Attachment #739510 - Flags: review?(doug.turner) → review+
Moved some of the shutdown code from bug 863599 here for more coherence.

Carrying r=dougt forward.
Attachment #739510 - Attachment is obsolete: true
Attachment #744462 - Flags: review+
Fix up changes against current b2g18.
Attachment #741140 - Attachment is obsolete: true
Attachment #741140 - Flags: review?(doug.turner)
Attachment #744468 - Flags: review+
Desktop does leak.

No reason to backout since push is disabled in m-c, but need to track this down.
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
Attachment #744468 - Flags: approval-mozilla-b2g18?
Attachment #744468 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
And a bustage fix since b2g18 still packages PushService with the browser too.
You need to log in before you can comment on or make changes to this bug.