Bug 29121 – XMLHttpRequest::getAllResponseHeaders throws INVALID_STATE_ERR when called in state HEADERS_RECEIVED

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Carol Szabo]

Despite http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-getallresponseheaders-method mentioning explicitely and with an example that XMLHttpRequest::getAllResponseHeaders should return the headers in readyState HEADERS_RECEIVED. The Webkit implementation still throws an exception in this scenario.

[Carol Szabo]

Created attachment 39341 [details]
Example test case

This is an example that reproduces the problem. just unzip the archive in any virtual directory of an http server and access the included garhs.htm from a Webkit browser.

[Carol Szabo]

Created attachment 39349 [details]
Proposed patch

[WebKit Commit Bot]

Comment on attachment 39349 [details]
Proposed patch

Rejecting patch 39349 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Sam Weinig]

Comment on attachment 39349 [details]
Proposed patch

This should really also fix getResponseHeader which has the same bug.

[WebKit Commit Bot]

Comment on attachment 39349 [details]
Proposed patch

Rejecting patch 39349 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Carol Szabo]

Created attachment 39362 [details]
New Patch proposal to address sam's concerns

Despite the fact that this bug does not cover the similar problem with getResponseHeader, in the interest of saving time and satisfying reviewers, I fixed both bugs here.

[Eric Seidel (no email)]

fast/xmlhttprequest/xmlhttprequest-getallheaders.html -> failed
was the test that failed when trying to land attachment 39349 [details].

[Carol Szabo]

(In reply to comment #7)
> fast/xmlhttprequest/xmlhttprequest-getallheaders.html -> failed
> was the test that failed when trying to land attachment 39349 [details].

Eric, can you attach the actual output from the failed tests to the bug so that I can improve the tests. I know the tests have some weaknesses regarding portability, but I'd like to see what exactly went wrong in your environment.
Thanks,
Carol

[Carol Szabo]

Created attachment 39437 [details]
Modified tests to make them more portable

My previous patch had at least one of the new tests fail according to Eric. I have modified the tests to make them less dependent on the environment.
Eric,
Can you see if these tests work for you and can you add this patch to the commit queue?
Thanks,
Carol

[Eric Seidel (no email)]

These were the difffs from the commit-queue's failure:
--- /tmp/layout-test-results/fast/xmlhttprequest/xmlhttprequest-getallheaders-expected.txt      2009-09-10 10:07:46.000000000 -0700
+++ /tmp/layout-test-results/fast/xmlhttprequest/xmlhttprequest-getallheaders-actual.txt        2009-09-10 10:07:46.000000000 -0700
@@ -8,5 +8,4 @@
  Header name and Header value is separated by COLON and SPACE.
  Set-Cookie and Set-Cookie2 Headers with any ASCII case-insensitive match NOT displayed.
 SUCCEEDED: getAllResponseHeaders returned:
-Cache-Control: no-cache

[Carol Szabo]

(In reply to comment #10)
> These were the difffs from the commit-queue's failure:
> ---
> /tmp/layout-test-results/fast/xmlhttprequest/xmlhttprequest-getallheaders-expected.txt
>      2009-09-10 10:07:46.000000000 -0700
> +++
> /tmp/layout-test-results/fast/xmlhttprequest/xmlhttprequest-getallheaders-actual.txt
>        2009-09-10 10:07:46.000000000 -0700
> @@ -8,5 +8,4 @@
>   Header name and Header value is separated by COLON and SPACE.
>   Set-Cookie and Set-Cookie2 Headers with any ASCII case-insensitive match NOT
> displayed.
>  SUCCEEDED: getAllResponseHeaders returned:
> -Cache-Control: no-cache

As I expected, the difference is in the headers that get returned.
In your environment, there are no headers returned for the "file" scheme, where as in mine there is one header.
I fixed this problem, by not displaying the headers if the test is ran with DumpRenderTree, just analyze them for problems. The scope of these tests is anyway just to make sure that no INVALID_STATE exception is thrown.
Please verify my new patch which only differs in the test area from the previous.

[Alexey Proskuryakov]

(In reply to comment #0)
> Despite
> http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-getallresponseheaders-method
> mentioning explicitely and with an example that
> XMLHttpRequest::getAllResponseHeaders should return the headers in readyState
> HEADERS_RECEIVED. The Webkit implementation still throws an exception in this
> scenario.

Have you tested IE and Firefox? Please do.

> In your environment, there are no headers returned for the "file" scheme, where
> as in mine there is one header.

I suggest making these HTTP tests. Asking for HTTP response headers of a file:/// resource doesn't make much sense, and there is no specification for XHR operating on such.

[Eric Seidel (no email)]

Comment on attachment 39437 [details]
Modified tests to make them more portable

Marking cq=? w/o marking r=? makes little sense.  Also, ap seems to have objections to this patch, so removing cq?

[Carol Szabo]

(In reply to comment #12)
> (In reply to comment #0)
> > Despite
> > http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-getallresponseheaders-method
> > mentioning explicitely and with an example that
> > XMLHttpRequest::getAllResponseHeaders should return the headers in readyState
> > HEADERS_RECEIVED. The Webkit implementation still throws an exception in this
> > scenario.
> 
> Have you tested IE and Firefox? Please do.

Firefox 3.0.8 works per the standard in this regard.
IE 6 does not have the concept of readyState
IE 7 and IE 8 have a concept of readyState, but it is not the same as the one specified in the standard, and thus IE has a different behavior than WebKit regardless of whether I put my change in or not. IE only gives access to headers when the whole load finished, which is not what WebKit did before my fix.


> I suggest making these HTTP tests. Asking for HTTP response headers of a
> file:/// resource doesn't make much sense, and there is no specification for
> XHR operating on such.

I agree, I will do that.

[Carol Szabo]

Created attachment 39471 [details]
Proposed patch. Moved tests to http/tests/xmlhttprequest from fast/xmlhttprequest

Also I have addressed ap's concerns.

[Alexey Proskuryakov]

Comment on attachment 39471 [details]
Proposed patch. Moved tests to http/tests/xmlhttprequest from fast/xmlhttprequest

FWIW, our current behavior is based on research summarized in bug 15356 comment 7.

+If the test succeeds one should see below the ruller the text "SUCCEEDED: getAllResponseHeaders returned: " followed by a conforming list of headers.

This text looks outdated. Same issue in another test.

We have a lot of data files in xmlhttprequest/resources already, do the new files really need to be added?

Looks good to me, r- to update the tests. It would also be great if you could review the discussion in bug 15356, just in case it has some important details about other browsers' behavior.

[Carol Szabo]

Comment on attachment 39471 [details]
Proposed patch. Moved tests to http/tests/xmlhttprequest from fast/xmlhttprequest

Alexei,
The text your are mentioning is intentionally left that way. If the test is run interactively, that is in a regular browser, not with DumpRenderTree the list of headers and the the value of the header respectively are still displayed. Also regardless of the user agent the very next paragraph after the one you mentioned explains that the header list and the header value respectively are omitted for DumpRenderTree to prevent the failure of the unattended tests.
I have read the discussion that you mentioned. I believe that my change which leads to an exception not being thrown, not to extra exceptions being thrown, increases (a little bit, not very much) the likelihood that a webpage will look as intended, as opposed to the change discussed in bug 15356, apparently would create a more restrictive behavior not a more lenient one. The risk with my change is that pages may be created that work only on Mozilla and WebKit or only on WebKit which is not that bad.
I know that somebody may rely on an exception being thrown instead of the header value being returned, but I think that that use case may be discounted.

[Alexey Proskuryakov]

(In reply to comment #17)
> The text your are mentioning is intentionally left that way. If the test is run
> interactively, that is in a regular browser, not with DumpRenderTree the list
> of headers and the the value of the header respectively are still displayed.

I like that the headers are fully dumped when the test is run in Safari, it can be very helpful.

But this text is confusing nonetheless. There is no reason to leave it in this state - it's easy to have different output for DRT. In fact, we often do the following for DRT-only tests:

if (!window.layoutTestController)
    document.write("<p>This test only works in run-webkit-tests</p>");

Since this code has a long history of changes, it is particularly important not to leave any confusing comments around it.

> I have read the discussion that you mentioned. I believe that my change which
> leads to an exception not being thrown, not to extra exceptions being thrown,

Yes, that's correct. I just wanted to give some additional context, as it appears that at least some browsers' behavior has changed since 2007.

[Carol Szabo]

Created attachment 39547 [details]
Improved text in the tests per ap's comments.

[Alexey Proskuryakov]

Comment on attachment 39547 [details]
Improved text in the tests per ap's comments.

This still includes old test results, so a committer will need to regenerate them - or you could submit a new patch to simplify committer's job.

+    <p>If the test succeeds one should see below the ruller the text

A typo, there is one "l" in "ruler".

r=me

[Carol Szabo]

Created attachment 39564 [details]
Fixed a few minor issues with the tests and some regretable mistakes in the test results.

[Alexey Proskuryakov]

I tried to land this, but tests don't pass on Mac OS X Leopard:

file:///tmp/layout-test-results/http/tests/xmlhttprequest/xmlhttprequest-InvalidStateException-getAllRequestHeaders-diffs.txt
-PASSED (INVALID_STATE_ERR: DOM Exception 11 ) 2
+FAILED ( header :Date: Tue, 15 Sep 2009 21:12:19 GMT

file:///tmp/layout-test-results/http/tests/xmlhttprequest/xmlhttprequest-InvalidStateException-getRequestHeader-diffs.txt
-PASSED (INVALID_STATE_ERR: DOM Exception 11 ) 2
+FAILED ( header :text/html)2

When running the test manually, it also looks wrong:

SUCCEEDED: getResponseHeader returned: null

[Carol Szabo]

Created attachment 39621 [details]
New patch that fixes problems with failing old http tests

This patch includes renaming of the test and expected results files. I believe it will be hard to read please advise on a better way to submit it if this method is not acceptable.
Thanks.

[Alexey Proskuryakov]

Comment on attachment 39621 [details]
New patch that fixes problems with failing old http tests

> This patch includes renaming of the test and expected results files. I believe
> it will be hard to read please advise on a better way to submit it if this
> method is not acceptable.

It's actually fine, we do get a diff from old to new in such patches.

r=me, and let's see if the bot manages to land this...

[WebKit Commit Bot]

Comment on attachment 39621 [details]
New patch that fixes problems with failing old http tests

Clearing flags on attachment: 39621

Committed r48433: <http://trac.webkit.org/changeset/48433>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.