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.
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.
Created attachment 39349 [details] Proposed patch
Comment on attachment 39349 [details] Proposed patch Rejecting patch 39349 from commit-queue. carol.szabo@nokia.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
Comment on attachment 39349 [details] Proposed patch This should really also fix getResponseHeader which has the same bug.
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
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.
fast/xmlhttprequest/xmlhttprequest-getallheaders.html -> failed was the test that failed when trying to land attachment 39349 [details].
(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
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
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
(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.
(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.
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?
(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.
Created attachment 39471 [details] Proposed patch. Moved tests to http/tests/xmlhttprequest from fast/xmlhttprequest Also I have addressed ap's concerns.
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.
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.
(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.
Created attachment 39547 [details] Improved text in the tests per ap's comments.
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
Created attachment 39564 [details] Fixed a few minor issues with the tests and some regretable mistakes in the test results.
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
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.
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...
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>
All reviewed patches have been landed. Closing bug.