After a 303 redirection, a HEAD request is changed into a GET request for EFL/GTK ports. This can be illustrated by loading http://www.mnot.net/javascript/xmlhttprequest/#mr.HEAD.301 On the contrary, QtWebKit, Chrome and Firefox keep using HEAD after a 303.
Created attachment 188905 [details] Patch
Comment on attachment 188905 [details] Patch Attachment 188905 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16629013 New failing tests: http/tests/xmlhttprequest/head-redirection.html media/video-controls-captions-trackmenu.html
Created attachment 189012 [details] Patch
Created attachment 189532 [details] Patch
Comment on attachment 189532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189532&action=review > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:423 > - if (message->method == SOUP_METHOD_GET) > + if (message->method == SOUP_METHOD_GET || message->method == SOUP_METHOD_HEAD) What requests should be changed to GET? Is this described in an RFC? I think this bit of code needs a comment, at least, with a reference to how we know this is the right decision.
(In reply to comment #5) > What requests should be changed to GET? Is this described in an RFC? I think this bit of code needs a comment, at least, with a reference to how we know this is the right decision. It's documented in the HTTP spec (RFC 2616), but in general, what web browsers do doesn't fully match that, and in specific, 2616 gets this case wrong (what it says is actually not even what the authors meant; it's been fixed in httpbis). Anyway, we know it's the right decision because it makes the tests pass. :-}
(In reply to comment #6) > It's documented in the HTTP spec (RFC 2616), but in general, what web browsers do doesn't fully match that, and in specific, 2616 gets this case wrong (what it says is actually not even what the authors meant; it's been fixed in httpbis). > > Anyway, we know it's the right decision because it makes the tests pass. :-} The test in this case is new. Perhaps a link to httbis in the comments is still in order. :)
sure, but everyone else already passes the new test, showing that it's right... httpbis is difficult to cite since it's not finished yet (and, eg, sections still might get renumbered). http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-21#section-7.4.4 contains the relevant bit that changed, but shouldRedirectAsGET combines multiple rules from different sections, so putting a link to there isn't really going to help. You could put a link to the whole 3xx response section, but you'd have to say something like "this is documented at ..., except for the parts where we don't do what they say, and we don't know why, other than that the other ports don't either".
Good point. Chrome and Firefox at least support that behavior. It is also made clearer in http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-21#section-7.4.4. I will add this link to the changelog and to the html test case.
Comment on attachment 189532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189532&action=review In general, this looks good to me. I've a few nits about the test. > LayoutTests/http/tests/xmlhttprequest/head-redirection-expected.txt:1 > +Test redirection codes with HEAD. I think you should include a longer description here. See LayoutTests/http/tests/misc/authentication-redirect-2/authentication-sent-to-redirect-same-origin-expected.txt for an example. There are also some issues with style in the test code. > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:28 > + if(hasError) { Missing a space here. > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:31 > + } > + else { should be } else { > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:32 > + var reqMethod = xhr.getResponseHeader('REQMETHOD'); Normally in WebKit we don't abbreviate variable names. > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:37 > + if(++testIndex<tests.length) { Missing a space here as well. > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:40 > + } > + else if (window.testRunner) { } else if {
Created attachment 189715 [details] Patch
Comment on attachment 189715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189715&action=review This looks good to me, but I'd rather have someone with a bit more HTTP test experience double-check the test. > LayoutTests/ChangeLog:9 > + Added tests checking HEAD redirection. > + Redirected HEAD requests are expected to remain HEAD requests. > + (http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-21#section-7.4) > + Marked test as failing for mac. > + Something funky happened here. The description should go under the "Reviewed by" line. > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:29 > + var testIndex=0; > + var tests = [ > + {testName:"HEAD-301",method: "HEAD", code:301, expectedMethod:"HEAD"}, > + {testName: "HEAD-302",method: "HEAD", code:302, expectedMethod:"HEAD"}, > + {testName:"HEAD-303",method: "HEAD", code:303, expectedMethod:"HEAD"}, > + {testName: "HEAD-307",method: "HEAD", code:307, expectedMethod:"HEAD"}, > + ]; Please use consistent spacing around operators and ':' In particular '=' should be surrounded by spaces. > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:44 > + if (++testIndex<tests.length) { Ditto.
ap, perhaps you'd be willing to double-check the sanity of this patch and, in particular, the new HTTP test. Sorry, if you aren't the right person to CC.
Comment on attachment 189715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189715&action=review > LayoutTests/platform/mac/TestExpectations:1448 > +# Test introduced for https://bugs.webkit.org/show_bug.cgi?id=110127 is failing on mac-wk2 > +http/tests/xmlhttprequest/head-redirection.html [ Failure ] This is strange. First, WebKit1 and WebKit2 should have the same behavior in this respect. Second, neither should have this bug.
Related to the mac test expectations, that may actually be a misinterpretation on my side. I thought that the mac-wk2 failure on the first patch was due to the new test. Checking the test log, the new test is not marked as failing, so it is probably actually fine. I cannot check it directly though. I will fix the test styles issues, remove the mac-wk2 test expectation line, resubmit the patch and check the EWS results.
Created attachment 194219 [details] Patch
Comment on attachment 194219 [details] Patch Attachment 194219 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17286126 New failing tests: http/tests/xmlhttprequest/head-redirection.html
Created attachment 194241 [details] Archive of layout-test-results from webkit-ews-11 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: <class 'webkitpy.common.config.ports.MacWK2Port'> Platform: Mac OS X 10.8.2
Comment on attachment 194219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194219&action=review > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:21 > + You seem to have an extra empty line here. > LayoutTests/http/tests/xmlhttprequest/resources/get_method.php:4 > + header("Access-Control-Allow-Origin: *"); Do you really need this header? All requests here seem to be same-origin ones.
(In reply to comment #18) > Created an attachment (id=194241) [details] > Archive of layout-test-results from webkit-ews-11 > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > Bot: webkit-ews-11 Port: <class 'webkitpy.common.config.ports.MacWK2Port'> Platform: Mac OS X 10.8.2 Does it mean mac-wk2 has the same problem as the soup backend?
(In reply to comment #19) > (From update of attachment 194219 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194219&action=review > > > LayoutTests/http/tests/xmlhttprequest/head-redirection.html:21 > > + > > You seem to have an extra empty line here. I will correct that > > > LayoutTests/http/tests/xmlhttprequest/resources/get_method.php:4 > > + header("Access-Control-Allow-Origin: *"); > > Do you really need this header? All requests here seem to be same-origin ones. I was initially planning to reuse that script for some additional CORS tests. Since it is not needed for that particular test case, I will remove it.
(In reply to comment #20) > (In reply to comment #18) > > Created an attachment (id=194241) [details] [details] > > Archive of layout-test-results from webkit-ews-11 > > > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > > Bot: webkit-ews-11 Port: <class 'webkitpy.common.config.ports.MacWK2Port'> Platform: Mac OS X 10.8.2 > > Does it mean mac-wk2 has the same problem as the soup backend? Here is the mac-wk2 log of this test: HEAD-301 - expected HEAD and received GET HEAD-302 - expected HEAD and received GET HEAD-303 - expected HEAD and received GET HEAD-307 - expected HEAD and received HEAD I do not have access to a mac platform so cannot go further than that.
Comment on attachment 194219 [details] Patch Attachment 194219 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17181697 New failing tests: http/tests/xmlhttprequest/head-redirection.html
Created attachment 194272 [details] Archive of layout-test-results from webkit-ews-07 The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.8.2
The test results for mac-wk2 and mac-ews are consistent. Redirected HEAD requests remain HEAD for 307 redirections but not for 301, 302 and 303 redirections. Looking at ResourceHandleCFNet.cpp, specific code enforces using the same method after 307 redirections. Changing the behavior could be done there. Doing it at a lower level may make more sense? To go on with the current patch, I can add a mac specific test expected file or skip the test for mac. Option 1 has the benefit of ensuring that no regression happens on the current behavior. Any suggestion?
> I can add a mac specific test expected file Yes, please.
Created attachment 194585 [details] Patch
Thank you! Looks good to me. Is it intentional that the patch is not current;y marked for review?
Is everybody fine with the patch?
(In reply to comment #29) > Is everybody fine with the patch? I'm happy with the soup bits, at least. :)
Comment on attachment 194585 [details] Patch Clearing flags on attachment: 194585 Committed r147914: <http://trac.webkit.org/changeset/147914>
All reviewed patches have been landed. Closing bug.