918764 - [XHR2] Wrong exception: "TypeError" is not "DOMException SyntaxError"

Status: RESOLVED INVALID

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

Description

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

Test cases where wrong exceptions are being thrown:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-bogus-name.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-bogus-value.htm

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

> void setRequestHeader(ByteString header, ByteString value);

http://xhr.spec.whatwg.org/#xmlhttprequest

> If the value of any element of x is greater than 255, then throw a TypeError.

http://dev.w3.org/2006/webapi/WebIDL/#es-ByteString

Can you fix the test?

Comment 2

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

I can certainly fix the test, but I note that all (two) other testable implementations (OpPresto and Chrome) pass now, and start failing if we require a TypeError instead..?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Since those are incorrect, that seems fine with me ;)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Yes, fixing the test seems to be the right thing here. Other implementations aren't validating the
arguments at all and just fall through to 3 and 4 of http://xhr.spec.whatwg.org/#dom-xmlhttprequest-setrequestheader ?

Comment 5

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

Let's see if I get this right.. so setRequestHeader() can throw two different exceptions for invalid arguments.

1) If either argument 'name' or 'value' contains a character with a code point higher than U+00FF, IDL argument validation of ByteString kicks in and a TypeError is thrown

2) If IDL validation deems all characters valid, the strings are checked against header name/value syntax rules, if the check fails a SyntaxError is thrown.

If I got that right, it seems that the tests are mostly correct as-is since most of them do not test > U+00FF characters. A couple of the tests do, and should be fixed. (Although IMHO it is a bit confusing to spec two different errors for invalid input here - maybe the spec should just call the input a DOMString and rely on the HTTP header syntax check? This is of course nitpicking though..)

Comment 6

[Anne (:annevk)]

We still haven't figured out the exact exception story. Just using TypeError here works for me.

Comment 7

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

(I was sort of suggesting using SyntaxError for everything.. ;)). Would be good to figure out this now to finalize test suite and spec..

I'd still like Ms2ger to confirm that I got the (ccurrent) logic right in comment 15.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yes, comment 15 is correct.

Comment 9

[Anne (:annevk)]

Hallvord, if we can get away with native JavaScript exceptions that's better than DOMException since DOMException is still in flux and kinda broken. Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=23346

Comment 10

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

OK, makes sense. I'll go through and fix the relevant tests accordingly when you've changed the spec (might also be able to close some of the [XHR2] bugs here in the process ;)).

Comment 11

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

So, if Anne wants everything to throw TypeError we still have work to do in this bug - reopening. This is what the tests are going to look like:

http://w3c-test.org/web-platform-tests/submissions/343/XMLHttpRequest/setrequestheader-bogus-name.htm
http://w3c-test.org/web-platform-tests/submissions/343/XMLHttpRequest/setrequestheader-bogus-value.htm

Comment 12

[Thomas Wisniewski]

There are two tests still failing in http://w3c-test.org/XMLHttpRequest/setrequestheader-bogus-value.htm

One has a tab (\t) in the header's value and the other has the DEL character (\x7f). The function being called to validate the string is nsHttp::IsReasonableHeaderValue(), which only invalidates if it finds an \r \n or \0. This doesn't follow the RFC 7230 productions for field-values, which the XHR spec says should be used to decide whether to throw a SyntaxError:

  field-value = *( field-content / obs-fold )
    field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
       field-vchar = VCHAR / obs-text
         VCHAR = any visible [USASCII] character
         obs-text = %x80-FF
    obs-fold = CRLF 1*( SP / HTAB )

However, I'm not certain if fixing IsReasonableHeaderValue() is the right choice, or if an similar alternative method should be made just for XHRs specifically. Plus, Chrome also fails these two tests, so I'm left wondering what the best course of action here is.

Comment 13

[Anne (:annevk)]

See https://github.com/whatwg/fetch/issues/115 and https://github.com/whatwg/fetch/issues/100. This was changed based on changes made to WebKit, but perhaps that's too strict?

If we change this we should do so in a way that affects XMLHttpRequest, fetch(), and networking in general. If we cannot change this we should probably open a new issue against Fetch.

Comment 14

[Thomas Wisniewski]

Attachment: 918764-follow-RFC7230-in-IsReasonableHeaderValue.diff

It looks like the Blink devs want more investigation before they take this:
https://bugs.chromium.org/p/chromium/issues/detail?id=455099

They don't seem inclined to investigate it with telemetry, though.

Regardless, here's a candidate patch which implements the same proposed changes, just in case we do want to make these changes sometime (should affect networking in general, not just XHRs).

Comment 15

[Anne (:annevk)]

Thomas, shall we close this? I think we pass the relevant tests now so this got resolved somehow, probably by changing the standard around.

Comment 16

[Thomas Wisniewski]

Yes, I don't see any reason to keep this open if the tests are passing now. We can always reopen if later consensus is reached on using RFC 7230 productions for field values.

Comment 17

[Anne (:annevk)]

I forgot, but I filed bug 1330297 on the remaining issues, but the patch here is not applicable either way.