URL Fragment Identifiers Break Service Worker Responses

RESOLVED FIXED in Firefox 59

Status

()

Core
DOM: Service Workers
P1
normal
RESOLVED FIXED
a month ago
22 days ago

People

(Reporter: chris, Assigned: bkelly)

Tracking

(Blocks: 1 bug, {regression})

59 Branch
mozilla61
Unspecified
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(relnote-firefox 59+, firefox-esr52 disabled, firefox59+ fixed, firefox60+ fixed, firefox61 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

Steps to reproduce:

Test page:
https://fetch-progress.anthum.com/test/sw-response-with-url-fragments/

URLs containing hash/fragment values (e.g. "image.png#value") prevent Service Workers from responding correctly when new Response() constructor is used.  This bug does not occur in FF 58 and was introduced in FF 59.

History of bug discovery:
https://github.com/AnthumChris/fetch-progress-indicators/issues/9


Actual results:

Images with URL fragment values do not display when intercepted by Service Worker


Expected results:

Images with URL fragment values should display.
(Reporter)

Comment 1

a month ago
Created attachment 8957119 [details]
FF 59 Image Load Error
Component: Untriaged → DOM: Service Workers
Product: Firefox → Core
OS: Unspecified → Mac OS X
Hi Chris, thanks for filing the bug.

I was able to reproduce this with the following specs:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv: 59.0) Gecko/20100101 Firefox 59.0
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

a month ago
Assignee: nobody → bugmail
status-firefox59: --- → affected
Priority: -- → P1

Comment 4

a month ago
Andrew, please take a look.
Flags: needinfo?(bugmail)
Thank you for your most excellent test case, chris!  I can confirm locally that (modulo the streams) things work in 58 and fail in 59 for the Blob response with fragment on the request.

In bug 1437760 (which was uplifted to 59) we recently changed to propagate the #value3 hash from the request onto the response.  The network inspector and HAR file show that the contents of the SVG are correctly propagated, so this is likely due to InternalResponse::GetUnfilteredURL/GetURL() returning an EmptyCString() for synthetic responses, which we blindly append the request's fragment to at https://searchfox.org/mozilla-central/source/dom/serviceworkers/ServiceWorkerEvents.cpp#723 (from the change at https://hg.mozilla.org/mozilla-central/rev/c6b9c2cb220d#l1.58).  Presumably some code freaks out when it sees a URL that's just a fragment.

Investigating further to prepare a fix and improve test coverage.
Depends on: 1437760
The specific path of breakage is:
- The responseURL in RespondWithHandler::ResolvedCallback() ends up being "#value" as a result of our propagation.  (Note that in comment 5 I said "#value3", but that was a typo; the 3 is before the hash in the image src of "sw-image.svg?blob-response3#value".)  Also note that at least that in my testing under nightly with e10s and child intercept, no entry will be found in the network inspector.  I may have been mistaken in comment 5, or it may be the case that in beta=59 (and I may have been non-e10s), it may show up.
- We end up in mozilla::net::InterceptedChannelContent::StartSynthesizedResponse from the ServiceWorkersEvents.cpp StartResponse runnable's Run method.
- We end up in the code block at https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/netwerk/protocol/http/InterceptedChannel.cpp#312 that looks like:
  if (!aFinalURLSpec.IsEmpty()) {
    nsresult rv = NS_NewURI(getter_AddRefs(responseURI), aFinalURLSpec);
- In the 2 successful blob specs, aFinalURLSpec was empty.
  - In the first case of "sw-image.svg?blob-response1", there was no "#", so no propagation happened.
  - In the second case of "sw-image.svg?blob-response2#", although there was a "#", our logic only fires for propagation if the fragment was non-empty.  We end up dropping the # which is a loss of information but is not currently the end of the world  because:
    - For HTML, per https://html.spec.whatwg.org/multipage/browsing-the-web.html#the-indicated-part-of-the-document just "#" does "indicate" the top of the document, but per https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-fragid the "target element" gets set to null in that case, the same as if there was no indicated part of the document.  This means CSS selectors can't get at it to break.  It would matter if we were navigating, but we explicitly don't propagate the URL or fragment if we're not navigating, so that's right out.
    - I'm less sure of what's going on with SVG, but for SVG1.1, https://www.w3.org/TR/SVG11/linking.html would seem to categorize "#" as an invalid identifier.  Similarly, SVG2 (draft) at https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiers seems to suggest SVG is cribbing from HTML4/earlier semantics.
- Here in the bad case where aFinalURLSpec is "#value" (which is not a legal URL on its own), NS_NewURI returns nsresult::NS_ERROR_MALFORMED_URI.
- That error results in InterceptedChannelContent::CancelInterception() and thereby HttpChannelChild::Cancel() and AsyncAbort() being invoked, canceling the channel and resulting in the broken image.
(Assignee)

Updated

a month ago
Blocks: 1226983
status-firefox60: --- → affected
status-firefox61: --- → affected
status-firefox-esr52: --- → disabled
(Assignee)

Comment 7

a month ago
Thanks for investigating while I was out Andrew!  I'll try to figure out a fix here.
Assignee: bugmail → bkelly
Flags: needinfo?(bugmail)
(Assignee)

Updated

a month ago
Keywords: regression
(Assignee)

Comment 8

a month ago
Created attachment 8960340 [details] [diff] [review]
P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth

This fixes the problem locally in the comment 0 demo.  Now I need to add a WPT test somewhere...
(Assignee)

Updated

a month ago
Attachment #8960340 - Attachment description: 1443850 P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth → P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth
(Assignee)

Comment 9

a month ago
Created attachment 8960353 [details] [diff] [review]
P2 Add a WPT test verifying synthetic responses work when a request fragment is present. r=asuth
(Assignee)

Comment 11

a month ago
Comment on attachment 8960340 [details] [diff] [review]
P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth

Andrew, this fixes the failure highlighted in comment 0 using your suggested fix.  It does not handle the existing-but-empty fragment case.  I figure we can tackle that once Anne sorts out the fragment spec handling a bit more.
Attachment #8960340 - Flags: review?(bugmail)
(Assignee)

Comment 12

a month ago
Comment on attachment 8960353 [details] [diff] [review]
P2 Add a WPT test verifying synthetic responses work when a request fragment is present. r=asuth

This patch adds a test case to fetch-event.https.html that performs a fetch() using a fragment request which is then intercepted with a synthetic response.  This fails on trunk currently and passes with the P1 patch.
Attachment #8960353 - Flags: review?(bugmail)
Attachment #8960340 - Flags: review?(bugmail) → review+
Comment on attachment 8960353 [details] [diff] [review]
P2 Add a WPT test verifying synthetic responses work when a request fragment is present. r=asuth

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

Restating: Adds a fetch test case that responds with a synthetic String via `new Response('Test string')` which is different from the Blob case from comment 0, but functionally equivalent for our breakage here.
Attachment #8960353 - Flags: review?(bugmail) → review+
(Reporter)

Comment 14

a month ago
Thank you everyone for your attention to this bug.  Long live the URL fragment...
(Assignee)

Comment 16

a month ago
Please see the try build in comment 15.  Thanks.
Keywords: checkin-needed

Comment 17

a month ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a17d9a6aee91
P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b66eb162975
P2 Add a WPT test verifying synthetic responses work when a request fragment is present. r=asuth
Keywords: checkin-needed

Comment 18

29 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a17d9a6aee91
https://hg.mozilla.org/mozilla-central/rev/7b66eb162975
Status: NEW → RESOLVED
Last Resolved: 29 days ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Comment 19

29 days ago
Comment on attachment 8960340 [details] [diff] [review]
P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1437760
[User impact if declined]: Some sites using service workers are failing to load certain resources.
[Is this code covered by automated tests?]: P2 patch here adds test coverage for the corner case missed in bug 1437760.
[Has the fix been verified in Nightly?]: Its landed in nightly.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk.
[Why is the change risky/not risky?]: Its a one line logic fix.  Its unfortunate we lacked the test coverage to catch this issue in bug 1437760.
[String changes made/needed]: None.
Attachment #8960340 - Flags: approval-mozilla-beta?
(Assignee)

Comment 20

29 days ago
Comment on attachment 8960353 [details] [diff] [review]
P2 Add a WPT test verifying synthetic responses work when a request fragment is present. r=asuth

See comment 19.
Attachment #8960353 - Flags: approval-mozilla-beta?
(Assignee)

Comment 21

29 days ago
I'm not sure if we will be able to get this into a dot release for 59.  Unfortunately I don't have a great work-around for you at the moment.
[Tracking Requested - why for this release]: Liz, fyi, Ben flagged this as a possible ride along for a 59 dot release
tracking-firefox59: --- → ?
tracking-firefox60: --- → +
Flags: needinfo?(lhenry)
Comment on attachment 8960340 [details] [diff] [review]
P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth

let's get this fix into 60.0b6
Attachment #8960340 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8960353 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
How common do we think this is, i.e. might it impact a lot of users or is this an uncommon corner case?
It seems ok to me as a ridealong, along with the tests, if we can verify the fix and don't see any problems when it is out as part of beta 6.
tracking-firefox59: ? → +
Flags: needinfo?(lhenry)
(Assignee)

Comment 26

29 days ago
Its difficult to say.  Its definitely a minority use case, but its not that niche.  It basically sites that use URL fragments (a lot) and service workers (small, but growing) and synthesize responses (no idea how many).
Comment on attachment 8960340 [details] [diff] [review]
P1 Don't try to apply the request fragment to an empty response URL when perform service worker interception. r=asuth

Seems like a low-risk change, let's include it as a ride-along in 59.0.2
Attachment #8960340 - Flags: approval-mozilla-release+

Updated

28 days ago
Attachment #8960353 - Flags: approval-mozilla-release+
Added to 59.0.2 release notes
relnote-firefox: --- → 59+
(Reporter)

Comment 30

27 days ago
Thanks again to everyone for fixing this.  I realize that it's an edge case, but URL fragments are a great way to communicate with service workers for the time-being.
(In reply to Ben Kelly [Part-time, not reviewing][:bkelly] from comment #19)
> [Is this code covered by automated tests?]: P2 patch here adds test coverage
> for the corner case missed in bug 1437760.
> [Has the fix been verified in Nightly?]: Its landed in nightly.
> [Needs manual test from QE? If yes, steps to reproduce]:  No

Setting qe-verify- based on Ben's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10222 for changes under testing/web-platform/tests
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.