Bug 110127

Summary: [GTK][EFL] HEAD requests changed to GET after 303 redirection
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, buildbot, commit-queue, danw, gns, lucas.de.marchi, mrobinson, rakuco, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-11
none
Archive of layout-test-results from webkit-ews-07
none
Patch none

Description youenn fablet 2013-02-18 09:00:46 PST
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.
Comment 1 youenn fablet 2013-02-18 09:30:54 PST
Created attachment 188905 [details]
Patch
Comment 2 Build Bot 2013-02-18 12:43:13 PST
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
Comment 3 youenn fablet 2013-02-19 00:29:45 PST
Created attachment 189012 [details]
Patch
Comment 4 youenn fablet 2013-02-21 08:13:36 PST
Created attachment 189532 [details]
Patch
Comment 5 Martin Robinson 2013-02-21 08:19:42 PST
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.
Comment 6 Dan Winship 2013-02-21 09:18:38 PST
(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. :-}
Comment 7 Martin Robinson 2013-02-21 09:27:44 PST
(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. :)
Comment 8 Dan Winship 2013-02-21 09:58:36 PST
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".
Comment 9 youenn fablet 2013-02-21 10:07:41 PST
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 10 Martin Robinson 2013-02-21 10:13:02 PST
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 {
Comment 11 youenn fablet 2013-02-22 01:30:02 PST
Created attachment 189715 [details]
Patch
Comment 12 Martin Robinson 2013-03-19 13:21:00 PDT
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.
Comment 13 Martin Robinson 2013-03-19 13:22:00 PDT
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 14 Alexey Proskuryakov 2013-03-19 13:36:33 PDT
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.
Comment 15 youenn fablet 2013-03-19 14:12:21 PDT
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.
Comment 16 youenn fablet 2013-03-21 03:51:16 PDT
Created attachment 194219 [details]
Patch
Comment 17 Build Bot 2013-03-21 05:49:14 PDT
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
Comment 18 Build Bot 2013-03-21 05:49:16 PDT
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 19 Raphael Kubo da Costa (:rakuco) 2013-03-21 06:08:49 PDT
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.
Comment 20 Raphael Kubo da Costa (:rakuco) 2013-03-21 06:09:54 PDT
(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?
Comment 21 youenn fablet 2013-03-21 06:21:12 PDT
(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.
Comment 22 youenn fablet 2013-03-21 06:25:01 PDT
(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 23 Build Bot 2013-03-21 08:53:05 PDT
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
Comment 24 Build Bot 2013-03-21 08:53:08 PDT
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
Comment 25 youenn fablet 2013-03-22 06:30:43 PDT
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?
Comment 26 Alexey Proskuryakov 2013-03-22 09:30:39 PDT
> I can add a mac specific test expected file

Yes, please.
Comment 27 youenn fablet 2013-03-22 10:26:24 PDT
Created attachment 194585 [details]
Patch
Comment 28 Alexey Proskuryakov 2013-03-22 11:06:11 PDT
Thank you! Looks good to me.

Is it intentional that the patch is not current;y marked for review?
Comment 29 youenn fablet 2013-03-28 03:58:34 PDT
Is everybody fine with the patch?
Comment 30 Martin Robinson 2013-03-28 06:17:02 PDT
(In reply to comment #29)
> Is everybody fine with the patch?

I'm happy with the soup bits, at least. :)
Comment 31 WebKit Commit Bot 2013-04-08 08:25:10 PDT
Comment on attachment 194585 [details]
Patch

Clearing flags on attachment: 194585

Committed r147914: <http://trac.webkit.org/changeset/147914>
Comment 32 WebKit Commit Bot 2013-04-08 08:25:14 PDT
All reviewed patches have been landed.  Closing bug.