Don't initialize PushService if dom.push.enabled=false

RESOLVED FIXED in Firefox 62

Status

()

defect
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: snorp, Assigned: snorp)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(2 attachments)

The Android-related PushService doesn't work right now with GeckoView. Initializing it hangs forever because it's waiting for responses that will never come. We should avoid this path until we actually support Web Push on GeckoView. The most straightforward way for this seems to be to set dom.push.enabled to false, but we appear to initialize PushService.jsm in PushComponents.js anyway.
Comment on attachment 8984607 [details]
Bug 1467919 - Don't initialize PushService unless dom.push.enabled is true

https://reviewboard.mozilla.org/r/250488/#review256756

Thanks, Snorp! I think this is a good enough workaround for now, but I have a suggestion about where to check for `this.service` instead. r=lina with that fixed.

On the one hand, this makes it a bit awkward to toggle the pref at runtime. On the other, that's probably complexity that we don't need, anyway, and lifting all the `.enabled` pref handling out of `PushService.jsm`, and into `PushComponents.js` is a big chunk of work.

::: dom/push/PushComponents.js:24
(Diff revision 1)
> +  if (Services.prefs.getBoolPref("dom.push.enabled")) {
> -  const {PushService} = ChromeUtils.import("resource://gre/modules/PushService.jsm",
> +    const {PushService} = ChromeUtils.import("resource://gre/modules/PushService.jsm",
> -                                           {});
> +                                            {});
> -  PushService.init();
> +    PushService.init();
> -  return PushService;
> +    return PushService;
> +  } else {

Nit: No need for `else` after return.

::: dom/push/PushComponents.js:258
(Diff revision 1)
>        ChromeUtils.originAttributesToSuffix(principal.originAttributes);
>  
>      return data;
>    },
>  
>    _handleRequest(name, principal, data) {

How about making this function `async`, so that exceptions get transformed into rejections, then checking if `PushService` exists, and throwing something like `NS_ERROR_NOT_AVAILABLE` in https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/dom/push/PushComponents.js#299 instead of here?

That should give us slightly better errors for the synchronous push service methods, too.
Attachment #8984607 - Flags: review+
Assignee: nobody → snorp
Status: NEW → ASSIGNED
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04f93535a93
Don't initialize PushService unless dom.push.enabled is true r=lina
Priority: -- → P2
Comment on attachment 8988839 [details]
Bug 1467919 - Don't try to clear PushService data if Push disabled

https://reviewboard.mozilla.org/r/253994/#review261052
Attachment #8988839 - Flags: review?(amarchesini) → review+
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22a25d017920
Don't initialize PushService unless dom.push.enabled is true r=lina
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e55c9eebbbc
Don't try to clear PushService data if Push disabled r=baku
Flags: needinfo?(snorp)
https://hg.mozilla.org/mozilla-central/rev/22a25d017920
https://hg.mozilla.org/mozilla-central/rev/3e55c9eebbbc
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8984607 [details]
Bug 1467919 - Don't initialize PushService unless dom.push.enabled is true

Approval Request Comment
[Feature/Bug causing the regression]: GeckoView
[User impact if declined]: Gecko will hang when content uses a Service Worker
[Is this code covered by automated tests?]: Partially
[Has the fix been verified in Nightly?]: Yes, via GeckoViewExample
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: Both patches in this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Mostly only affects GeckoView
[String changes made/needed]: None
Attachment #8984607 - Flags: approval-mozilla-beta?
Comment on attachment 8984607 [details]
Bug 1467919 - Don't initialize PushService unless dom.push.enabled is true

Needed in order to disable web push in bug 1467921. 
This should land in beta 7.
Attachment #8984607 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.