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

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect, P2)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

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 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1467919 - Don't initialize PushService unless dom.push.enabled is true

Review commit: https://reviewboard.mozilla.org/r/250488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250488/

Comment 2

[Lina Butler [:lina]]

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.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8984607 [details]
Bug 1467919 - Don't initialize PushService unless dom.push.enabled is true

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250488/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04f93535a93
Don't initialize PushService unless dom.push.enabled is true r=lina

Comment 5

[Narcis Beleuzu [:NarcisB]]

Backed out for xpcshell failures

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5f39774d8f1f37814824016ee716846edae4df5
Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f55e78d6593656aff9f61a684bfbc7ba1f3ce264
Log link1: https://treeherder.mozilla.org/logviewer.html#?job_id=185433695&repo=mozilla-inbound&lineNumber=1885
Log link2: https://treeherder.mozilla.org/logviewer.html#?job_id=185430765&repo=mozilla-inbound&lineNumber=2065

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1467919 - Don't try to clear PushService data if Push disabled

Review commit: https://reviewboard.mozilla.org/r/253994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253994/

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8984607 [details]
Bug 1467919 - Don't initialize PushService unless dom.push.enabled is true

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250488/diff/2-3/

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8988839 [details]
Bug 1467919 - Don't try to clear PushService data if Push disabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253994/diff/1-2/

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8988839 [details]
Bug 1467919 - Don't try to clear PushService data if Push disabled

https://reviewboard.mozilla.org/r/253994/#review261052

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/22a25d017920
https://hg.mozilla.org/mozilla-central/rev/3e55c9eebbbc

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/22a25d017920
https://hg.mozilla.org/mozilla-central/rev/3e55c9eebbbc

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d9b44bd2d74
https://hg.mozilla.org/releases/mozilla-beta/rev/3c8ab5a011e3