1437760 - SVG images lose their fragment from url when requested via service worker

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

Steps to reproduce:
1. extract the attached testcase
2. run a http server to serve the files (from the root)
3. open the page via the http server
4. refresh the page

Expected result:
The twitter icon should always show up.

Actual result:
The twitter icon disappear after refresh (when it is served via service worker).


According to mozregression, it is a regression from bug 1222008.

Here, the service worker is simply doing:
> self.addEventListener('fetch', evt => {
>   evt.respondWith(fetch(evt.request));
> });
and the SVG file has a
> <style>use:not(:target) { display: none; }</style>
which is arguably common in SVG files for sharing.

It seems that when the request goes via service worker, the url loses its fragment part, and thus this CSS rule inside SVG file doesn't work, and consequently there is nothing show up.

Interestingly, if you reconstruct the response, everything would start working again:
> async function inspect(request) {
>   let resp = await fetch(request);
>   let headers = resp.headers;
>   let blob = await resp.blob();
>   return new Response(blob, {headers});
> }
> self.addEventListener('fetch', evt => {
>   evt.respondWith(inspect(evt.request));
> });

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

This is the regression I mentioned in bug 1317036 comment 10.

Ben, WDYT?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I think this is a spec problem.  The Response.url strips the fragment and we now expose that Response.url as the final nsIChannel URL.  Creating an explicit synthetic Response works around the problem because that effectively ends up using the FetchEvent.request.url which still has the fragment.

See this spec issue:

https://github.com/whatwg/fetch/issues/505#issuecomment-365373835

You might need to do the explicit Response as a work-around for now.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Ben Kelly [:bkelly] from comment #2)
> I think this is a spec problem.  The Response.url strips the fragment and we
> now expose that Response.url as the final nsIChannel URL.  Creating an
> explicit synthetic Response works around the problem because that
> effectively ends up using the FetchEvent.request.url which still has the
> fragment.

If this is a spec problem, then fragment seems to be basically useless anywhere.

IIUC, spec-wise, we are unifying all network requests to the fetch model, and if the Response.url striping fragment indicates fragment is ignoed when handling the resource, fragment should be consistently dropped everywhere, which is definitely wrong.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

No, because document navigation does not look at Response.url.  It explicitly looks at it Request.url per the spec.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

But the fetch spec is coming around to the idea that stripping the fragment is wrong and we're trying to fix it at the spec level.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

In the mean time, doesn't it make more sense to change our behavior to match other browsers? As far as I can see, neither Chrome nor Safari breaks this.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Well, someone has to implement the spec first.  The fact the other browsers have not done so yet is not a blocker for us implementing the spec.

If there are other sites broken, I would be interested to know.  So far this is the first report I've seen.

For right now, though, I'd at least like to get some feedback from Anne on the spec issue before doing anything.  We kind of need to know where we are going in order to know if a quirk makes sense.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Ben Kelly [:bkelly] from comment #4)
> No, because document navigation does not look at Response.url.  It
> explicitly looks at it Request.url per the spec.

I mean, for all the sub-resources including images, videos, etc.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Anne, when you get back from PTO can you help me figure out the best way forward on this issue?  Its unclear to me how to handle the case where we propagate back the intercepted Response.url to the outer network request and the fragment gets stripped as a result.  I could pipe the fragment back in a hidden field, but that seems bad unless we want to spec that.

Comment 10

[Anne (:annevk)]

I think technically the specification already preserves the fragment in a hidden field, it just doesn't return it? In any case, I think we should change the specification to have fragment propagation always, and don't strip it when serializing any URL. The main issue is that other implementations do not do that either so there might be compatibility issues, even if it's clearly better.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anne (:annevk) from comment #10)
> I think technically the specification already preserves the fragment in a
> hidden field, it just doesn't return it? In any case, I think we should

Sure, but I don't see where the html spec consumer would access that hidden field.

Of course, I don't see in the spec how the html spec consumer accesses the filtered body of an opaque response either.  AFAICT stuff like <img> just says to "Fetch request", but doesn't look for a filtered response to pull out the internal data.  I'll file a spec issue on this:

https://github.com/whatwg/html/issues/3483

As a side note, I can't really find the processing model for SVG.  Its hard to tell what its supposed to do at all during loading.  Since it has a document, maybe it should be doing more of what navigation does???

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I think the short term solution Anne and I have settled on is to take the Request.url fragment and apply it to the Response.url when service worker interception happens.  We think this matches what we want in the future.  Also, in the future Response.url will also probably be able to have a different fragment as well, but that will take longer to figure out.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

This is a web compat issue that will merge to release in a few weeks.  I'm going to do the request-to-response fragment propagation for now.  I'll do a mochitest since the spec is still in flux.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Actually, I can do a .tentative WPT test.  I was not aware of that naming convention and it would help Anne with the further spec work.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Propagate the FetchEvent.request.url fragment to the synthesized Response.url. r=asuth

This fixes the comment 0 test case for me.  I'll turn it into a tentative WPT test.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Hmm, it seems like background images and <img> elements don't reflect the final URL.  So I can't just programmatically check the .src attribute.  I think I would have to write a reftest, which I've never done before.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a reftest that verifies svg images with fragment based targets renders properly when service workers intercept. r=xidorn

This reftest passes with the P1 patch and fails without it.  Xidorn, can you review this for me?  Its my first reftest and I believe the first reftest in the service-workers part of WPT.  Also, its my first svg as well.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35d9cc427af83d6c8105c0357cf79c75a8a24b34

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8952422 [details] [diff] [review]
P1 Propagate the FetchEvent.request.url fragment to the synthesized Response.url. r=asuth

Andrew, this patch applies the FetchEvent.request.url's fragment to the propagated Response.url.  This is very similar to what we do for redirects and end consumer code in gecko depends on it.

Ultimately it looks like we will also expose fragments on Response.url, but for right now that still needs to be spec'd.  See:

https://github.com/whatwg/fetch/issues/505#issuecomment-365373835

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I chose not to make the WPT a .tentative.  I think it should be somewhat uncontroversial that a pass-through fetch should not break stuff in the window that works when there is no service worker.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8952500 [details] [diff] [review]
P2 Add a reftest that verifies svg images with fragment based targets renders properly when service workers intercept. r=xidorn

I need to figure out why this is failing in automation.  I must not be waiting for the image to load or something.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a reftest that verifies svg images with fragment based targets renders properly when service workers intercept. r=xidorn

The previous patch was missing an `await` before the `with_iframe()` call.

Please see comment 17 for the previous description of the patch.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92a10ba5d91a493dc9d56cc263d1249d400821b5

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Note, the test now passes --verify locally.

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8952422 [details] [diff] [review]
P1 Propagate the FetchEvent.request.url fragment to the synthesized Response.url. r=asuth

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

Affirming:
- Sanity check: The fragment doesn't include # explicitly per the logic in nsBaseURLParser::ParsePath, so we don't need to worry about "#foo" becoming "##foo".
- More explicit SW/fetch test coverage of this change is waiting for spec changes to fall out of https://github.com/whatwg/fetch/issues/505.  When that happens, a test that makes sure #'s don't multiple wouldn't hurt, though.
- http://web-platform-tests.org/writing-tests/reftests.html has examples where there's no <html> tag in html documents, so part 2 is legit even if parts of me are freaking out about the lack of precious, precious tags, even if the HTML spec totally is fine with it because things like `"before html" insertion mode` handle it.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8952519 [details] [diff] [review]
P2 Add a reftest that verifies svg images with fragment based targets renders properly when service workers intercept. r=xidorn

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

Looks good to me, thanks.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b9c2cb220d
P1 Propagate the FetchEvent.request.url fragment to the synthesized Response.url. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3c9990840a
P2 Add a reftest that verifies svg images with fragment based targets renders properly when service workers intercept. r=xidorn

Comment 28

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9604 for changes under testing/web-platform/tests

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8952422 [details] [diff] [review]
P1 Propagate the FetchEvent.request.url fragment to the synthesized Response.url. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1222008
[User impact if declined]: Sites using svg images with targets and service workers will not work properly.  There may be blank or incorrect information shown on the page.
[Is this code covered by automated tests?]: Yes, P2 adds a WPT reftest.
[Has the fix been verified in Nightly?]: It has not landed in nightly yet.  I am flagging a bit early since the merge is getting close and I don't want to forget.
[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?]: Prior to bug 1222008 we effectively preserved the fragment in the same way by just using the Request.url.  The operation here is also similar to what is done for standard redirects.  The change is also relatively small and contained to only service worker code.
[String changes made/needed]: None

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8952519 [details] [diff] [review]
P2 Add a reftest that verifies svg images with fragment based targets renders properly when service workers intercept. r=xidorn

See comment 29.

Comment 31

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c6b9c2cb220d
https://hg.mozilla.org/mozilla-central/rev/bb3c9990840a

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8952422 [details] [diff] [review]
P1 Propagate the FetchEvent.request.url fragment to the synthesized Response.url. r=asuth

Web-compat issue with a low-risk fix and automated tests. Let's take it for 59b12.

Comment 34

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0210188bd3aa
https://hg.mozilla.org/releases/mozilla-beta/rev/c4a9ef59aa89

Comment 35

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged