Closed Bug 1222097 Opened 5 years ago Closed 4 years ago

openWindow: path resolution works differently in Firefox vs Chrome

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(2 files, 1 obsolete file)

Blocks: WADI
Blocks: 1201571
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.  :-)
Attached patch PatchSplinter Review
Attachment #8684288 - Flags: review?(bkelly)
Attached patch Test (obsolete) — Splinter Review
Attachment #8684289 - Flags: review?(bkelly)
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.
Attached patch Test v2Splinter Review
I've had to update the expected number of opened windows. Carrying r+ since it's a straightforward change.
Attachment #8684289 - Attachment is obsolete: true
Attachment #8684382 - Flags: review+
Keywords: checkin-needed
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.
Flags: needinfo?(mar.castelluccio)
I actually requested the uplift here, not Marco.  Yes, please uplift both patches.
Flags: needinfo?(mar.castelluccio)
You need to log in before you can comment on or make changes to this bug.