Bug 29140 – getResponseHeader returns "" instead of null as the spec requires.

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Carol Szabo]

getResponseHeader returns an empty string when called with "Set-Cookie" as a parameter in the proper readyState. http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-getresponseheader-method specifies that null should be returned.

[Carol Szabo]

Intend to submit patch soon.

[Carol Szabo]

I have read the discussion on bug 15356, and I saw that some changes were made to the spec probably because of those discussions, but I believe that WebKit is still not conforming to the spec and not only that, but due to its behavior makes it difficult for JavaScript developers to distinguish between missing and empty headers, while the spec makes that easy and I also do not believe that changing this behavior should cause major compatibility issues.
If nobody dares to break with a bad legacy, progress will die, and furthermore things will become worse and worse, as mistakes will keep creeping in over the years.

[Alexey Proskuryakov]

That's very interesting - from my reading of the code, this doesn't seem to be the case for JS bindings at least.

> If nobody dares to break with a bad legacy, progress will die

This is not the kind of argument that helps move a patch forward - instead, many people will avoid bugs with politically charged discussions.

Please do test what IE and Firefox return, both for blacklisted headers like Set-Cookie, and for ones that just aren't in the response.

[Alexey Proskuryakov]

(In reply to comment #3)
> That's very interesting - from my reading of the code, this doesn't seem to be
> the case for JS bindings at least.

Oh sorry, my reading was incorrect :)

[Carol Szabo]

Created attachment 39698 [details]
Proposed patch

I have verified that Firefox returns null for the valid states (>=2), but does not throw an exception for the 0 state, and in some versions not for the 1 state either.
I was unable to install a recent version of IE (7.0 or better) on the computers that I have access to (download restrictions and failure of internal install packages) thus I am not sure what recent IE behavior is but IE 6.0 is very far from the standard: has redefined states, headers being accessible only after the entire document is loaded, and then sometimes it returns an empty string, sometimes it throws an exception (like in the case of null).

[Carol Szabo]

I have checked IE and IE is inconsistent in handling XMLHttpRequest between major versions, but for the most part it returns an empty string for missing, unsafe. empty, and malformed header names.
I believe that we should go with the specification, not only because it is the specification, but because it allows distinguishing a missing header from a header with an empty value in it.

[Carol Szabo]

Here is the response from the editor of the XMLHttpRequest Level 2
specification to the question that this bug report is trying to address:

Re: XMLHttpRequest: IE compatibility: null versus empty string for    
requesting missing, unsafe, or malformed header names
To: [e-mail]
Cc: [e-mail]
On Mon, Sep 21, 2009 at 6:57 PM,  <[e-mail]> wrote:
> IE 8.0 returns an empty string, except that it throws an exception if the
> header name argument is null.
> The standard that I read specifies null for the cases mentioned in the
> subject. Is this difference intentional?
> I personally prefer to have it the way the standard specifies the handling
> of these cases, as it allows a distinction between a header not being
> present and having an empty value, but I would like to know whether the
> current state of the standard is likely to remain as is, or it is likely to
> change to emulate IE, which while providing less information makes it easier
> for the developers to treat missing and empty headers the same way, which is
> probably ok in most cases.
>
> Any thoughts that you might have on this issue are greatly appreciated.

The standard at one point returned the empty string. We changed it for
exactly the reason you mention and I do not plan to change that. By
the way, it's better to use [e-mail] if you want to reach me
on such matters. [e-mail] is just for WebKit
Bugzilla.

Cheers,


-- 
Anne van Kesteren
http://annevankesteren.nl/

[Alexey Proskuryakov]

Comment on attachment 39698 [details]
Proposed patch

> -    if (!isValidToken(name))
> -        return "";
<...>
> +    //if name is not a "Valid token" it shall not match any valid header name thus the check for isValidToken is not needed.
> +    //since httpHeaderField(name) returns a null string anyway.
>      return m_response.httpHeaderField(name);

It seems fine to remove the check - I think that we should suggest removing this clause form XMLHttpRequest spec, too.

I do not think that the comment is helpful - it talks about code that used to be there, but isn't any more. Also, sentences should start with capital letters, and there should be a space after "//". I suggest just removing the comment, and e-mailing W3C WebApps working group about changing the spec.

> +        * http/tests/xmlhttprequest/xmlhttprequest-invalidHeader-getRequestHeader.html: Removed.
> +          getResponseHeader.html now covers this case as well.

You should remove expected results for this test, as well.

[Carol Szabo]

Created attachment 39943 [details]
Proposed Patch: Addresses AP's concerns.

[Alexey Proskuryakov]

Comment on attachment 39943 [details]
Proposed Patch: Addresses AP's concerns.

r=me

[Eric Seidel (no email)]

Comment on attachment 39943 [details]
Proposed Patch: Addresses AP's concerns.

Carol is not a committer, so assuming he wanted this auto-committed given no indication otherwise.

[WebKit Commit Bot]

Comment on attachment 39943 [details]
Proposed Patch: Addresses AP's concerns.

Rejecting patch 39943 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11318 test cases.
http/tests/xmlhttprequest/getResponseHeader.html -> failed

Exiting early after 1 failures. 8974 tests run.
252.41s total testing time

8973 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

[Eric Seidel (no email)]

The test failure looks related to your change, so this rejection is real (not a flakey test).

[Carol Szabo]

(In reply to comment #13)
> The test failure looks related to your change, so this rejection is real (not a
> flakey test).

Eric,
I have run this test in my environment and it succeeded. I think the output is apparently different on Mac than on Linux, I assume that there probably are extra warnings or something.
Can you provide the actual output of the failed test such that I can address the issue.

Thanks,
Carol

[Carol Szabo]

Created attachment 40076 [details]
Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now.

I fixed the expected test result for the failing test. The difference was in the output formatting.

[Eric Seidel (no email)]

OK.  Best to let ap set r+ again since he reviewed the original patch.

[Alexey Proskuryakov]

Comment on attachment 40076 [details]
Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now.

r=me

[WebKit Commit Bot]

Comment on attachment 40076 [details]
Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now.

Clearing flags on attachment: 40076

Committed r48739: <http://trac.webkit.org/changeset/48739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.