Bug 29140 - getResponseHeader returns "" instead of null as the spec requires.
Summary: getResponseHeader returns "" instead of null as the spec requires.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-09-10 12:16 PDT by Carol Szabo
Modified: 2009-09-24 16:44 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (15.79 KB, patch)
2009-09-17 09:41 PDT, Carol Szabo
ap: review-
Details | Formatted Diff | Diff
Proposed Patch: Addresses AP's concerns. (16.14 KB, patch)
2009-09-22 14:14 PDT, Carol Szabo
ap: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now. (16.14 KB, patch)
2009-09-24 11:26 PDT, Carol Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carol Szabo 2009-09-10 12:16:22 PDT
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.
Comment 1 Carol Szabo 2009-09-10 12:16:59 PDT
Intend to submit patch soon.
Comment 2 Carol Szabo 2009-09-14 09:36:27 PDT
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.
Comment 3 Alexey Proskuryakov 2009-09-14 11:13:11 PDT
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.
Comment 4 Alexey Proskuryakov 2009-09-14 11:13:40 PDT
(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 :)
Comment 5 Carol Szabo 2009-09-17 09:41:26 PDT
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).
Comment 6 Carol Szabo 2009-09-21 08:35:49 PDT
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.
Comment 7 Carol Szabo 2009-09-22 10:28:19 PDT
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: carol.szabo@nokia.com
Cc: ap@webkit.org
On Mon, Sep 21, 2009 at 6:57 PM,  <carol.szabo@nokia.com> 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 annevk@opera.com if you want to reach me
on such matters. annevankesteren+webkit@gmail.com is just for WebKit
Bugzilla.

Cheers,


-- 
Anne van Kesteren
http://annevankesteren.nl/
Comment 8 Alexey Proskuryakov 2009-09-22 12:50:42 PDT
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.
Comment 9 Carol Szabo 2009-09-22 14:14:02 PDT
Created attachment 39943 [details]
Proposed Patch: Addresses AP's concerns.
Comment 10 Alexey Proskuryakov 2009-09-22 16:36:09 PDT
Comment on attachment 39943 [details]
Proposed Patch: Addresses AP's concerns.

r=me
Comment 11 Eric Seidel (no email) 2009-09-23 18:01:45 PDT
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.
Comment 12 WebKit Commit Bot 2009-09-23 18:08:12 PDT
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
Comment 13 Eric Seidel (no email) 2009-09-23 18:16:22 PDT
The test failure looks related to your change, so this rejection is real (not a flakey test).
Comment 14 Carol Szabo 2009-09-24 06:00:26 PDT
(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
Comment 15 Carol Szabo 2009-09-24 11:26:02 PDT
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.
Comment 16 Eric Seidel (no email) 2009-09-24 13:03:24 PDT
OK.  Best to let ap set r+ again since he reviewed the original patch.
Comment 17 Alexey Proskuryakov 2009-09-24 16:32:13 PDT
Comment on attachment 40076 [details]
Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now.

r=me
Comment 18 WebKit Commit Bot 2009-09-24 16:44:25 PDT
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>
Comment 19 WebKit Commit Bot 2009-09-24 16:44:30 PDT
All reviewed patches have been landed.  Closing bug.