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




6 years ago
2 years ago


(Reporter: hsteen, Unassigned)


Firefox Tracking Flags

(Not tracked)




(1 attachment)

> void setRequestHeader(ByteString header, ByteString value);


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


Can you fix the test?
Last Resolved: 6 years ago
Flags: needinfo?(hsteen)
Resolution: --- → INVALID

Comment 2

6 years ago
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..?
Since those are incorrect, that seems fine with me ;)
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

6 years ago
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..)
Flags: needinfo?(hsteen)

Comment 6

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

Comment 7

6 years ago
(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.
Flags: needinfo?(Ms2ger)
Yes, comment 15 is correct.
Flags: needinfo?(Ms2ger)

Comment 9

6 years ago
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

6 years ago
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

6 years ago
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:

Resolution: INVALID → ---


6 years ago
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

3 years ago
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.
It looks like the Blink devs want more investigation before they take this:

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

2 years ago
Thomas, shall we close this? I think we pass the relevant tests now so this got resolved somehow, probably by changing the standard around.
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.
Last Resolved: 6 years ago2 years ago
Resolution: --- → INVALID

Comment 17

2 years ago
I forgot, but I filed bug 1330297 on the remaining issues, but the patch here is not applicable either way.
You need to log in before you can comment on or make changes to this bug.