Closed
Bug 924319
Opened 11 years ago
Closed 8 years ago
[XHR2] Do not fake content-length header for data: URLs
Categories
(Core :: DOM: Core & HTML, defect, P4)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: hsteen, Assigned: wisniewskit, NeedInfo)
References
()
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
The 7th subtest here: http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/data-uri-basic.htm fails with this description: Fail GET getAllResponseHeaders assert_equals: expected "content-type: text/plain" but got "Content-Type: text/plain\r\nContent-Length: 13\r\n" We should not report a Content-Length header. See http://lists.w3.org/Archives/Public/public-webapps/2013JulSep/0606.html + thread and http://logbot.glob.com.au/?c=mozilla%23developers&s=2+Oct+2013&e=2+Oct+2013#c777688 for discussion and background.
Comment 1•11 years ago
|
||
I believe simply removing http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/data/nsDataChannel.cpp?rev=aceb8d1e6eba&mark=84-84#84 will have this effect... But note that this will also affect all other progress notifications in the system.
I don't have a strong opinion either way here. Though disabling it for all nsDataChannels without first auditing where those are used sounds scary. I'd recommend doing it on a per-instance unless that gets very hairy.
Assignee | ||
Comment 3•8 years ago
|
||
Here's a simple patch that passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=995aea23f463 (it includes the fix for the dom/base/test/test_bug782342.html failures; the others seem unrelated). There was no need to touch nsDataChannel or anything, as the code in XMLHttpRequest.getAllResponseHeaders() was explicitly sending a content-length header, and just removing that code-block sufficed.
Attachment #8763346 -
Flags: review?(mrbkap)
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8763346 [details] [diff] [review] 924319-do-not-include-content-length-in-getallresponseheaders.diff Review of attachment 8763346 [details] [diff] [review]: ----------------------------------------------------------------- Disregard, I somehow submitted an incomplete patch that doesn't limit to data: URLs. I'll fix it ASAP.
Attachment #8763346 -
Flags: review?(mrbkap)
Assignee | ||
Comment 5•8 years ago
|
||
Here's a new version of the patch which only reports Content-Length for non-data-URIs in getAllResponseHeaders. It passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=783dfee256cd But note that I had to re-run a couple of jobs as the tryserver seemed to be hiccuping the first time it ran M-e10s 2 and bc2.
Attachment #8763346 -
Attachment is obsolete: true
Attachment #8765954 -
Flags: review?(jst)
Comment 6•8 years ago
|
||
Comment on attachment 8765954 [details] [diff] [review] 924319-do-not-provide-content-length-for-data-uris-in-getallresponseheaders.diff + nsAutoCString scheme; + uri->GetScheme(scheme); + isDataURI = scheme.LowerCaseEqualsLiteral("data"); Any reason not to use uri->SchemeIs() here instead of extracting the scheme and doing a string compare? If not, please change that and re-request review on the update patch, or re-request review on this one with an explanation why that isn't a good idea. Thanks!
Attachment #8765954 -
Flags: review?(jst)
Assignee | ||
Comment 7•8 years ago
|
||
> Any reason not to use uri->SchemeIs() here
Nope, I just didn't think to check for a SchemeIs() method. Here's a new version that uses it instead.
Attachment #8765954 -
Attachment is obsolete: true
Attachment #8766102 -
Flags: review?(jst)
Comment 8•8 years ago
|
||
Comment on attachment 8766102 [details] [diff] [review] 924319-do-not-provide-content-length-for-data-uris-in-getallresponseheaders.diff Looks good, thanks!
Attachment #8766102 -
Flags: review?(jst) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Alright, thanks. Let me know if anything else is needed before check-in (I can't set checkin-needed on my own).
Comment 10•8 years ago
|
||
(In reply to Thomas Wisniewski from comment #9) > Alright, thanks. Let me know if anything else is needed before check-in (I > can't set checkin-needed on my own). Should be good to go, and now you can set checkin-needed yourself :)
Assignee | ||
Comment 11•8 years ago
|
||
Thanks!
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → wisniewskit
Comment 12•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/53c93df54e84 do not include Content-Length for data URIs in XMLHttpRequest.getAllResponseHeaders. r=jst
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/53c93df54e84
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 14•8 years ago
|
||
So why do we want to limit this to data: URLs? Are there other non-HTTP URLs that are supposed to pretend to have a Content-Length header?
Flags: needinfo?(jst)
Comment 15•8 years ago
|
||
Hmm, that is a good question. Per spec I'm not aware of any, but we may have other internal URI schemes that could fall into this category.
Flags: needinfo?(jst)
Comment 16•8 years ago
|
||
Hmm. So I looked at the spec more closely. https://xhr.spec.whatwg.org/#the-getallresponseheaders()-method says to return things from the response's header list. Per spec, the only things which have a Content-Length header in the header list are blob: and http/https, with file/ftp/filesystem behavior undefined. Of course the spec assumes there are no extensions to fetch, which for us there are. Anyway, now we're following the spec for data:, but not for for about:blank, for example... It's not really clear to me why the spec doesn't synthesize a Content-Length for data: and about:blank, just like it does for blob: and just like it synthesizes Content-Type for both of those. Anne, do you know? And does anyone know what other UAs do for file/ftp/filesystem? That is, should this really be a blacklist of things that do NOT get Content-Length synthesized, as in this patch, or should it be a whitelist of things that _do_ get it synthesized? In any case, it seems like this should be controlled by the protocol handler, not XHR code directly...
Flags: needinfo?(jst)
Flags: needinfo?(annevk)
Comment 17•8 years ago
|
||
https://lists.w3.org/Archives/Public/public-webapps/2013JulSep/thread.html#msg614 has the discussion. For blob URLs we know it's synchronously available due to Blob.prototype.size being synchronous. For data URLs you could have an architecture where that is harder, and it requires actually parsing the complete URL in advance (which is not always needed).
Flags: needinfo?(annevk)
Comment 18•8 years ago
|
||
Ah, I see. What about about:blank? That one has no such issue, since the length is known to be 0...
Comment 19•8 years ago
|
||
We could include C-L for about:blank, but I'm not sure it's worth it since with service workers, synthetic responses, and also due to content codings, C-L is not really a reliable indicator for developers. It would be reliable for about:blank of course, but if you first had to determine whether it's reliable by checking about:blank, you already have all the information you need at that point.
You need to log in
before you can comment on or make changes to this bug.
Description
•