Bug 15356 – getResponseHeader and getAllResponseHeaders do not throw exceptions

• Component: XML
• Status: RESOLVED FIXED

[Julien Chaffraix]

According to the spec, getReponseHeader and getAllResponseHeaders should send an INVALID_STATE_ERR exception if state is RECEIVING or DONE.
getResponseHeader should also send a SYNTAX_ERR exception if the header argument does not match the field-name production.

Following patch corrects those behaviors.

[Julien Chaffraix]

Created attachment 16516 [details]
First patch

The patch corrects the behavior of the two methods.

[Alexey Proskuryakov]

+    if (!isValidHeaderValue(name)) {

It seems that a new isValidHeaderName() function is needed here - header name and value productions are different. Eric has made some comments on IRC, too.

[Julien Chaffraix]

>+    if (!isValidHeaderValue(name)) {

> It seems that a new isValidHeaderName() function is needed here - header name
> and value productions are different. Eric has made some comments on IRC, too.

I have mistaken the function to call. I should have called isValidToken as it is the case in setResponseHeader (according to the RFC 2616 header-name is a token).
Thanks for pointing the error.

Concerning the tests suggested by Eric, I have tested  with the latest Firefox and it seems that it does not support all the exception added by the patch.
I will test on IE7 tomorrow once I can put my hand on it and update the results here.

[Darin Adler]

Comment on attachment 16516 [details]
First patch

Throwing exceptions in these cases can easily cause a compatibility regression with web pages or widgets that are doing it wrong and "getting away with it".

But I think these calls are rarely enough used that it's not going to be a problem.

r=me for the feature branch

[Alexey Proskuryakov]

Comment on attachment 16516 [details]
First patch

Actually marking r- because of the isValidHeaderValue issue (I should have done that right away).

[Darin Adler]

(In reply to comment #5)
> Actually marking r- because of the isValidHeaderValue issue (I should have done
> that right away).

Good catch. Sorry!

[Julien Chaffraix]

Created attachment 16596 [details]
Updated patch

Concerning Darin's comment about regression, I did some testing (as I told you I would) and updated the patch and the tests according to the results.

For INVALID_STATE_ERR :

IE7 : raise an exception when readyState < 4
FF 3 : has a buggy behaviour that depends on the version I have used but i does raise an exception at some point before readyState 3.
Opera : raise an exception when readyState < 3

Here raising an exception when readyState < 3 seems the right behaviour and I have not changed that part.

For SYNTAX_ERROR :

IE 7 & Opera : returns an empty string
Firefox : returns null

I sided with Opera and IE and returned an empty string if the requested header is not valid.

[Darin Adler]

Comment on attachment 16596 [details]
Updated patch

+        // Should raise a SYNTAX_ERR exception
+        // but Opera and IE returns an empty string, so we just return an empty string

We should get this fixed in the XMLHttpRequest specification if browsers really can't follow it and be compatible.

Is there really a need to add the isValidToken check? What's the behavior if we leave it out? Do any of the tests you wrote fail if you leave out the isValidToken check?

+            //used to call getResponseHeader but could raise unnecessary exception

I don't think that's a helpful comment. Please just leave it out.

+    String getAllResponseHeaders(ExceptionCode& ec) const;
+    String getResponseHeader(const String& name, ExceptionCode& ec) const;

The argument doesn't need a name in the header (see the setRequestHeader above for an example of how we write these).

review- mainly because of the isValidToken question

[Julien Chaffraix]

> Is there really a need to add the isValidToken check? What's the behavior if we
> leave it out? Do any of the tests you wrote fail if you leave out the
> isValidToken check?

Without isValidToken we return null as does Firefox. A side effect is to make the test case fail but it was designed to fail if something else that an empty string is returned so it is not relevant.

So basically we have the choice to side with Firefox (return null) or Opera/IE (return an empty string).

[Alexey Proskuryakov]

(In reply to comment #8)
> +        // Should raise a SYNTAX_ERR exception
> +        // but Opera and IE returns an empty string, so we just return an
> empty string
> 
> We should get this fixed in the XMLHttpRequest specification if browsers really
> can't follow it and be compatible.

Cc'ing Anne.

[Anne van Kesteren]

Ok, so instead of throwing a SYNTAX_ERR for incorrect header names the empty string would be returned. I guess this is ok, although that is also returned for valid header names that are empty.

If you could e-mail the public-webapi list that you'd like getResponseHeader() header changed in that way that would be great.

[Julien Chaffraix]

> If you could e-mail the public-webapi list that you'd like getResponseHeader()
> header changed in that way that would be great.

Just send an email requesting the modification.

Updated patch will follow

[Julien Chaffraix]

Created attachment 16688 [details]
Patch updated with Darin's comments and current discussion

[Darin Adler]

Comment on attachment 16688 [details]
Patch updated with Darin's comments and current discussion

r=me (hope Alexey doesn't notice something I missed this time!)

[Mark Rowe (bdash)]

Landed in r26986.  Be wary of tabs, your new test cases contained a few.

[Mark Rowe (bdash)]

I also had to land new test results in r26989 as the results included in the patch didn't match what the test was expected to generate.

[Carol Szabo]

I opened a new bug (29140) related to this issue. The issue is a little different than the one in the title of this bug, but it touches on issues discussed in the comments here. https://bugs.webkit.org/show_bug.cgi?id=29140

[Anne van Kesteren]

By the way, the spec says to return null in case the production does not match so you might want to change the code.