Closed Bug 1443850 Opened 6 years ago Closed 6 years ago

URL Fragment Identifiers Break Service Worker Responses

Categories

(Core :: DOM: Service Workers, defect, P1)

59 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
relnote-firefox --- 59+
firefox-esr52 --- disabled
firefox59 + fixed
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: chris, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

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.
Attached image 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
Assignee: nobody → bugmail
Priority: -- → P1
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.
Thanks for investigating while I was out Andrew!  I'll try to figure out a fix here.
Assignee: bugmail → bkelly
Flags: needinfo?(bugmail)
Keywords: regression
This fixes the problem locally in the comment 0 demo.  Now I need to add a WPT test somewhere...
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
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)
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+
Thank you everyone for your attention to this bug.  Long live the URL fragment...
Please see the try build in comment 15.  Thanks.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/a17d9a6aee91
https://hg.mozilla.org/mozilla-central/rev/7b66eb162975
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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?
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?
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
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.
Flags: needinfo?(lhenry)
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+
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
You need to log in before you can comment on or make changes to this bug.