1307577 - Punycode sites do not show push notifications after successful subscription

Status: RESOLVED FIXED

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

Description

[jasonpang2011]

Attachment: FirefoxAndroid49PushServicePunycodeLog

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

1. Followed the section on AutoPush debugging to enable browser logging for Mozilla's push service: http://autopush.readthedocs.io/en/latest/testing.html#debugging.

2. Open the Browser Console (Mac: Firefox toolbar -> Tools -> Web Developer -> Browser Console. Windows: Three-menu bar options -> Developer -> Web Developer -> Browser Console).

3. Visit https://xn--90aexm.xn--80ag3aejvc.xn--p1ai.

4. Click the red subscription bell on the bottom right to get prompted to subscribe to notifications.

5. Allow notification permissions, and observe the output in the browser console. You should see something like: https://i.imgur.com/hek50Ox.png with "missing push permission" as the message. A welcome push notification would normally be shown, but isn't visible. You can send notifications to yourself by opening the Developer Tools Console for the site and typing "OneSignal.sendSelfNotification();".

-------

Other working site: Visit https://onesignal.com/webpush and click "Click to see an example". You should receive a push notification (to compare the browser console push service outputs). Here is the browser console output for this non-Punycode site: http://i.imgur.com/Neb60oz.png

Tested on:
- Firefox Desktop 49 on Mac OS X 10.11
- Firefox Desktop 49 on Windows 10
- Firefox on Android 49 on Android 7 Nougat

Firefox on Android 49's error message is a little different saying "Message: Error: Ignoring message sent to unvisited origin". Full log of subscription and attempted message receipt attached for Firefox on Android.




Actual results:

The push service delivered the message to Firefox but there was an error "missing push permission" and the notification was not displayed.


Expected results:

The push notification should have been delivered to the service worker and a push notification should have been displayed.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hmmm I couldn't even subscribe successfully... 
Maybe Kit has some ideas about what's going on in comment 0. :)

Comment 2

[Lina Butler [:lina]]

Thanks for the report, Jason and Hsin-Yi! I'm sorry this slid off my radar. This is an interesting one: `createCodebasePrincipalFromOrigin` mangles the origin when it parses out the suffix. It works if I create the URI separately and pass it to `createCodebasePrincipal`, though.

The right fix is to stop reconstructing the principal, and just store the one we get when the page subscribes. I started working on that, but the patch is involved (lots of `db.put` calls to convert). Putting up a simpler patch in the meantime.

Comment 3

[Lina Butler [:lina]]

Attachment: Bug 1307577 - Switch to `createCodebasePrincipal` in `PushRecord`.

`createCodebasePrincipalFromOrigin` mangles the origin after extracting
the suffix. The proper long-term fix is to serialize and store the
original principal, instead of reconstructing a new codebase principal
from the scope and origin suffix.

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

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8803007 [details]
Bug 1307577 - Switch to `createCodebasePrincipal` in `PushRecord`.

https://reviewboard.mozilla.org/r/87248/#review86378

::: dom/push/test/xpcshell/test_record.js:85
(Diff revision 1)
> +    scope: 'https://example.com/',
> +  }, {
> +    scope: 'https://example.com/',
> +    originAttributes: '^userContextId=1',
> +  }, {
> +    scope: 'https://блог.фанфрог.рф/',

did you intend to use this name in the next test also?

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8803007 [details]
Bug 1307577 - Switch to `createCodebasePrincipal` in `PushRecord`.

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

Comment 6

[Lina Butler [:lina]]

This patch should fix the issue on Desktop. Keeping this bug open until I can test on my Nexus 7 tomorrow, because I don't think we run *any* push on tests on Android. :-O Bug 1214362 is for mochitests; I can't find one for xpcshell.

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8803007 [details]
Bug 1307577 - Switch to `createCodebasePrincipal` in `PushRecord`.

https://reviewboard.mozilla.org/r/87248/#review86378

> did you intend to use this name in the next test also?

I did.

Comment 8

[Lina Butler [:lina]]

(In reply to Kit Cambridge [:kitcambridge] from comment #6)
> Bug 1214362 is for mochitests; I can't find one for
> xpcshell.

Filed bug 1311849.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f1273f17e6f
Switch to `createCodebasePrincipal` in `PushRecord`. r=mt

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f1273f17e6f

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Mass wontfix for bugs affecting firefox 52.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Kit, did you get any luck of testing on nexus7 as comment 6 planned? Anything left here before we could close this one?

Comment 13

[Lina Butler [:lina]]

Attachment: android-push-success.png

Apologies for the delay, Hsin-Yi, I've been heads-down with other work. Verified that Android works as expected, so yes, we can close this. Thanks for following up!

Comment 14

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.