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

RESOLVED FIXED in Firefox 59

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: bkelly)

Tracking

(Blocks 1 bug, {regression, site-compat})

Trunk
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 fixed, firefox60 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Posted file 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));
> });
(Reporter)

Comment 1

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

Ben, WDYT?
Flags: needinfo?(bkelly)
Priority: -- → P2
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.
Flags: needinfo?(bkelly)
Priority: P2 → P3
(Reporter)

Comment 3

a year ago
(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.
No, because document navigation does not look at Response.url.  It explicitly looks at it Request.url per the spec.
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.
(Reporter)

Comment 6

a year ago
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.
(Reporter)

Updated

a year ago
Keywords: site-compat
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.
(Reporter)

Comment 8

a year ago
(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.
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.
Flags: needinfo?(annevk)

Comment 10

a year ago
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.
Flags: needinfo?(annevk)
(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???
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.
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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.
This fixes the comment 0 test case for me.  I'll turn it into a tentative WPT test.
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.
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.
Attachment #8952500 - Flags: review?(xidorn+moz)
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
Attachment #8952422 - Flags: review?(bugmail)
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 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.
Attachment #8952500 - Flags: review?(xidorn+moz)
The previous patch was missing an `await` before the `with_iframe()` call.

Please see comment 17 for the previous description of the patch.
Attachment #8952500 - Attachment is obsolete: true
Attachment #8952519 - Flags: review?(xidorn+moz)
Note, the test now passes --verify locally.
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.
Attachment #8952422 - Flags: review?(bugmail) → review+
(Reporter)

Comment 26

a year ago
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.
Attachment #8952519 - Flags: review?(xidorn+moz) → review+

Comment 27

a year ago
Pushed by bkelly@mozilla.com:
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
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9604 for changes under testing/web-platform/tests
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
Attachment #8952422 - Flags: approval-mozilla-beta?
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.
Attachment #8952519 - Flags: approval-mozilla-beta?
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c6b9c2cb220d
https://hg.mozilla.org/mozilla-central/rev/bb3c9990840a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: in-testsuite+
Keywords: regression
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.
Attachment #8952422 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8952519 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.