863599 - SimplePush: Enable PushService on desktop/android

Status: RESOLVED DUPLICATE of bug 871372

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

Description

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

PushService.jsm (once 863598 lands) should be included in browser/base/content/browser.js based on "services.push.enabled" being true.

Add a uninit function to PushService that will remove observers.

This is a simple first bug.

Comment 1

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

Attachment: Patch

Comment 2

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

This patch does not enable push because the preference is still disabled. Patch for bug 857464 does that.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 743572 [details] [diff] [review]
Patch

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

::: browser/base/content/browser.js
@@ +1361,5 @@
>        OfflineApps.uninit();
>        IndexedDBPromptHelper.uninit();
>        AddonManager.removeAddonListener(AddonsMgrListener);
>        SocialUI.uninit();
> +      PushService.uninit();

Is this executed each time a browser window is closed, or only when the last browser window is closed?

Comment 4

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

Attachment: Patch

@flo: thanks for that, that was indeed running on closing a window.

Turns out the code already watched profile-change-teardown, so I merged uninit() into _shutdown(), which now closes down the database and cancels timers and all sockets.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 744159 [details] [diff] [review]
Patch

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

::: browser/base/content/browser.js
@@ +7,5 @@
>  let Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource:///modules/RecentWindow.jsm");
> +Cu.import("resource://gre/modules/PushService.jsm");

make sure you don't leak anything.

::: dom/push/src/PushService.jsm
@@ +496,5 @@
> +    if (this._retryTimeoutTimer)
> +      this._retryTimeoutTimer.cancel();
> +
> +    if (this._requestTimeoutTimer)
> +      this._requestTimeoutTimer.cancel();

isn't the style :

if (t) {
  stmt;
}

Comment 6

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

Will try complain if the code leaks? I can't find any push specific leaks on local builds.

Comment 7

[Doug Turner (:dougt)]

yup, it should.

Comment 8

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

Attachment: Patch

Moved some shutdown code into bug 863598 for more coherence.

carrying forward r=dougt.

Comment 9

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

land once bug 863598 makes its way to m-c.

Comment 10

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

Don't land until leak is fixed (see bug 863598 comment 9).

Comment 11

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

Try run with nothing going wrong - https://tbpl.mozilla.org/?tree=Try&rev=d1cb326a55a3

Comment 12

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

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

Comment 13

[David Baron :dbaron:]

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

Comment 14

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

browser.js is part of the Firefox module, and so you need to get Firefox peer review before changing it.

I don't think this is the right way to initialize the service. There's no need to pollute the Firefox window scope with PushService.jsm's symbol, and having import() cause side-effects (triggering initialization of the service) is also generally frowned upon. It seems odd to want to have this be Firefox-desktop-specific to begin with, but if that's really what you want then having the initialization occur (explicitly) in nsBrowserGlue is probably a better idea.

Comment 15

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

I filed bug 871372.

Comment 16

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

Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f4da66d8655

Comment 17

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

The patch in bug 871372 implements this.

Comment 18

[Ed Morley [:emorley]]

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