924319 - [XHR2] Do not fake content-length header for data: URLs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P4)

Description

[Hallvord R. M. Steen [:hallvors]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Thomas Wisniewski]

Attachment: 924319-do-not-include-content-length-in-getallresponseheaders.diff

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.

Comment 4

[Thomas Wisniewski]

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.

Comment 5

[Thomas Wisniewski]

Attachment: 924319-do-not-provide-content-length-for-data-uris-in-getallresponseheaders.diff

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.

Comment 6

[Johnny Stenback (:jst)]

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!

Comment 7

[Thomas Wisniewski]

Attachment: 924319-do-not-provide-content-length-for-data-uris-in-getallresponseheaders.diff

> 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.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 8766102 [details] [diff] [review]
924319-do-not-provide-content-length-for-data-uris-in-getallresponseheaders.diff

Looks good, thanks!

Comment 9

[Thomas Wisniewski]

Alright, thanks. Let me know if anything else is needed before check-in (I can't set checkin-needed on my own).

Comment 10

[Johnny Stenback (:jst)]

(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 :)

Comment 11

[Thomas Wisniewski]

Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c93df54e84
do not include Content-Length for data URIs in XMLHttpRequest.getAllResponseHeaders. r=jst

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53c93df54e84

Comment 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

[Johnny Stenback (:jst)]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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...

Comment 17

[Anne (:annevk)]

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).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  What about about:blank?  That one has no such issue, since the length is known to be 0...

Comment 19

[Anne (:annevk)]

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.