1222097 - openWindow: path resolution works differently in Firefox vs Chrome

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Marco Castelluccio [:marco]]

For example:
https://localhost:3003/push-clients/service-worker.js
clients.openWindow('./push-clients/index.html');
in Firefox opens https://localhost:3003/push-clients/index.html
in Chrome opens https://localhost:3003/push-clients/push-clients/index.html

Comment 1

[Marco Castelluccio [:marco]]

To reproduce:
- clone https://github.com/marco-c/serviceworker-cookbook/tree/push_notification_clients
- run 'npm install'
- run 'gulp start-server'
- open 'http://localhost:3003/push-clients/index.html'

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Yes, this is a bug in FF.  The spec says:

  "Let url be the result of parsing url with entry settings object's API base URL."

Here the entry settings object is for the ServiceWorker thread, so it should be the scriptURL.

But our code just uses the origin:

  https://dxr.mozilla.org/mozilla-central/rev/cc48981c026c50fdf80d47b040ae1fb8fe99ad07/dom/workers/ServiceWorkerClients.cpp#427

I think we just need to change info.mOrigin to info.mHref on that line.

Comment 3

[Marco Castelluccio [:marco]]

Seems simple, I'll take a stab at it.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Marco Castelluccio [:marco] from comment #3)
> Seems simple, I'll take a stab at it.

Thanks!  It would be great to have a wpt test for it as well.  :-)

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Test

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8684289 [details] [diff] [review]
Test

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

Looks good!  You verified this fails without the P1 patch?

Comment 8

[Marco Castelluccio [:marco]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bcd97ead0f5

Comment 9

[Marco Castelluccio [:marco]]

> Looks good!  You verified this fails without the P1 patch?

Yes, it opens http://mochi.test:8888/open_window/client.html instead of http://mochi.test:8888/tests/dom/workers/test/serviceworkers/open_window/client.html.

Comment 10

[Marco Castelluccio [:marco]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f79172bd7a

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Test v2

I've had to update the expected number of opened windows. Carrying r+ since it's a straightforward change.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebf32f639a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f8165c1152

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

Requesting uplift since this fix is important for push notifications which I believe we want to ship in 44.

This request is for both patches.

Approval Request Comment
[Feature/regressing bug #]: ServiceWorkers and push notifications
[User impact if declined]: Sites using openWindow() from push notifications with a relative path may not work.
[Describe test coverage new/current, TreeHerder]: Includes mochitests
[Risks and why]: Minimal.  Only affects service worker based openWindow().
[String/UUID change made/needed]: None

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7ebf32f639a9
https://hg.mozilla.org/mozilla-central/rev/87f8165c1152

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Seems safe enough. Let's uplift to Aurora44.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Marco, will you be landing the mochitest on Aurora as well? I think that would be very helpful to catch regressions.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

I actually requested the uplift here, not Marco.  Yes, please uplift both patches.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/86a326cd5146
https://hg.mozilla.org/releases/mozilla-aurora/rev/b83e637429b0

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/86a326cd5146
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b83e637429b0