WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
110127
[GTK][EFL] HEAD requests changed to GET after 303 redirection
https://bugs.webkit.org/show_bug.cgi?id=110127
Summary
[GTK][EFL] HEAD requests changed to GET after 303 redirection
youenn fablet
Reported
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.
Attachments
Patch
(6.84 KB, patch)
2013-02-18 09:30 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(7.68 KB, patch)
2013-02-19 00:29 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(6.76 KB, patch)
2013-02-21 08:13 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(7.94 KB, patch)
2013-02-22 01:30 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(7.33 KB, patch)
2013-03-21 03:51 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-11
(481.59 KB, application/zip)
2013-03-21 05:49 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-07
(612.24 KB, application/zip)
2013-03-21 08:53 PDT
,
Build Bot
no flags
Details
Patch
(8.40 KB, patch)
2013-03-22 10:26 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2013-02-18 09:30:54 PST
Created
attachment 188905
[details]
Patch
Build Bot
Comment 2
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
youenn fablet
Comment 3
2013-02-19 00:29:45 PST
Created
attachment 189012
[details]
Patch
youenn fablet
Comment 4
2013-02-21 08:13:36 PST
Created
attachment 189532
[details]
Patch
Martin Robinson
Comment 5
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.
Dan Winship
Comment 6
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. :-}
Martin Robinson
Comment 7
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. :)
Dan Winship
Comment 8
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".
youenn fablet
Comment 9
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.
Martin Robinson
Comment 10
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 {
youenn fablet
Comment 11
2013-02-22 01:30:02 PST
Created
attachment 189715
[details]
Patch
Martin Robinson
Comment 12
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.
Martin Robinson
Comment 13
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.
Alexey Proskuryakov
Comment 14
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.
youenn fablet
Comment 15
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.
youenn fablet
Comment 16
2013-03-21 03:51:16 PDT
Created
attachment 194219
[details]
Patch
Build Bot
Comment 17
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
Build Bot
Comment 18
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
Raphael Kubo da Costa (:rakuco)
Comment 19
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.
Raphael Kubo da Costa (:rakuco)
Comment 20
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?
youenn fablet
Comment 21
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.
youenn fablet
Comment 22
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.
Build Bot
Comment 23
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
Build Bot
Comment 24
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
youenn fablet
Comment 25
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?
Alexey Proskuryakov
Comment 26
2013-03-22 09:30:39 PDT
> I can add a mac specific test expected file
Yes, please.
youenn fablet
Comment 27
2013-03-22 10:26:24 PDT
Created
attachment 194585
[details]
Patch
Alexey Proskuryakov
Comment 28
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?
youenn fablet
Comment 29
2013-03-28 03:58:34 PDT
Is everybody fine with the patch?
Martin Robinson
Comment 30
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. :)
WebKit Commit Bot
Comment 31
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
>
WebKit Commit Bot
Comment 32
2013-04-08 08:25:14 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug