Bug 110127 – [GTK][EFL] HEAD requests changed to GET after 303 redirection

• Component: WebKit EFL
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

Created attachment 188905 [details]
Patch

[Build Bot]

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

[youenn fablet]

Created attachment 189012 [details]
Patch

[youenn fablet]

Created attachment 189532 [details]
Patch

[Martin Robinson]

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.

[Dan Winship]

(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. :-}

[Martin Robinson]

(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. :)

[Dan Winship]

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".

[youenn fablet]

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.

[Martin Robinson]

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 {

[youenn fablet]

Created attachment 189715 [details]
Patch

[Martin Robinson]

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.

[Martin Robinson]

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.

[Alexey Proskuryakov]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 194219 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

(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?

[youenn fablet]

(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.

[youenn fablet]

(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.

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

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?

[Alexey Proskuryakov]

> I can add a mac specific test expected file

Yes, please.

[youenn fablet]

Created attachment 194585 [details]
Patch

[Alexey Proskuryakov]

Thank you! Looks good to me.

Is it intentional that the patch is not current;y marked for review?

[youenn fablet]

Is everybody fine with the patch?

[Martin Robinson]

(In reply to comment #29)
> Is everybody fine with the patch?

I'm happy with the soup bits, at least. :)

[WebKit Commit Bot]

Comment on attachment 194585 [details]
Patch

Clearing flags on attachment: 194585

Committed r147914: <http://trac.webkit.org/changeset/147914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.