Closed
Bug 1437760
Opened 3 years ago
Closed 3 years ago
SVG images lose their fragment from url when requested via service worker
Categories
(Core :: DOM: Service Workers, defect, P3)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | fixed |
firefox60 | --- | fixed |
People
(Reporter: xidorn, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
(Keywords: regression, site-compat)
Attachments
(3 files, 1 obsolete file)
1.23 KB,
application/zip
|
Details | |
4.34 KB,
patch
|
asuth
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
12.11 KB,
patch
|
xidorn
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•3 years ago
|
||
This is the regression I mentioned in bug 1317036 comment 10. Ben, WDYT?
Flags: needinfo?(bkelly)
Reporter | ||
Updated•3 years ago
|
status-firefox59:
--- → affected
Updated•3 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•3 years ago
|
||
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.
Blocks: ServiceWorkers-compat
Flags: needinfo?(bkelly)
Updated•3 years ago
|
Priority: P2 → P3
Reporter | ||
Comment 3•3 years 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.
Assignee | ||
Comment 4•3 years ago
|
||
No, because document navigation does not look at Response.url. It explicitly looks at it Request.url per the spec.
Assignee | ||
Comment 5•3 years ago
|
||
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•3 years 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•3 years ago
|
Keywords: site-compat
Assignee | ||
Comment 7•3 years ago
|
||
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•3 years 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.
Assignee | ||
Comment 9•3 years ago
|
||
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•3 years 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)
Assignee | ||
Comment 11•3 years ago
|
||
(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???
Assignee | ||
Comment 12•3 years ago
|
||
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.
Assignee | ||
Comment 13•3 years ago
|
||
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
Assignee | ||
Comment 14•3 years ago
|
||
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.
Assignee | ||
Comment 15•3 years ago
|
||
This fixes the comment 0 test case for me. I'll turn it into a tentative WPT test.
Assignee | ||
Comment 16•3 years ago
|
||
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.
Assignee | ||
Comment 17•3 years ago
|
||
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)
Assignee | ||
Comment 18•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35d9cc427af83d6c8105c0357cf79c75a8a24b34
Assignee | ||
Comment 19•3 years ago
|
||
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)
Assignee | ||
Comment 20•3 years ago
|
||
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.
Assignee | ||
Comment 21•3 years ago
|
||
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)
Assignee | ||
Comment 22•3 years ago
|
||
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)
Assignee | ||
Comment 23•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92a10ba5d91a493dc9d56cc263d1249d400821b5
Assignee | ||
Comment 24•3 years ago
|
||
Note, the test now passes --verify locally.
Comment 25•3 years ago
|
||
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•3 years 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•3 years 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
Assignee | ||
Comment 29•3 years ago
|
||
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?
Assignee | ||
Comment 30•3 years 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 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•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c6b9c2cb220d https://hg.mozilla.org/mozilla-central/rev/bb3c9990840a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•3 years ago
|
status-firefox58:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: in-testsuite+
Keywords: regression
Comment 33•3 years ago
|
||
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+
Updated•3 years ago
|
Attachment #8952519 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
![]() |
||
Comment 34•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0210188bd3aa https://hg.mozilla.org/releases/mozilla-beta/rev/c4a9ef59aa89
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•