Closed Bug 1222097 Opened 5 years ago Closed 4 years ago
Window: path resolution works differently in Firefox vs Chrome
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
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'
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.
Seems simple, I'll take a stab at it.
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
(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. :-)
Attachment #8684288 - Flags: review?(bkelly) → review+
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?
Attachment #8684289 - Flags: review?(bkelly) → review+
> 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.
I've had to update the expected number of opened windows. Carrying r+ since it's a straightforward change.
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
Attachment #8684288 - Flags: approval-mozilla-aurora?
Comment on attachment 8684288 [details] [diff] [review] Patch Seems safe enough. Let's uplift to Aurora44.
Attachment #8684288 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marco, will you be landing the mochitest on Aurora as well? I think that would be very helpful to catch regressions.
I actually requested the uplift here, not Marco. Yes, please uplift both patches.
You need to log in before you can comment on or make changes to this bug.