Hallvord R. M. Steen [:hallvors] Reporter
	Description • 11 years ago

We're supposed to throw per
http://xhr.spec.whatwg.org/#dom-xmlhttprequest-overridemimetype

Noting that making overrideMimeType() throw more exceptions is slightly controversial:
https://bugs.webkit.org/show_bug.cgi?id=95728#c10

Test case:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/overridemimetype-invalid-mime-type.htm

	Thomas Wisniewski
	Comment 1 • 8 years ago

Anne, is this still something we want to add to XHRs? Chrome, Edge and WebKit also don't support it.

	Anne (:annevk)
	Comment 2 • 8 years ago

So instead of throwing for invalid MIME types, we'd just return early and cause it to have any effect? Or what is the processing model we'd change the specification to?

	Thomas Wisniewski
	Comment 3 • 8 years ago

Based on them failing this test, they simply don't perform this check at all (that is, they allow overriding to an invalid MIME type). Reading the spec, that would seem to be step 2 for the method: https://xhr.spec.whatwg.org/#dom-xmlhttprequest-overridemimetype

	Anne (:annevk)
	Comment 4 • 8 years ago

I see, so

  Content-Type: text/xml

  overrideMimeType("bogus")

results in responseXML not working.

I filed https://github.com/whatwg/xhr/issues/82 to fix this in the standard.

	Thomas Wisniewski
	Comment 5 • 8 years ago

Heh. If it makes you feel better, I'd personally also prefer having the MIME type validation check :)

	Anne (:annevk)
	Comment 6 • 7 years ago

I updated the test in https://github.com/w3c/web-platform-tests/pull/4955 and added more in https://github.com/w3c/web-platform-tests/pull/4993. The standard was updated as well. There's a couple things here we could clean up to get better convergence and not end up breaking things by throwing as previously required.

	Thomas Wisniewski
	Comment 7 • 7 years ago

Anne, could you explain why the WPT for overrideMimeType("text/xml;charset=†") is expecting the response.type to end up as "application/octet-stream"?

Because if it's supposed to be failing on step 3 "If mime is a parsable MIME type" then I don't see anything about it which should not be parsable (according to the current "parseable MIME type" algorithm provided in the links).

Or is the algorithm rejecting non-ASCII characters somehow and I'm just a bit too tired for my own good?

	Anne (:annevk)
	Comment 8 • 7 years ago

Yeah, https://mimesniff.spec.whatwg.org/#parse-a-mime-type returns undefined when you have non-ASCII bytes. I'm not sure how stable all that is to be fair and I'm trying to find out in https://github.com/whatwg/mimesniff/issues/30 so you could leave that as a follow-up if nobody else does it.

If that's the case let me know and I'll dig deeper.

	Thomas Wisniewski
	Comment 9 • 7 years ago

Ah, ok. It doesn't seem to me that the algorithm as-written there currently rejects non-ASCII bytes, so it might be worth making that explicit (or if this is meant to be XHR-specific, to mention that in the XHR spec rather than that algorithm).

I don't mind writing a patch which fixes this, once I know if it's XHR-specific or if we ought to be changing more of the Gecko internals like nsIOService::ExtractCharsetFromContentType to reject non-ASCII characters. Thoughts?

	Anne (:annevk)
	Comment 10 • 7 years ago

It would have to be for all MIME types parsed by Gecko. I don't think it makes sense to introduce XMLHttpRequest-specific code. And you're right, I don't think that algorithm currently disallows non-ASCII. Hmm. I should probably do more work first, unless you're happy to check what other browsers do and report back.

	Thomas Wisniewski
	Comment 11 • 7 years ago

Well, only Chrome is currently passing that test: https://wptdashboard.appspot.com/XMLHttpRequest/overridemimetype-blob.html

And their XHR code doesn't seem to be what's doing the ASCII-filtering, nor do I see anything obvious in their equivalent for ExtractCharsetFromContentType:
XMLHttpRequest::overrideMimeType: https://chromium.googlesource.com/chromium/blink.git/+/master/Source/core/xmlhttprequest/XMLHttpRequest.cpp#1150
XMLHttpRequest::didReceiveResponse: https://chromium.googlesource.com/chromium/blink.git/+/master/Source/core/xmlhttprequest/XMLHttpRequest.cpp#1493
HTTPParsers::extractCharsetFromMediaType: https://chromium.googlesource.com/chromium/blink.git/+/master/Source/platform/network/HTTPParsers.cpp#335

However, I do think what may be happening to help them pass is that they keep track of the (invalid) charset all the way down until their XHR class creates its decoder:
https://chromium.googlesource.com/chromium/blink.git/+/master/Source/core/xmlhttprequest/XMLHttpRequest.cpp#1528
https://chromium.googlesource.com/chromium/blink.git/+/master/Source/core/html/parser/TextResourceDecoder.h#49

That constructor's call to defaultEncoding then ends up testing whether the encoding/charset isValid() and fails, defaulting to Latin-1:
https://chromium.googlesource.com/chromium/blink.git/+/master/Source/core/html/parser/TextResourceDecoder.cpp#104

That would be because isValid() passes the charset through atomicCanonicalTextEncodingName(), which doesn't know that charset:
https://chromium.googlesource.com/chromium/blink/+/master/Source/wtf/text/TextEncoding.h#42
https://chromium.googlesource.com/chromium/blink/+/master/Source/wtf/text/TextEncoding.cpp#47
https://github.com/scheib/chromium/blob/master/third_party/WebKit/Source/platform/wtf/text/TextEncodingRegistry.cpp#L219

I could very well be wrong though, as I don't have time to get a build of Chromium going to diagnose this right now.

	Anne (:annevk)
	Comment 12 • 7 years ago

FWIW, that cannot be the problem as changing it to client.overrideMimeType("text/xml;x=†") will also make Chrome treat it as bogus.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp?l=1375 shows they have some MIME type validation (I'm guessing the repository you looked at is out-of-date). Whether they consistently use that though is another story. I suspect they added it because of this test.

	Anne (:annevk)
	Comment 13 • 7 years ago

So I tested a response with Content-Type set to a MIME type with non-ASCII bytes in it and no browser ends up disregarding that MIME type. So a validity check of that sorts is probably out of the question. If that's the case, I'm not sure we should have it for overrideMimeType().

I guess best to escalate this back to the specification so we get some kind of consistent whole?

	Thomas Wisniewski
	Comment 14 • 7 years ago

>I guess best to escalate this back to the specification so we get some kind of consistent whole?

Yeah, let's do that.

Thanks for looking into this. (I'm a bit weirded out that I was looking at the wrong code).

	Anne (:annevk)
	Comment 15 • 7 years ago

https://github.com/whatwg/xhr/issues/157