Bug 15356 - getResponseHeader and getAllResponseHeaders do not throw exceptions
: getResponseHeader and getAllResponseHeaders do not throw exceptions
Status: RESOLVED FIXED
: WebKit
XML
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2007-10-03 03:00 PST by
Modified: 2009-09-14 09:55 PST (History)


Attachments
First patch (10.36 KB, patch)
2007-10-03 05:00 PST, Julien Chaffraix
ap: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch (13.90 KB, patch)
2007-10-09 09:31 PST, Julien Chaffraix
darin: review-
Review Patch | Details | Formatted Diff | Diff
Patch updated with Darin's comments and current discussion (13.66 KB, patch)
2007-10-16 15:20 PST, Julien Chaffraix
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-10-03 03:00:16 PST
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.
------- Comment #1 From 2007-10-03 05:00:20 PST -------
Created an attachment (id=16516) [details]
First patch

The patch corrects the behavior of the two methods.
------- Comment #2 From 2007-10-03 12:44:59 PST -------
+    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.
------- Comment #3 From 2007-10-03 17:04:47 PST -------
>+    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.
------- Comment #4 From 2007-10-04 09:17:45 PST -------
(From update of attachment 16516 [details])
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
------- Comment #5 From 2007-10-04 09:22:05 PST -------
(From update of attachment 16516 [details])
Actually marking r- because of the isValidHeaderValue issue (I should have done that right away).
------- Comment #6 From 2007-10-04 09:25:27 PST -------
(In reply to comment #5)
> Actually marking r- because of the isValidHeaderValue issue (I should have done
> that right away).

Good catch. Sorry!
------- Comment #7 From 2007-10-09 09:31:57 PST -------
Created an attachment (id=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.
------- Comment #8 From 2007-10-15 09:19:39 PST -------
(From update of attachment 16596 [details])
+        // 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
------- Comment #9 From 2007-10-15 15:01:32 PST -------
> 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).
------- Comment #10 From 2007-10-16 04:47:04 PST -------
(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.
------- Comment #11 From 2007-10-16 06:55:22 PST -------
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.
------- Comment #12 From 2007-10-16 14:58:55 PST -------
> 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
------- Comment #13 From 2007-10-16 15:20:28 PST -------
Created an attachment (id=16688) [details]
Patch updated with Darin's comments and current discussion
------- Comment #14 From 2007-10-19 10:51:43 PST -------
(From update of attachment 16688 [details])
r=me (hope Alexey doesn't notice something I missed this time!)
------- Comment #15 From 2007-10-24 06:00:51 PST -------
Landed in r26986.  Be wary of tabs, your new test cases contained a few.
------- Comment #16 From 2007-10-24 06:39:23 PST -------
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.
------- Comment #17 From 2009-09-14 09:30:53 PST -------
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
------- Comment #18 From 2009-09-14 09:55:34 PST -------
By the way, the spec says to return null in case the production does not match so you might want to change the code.