Bug 93214

For the record: as of r129481, it is failing on the WK2 bot with [ Failure Timeout Pass ].

Created attachment 186569 [details] Patch

Comment on attachment 186569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186569&action=review > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:425 > if (message->method == SOUP_METHOD_GET) > - return false; > + return request.httpMethod() != "GET"; Why are message->method and request.httpMethod() different?

request->httpMethod() is set from POST to GET after the 303 redirection in doRedirect. But handle->firstRequest().httpMethod() is left unchanged. On the next call to doRedirect, request->httpMethod() will be again set to POST. message and message->method are persistent over the redirection response. After the 303 redirection, message->method is set to GET and its value will remain as is.

ah... kov and I were talking about this at one point... There's a lot of code that's using firstRequest when it really means "mostRecentRequest". I tried fixing this at one point but didn't get very far. But anyway, that's the problem here; the logic in shouldRedirectAsGET() is correct, it's just that "request" was initialized with the wrong method. So I think the right fix is to add request.setHTTPMethod(message->method); right after the request.setURL() line.

ok, actually that won't work because firstRequest will still have the request body too... I guess your patch works, but the underlying problem is that we're using firstRequest to initialize the new request, and we really should be using the request that we sent last time instead.

(In reply to comment #6) > ok, actually that won't work because firstRequest will still have the request body too... > > I guess your patch works, but the underlying problem is that we're using firstRequest to initialize the new request, and we really should be using the request that we sent last time instead. Could we fix this problem by simply storing that request?

Ah, I thought keeping firstRequest as is was part of the design. I will look at this again with that in mind. Also, would it make sense to try renaming "firstRequest" in the source code?

firstRequest is part of the platform-independent part of ResourceHandle, and there are places where it's used where they really do want the first request. (I think...)

Created attachment 187594 [details] Patch

After reading the code again, I would tend to stick with the current approach. Storing a modified ResourceRequest is deviating from other port strategies. It may also take more work to keep in sync things (referer, auth..) between the hops than go from firstRequest to the latest hop. In the newly submitted patch, shouldRedirectAsGET is not modified. The check is directly added in doRedirect. The check is a bit looser than in the previous patch but I hope it is fine as is. I also renamed request in newRequest, in the hope it makes the code clearer. In the same vein, there is some code that looks a bit suspicious in resourceHandleSoup.cpp/handleUnignoredTLSErrors. The host name that is used to retrieve certificates (that are not CA validated but granted by user) is coming from firstRequest. To properly handle redirections, it may be better to get it directly from the soup layer.

Comment on attachment 187594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187594&action=review Perhaps it makes sense to unskip this test on GTK+ as well? > Source/WebCore/ChangeLog:5 > + [EFL][DRT] Ensured that GET verb is used for requests > + triggered from a succession of 307 and other 3XX responses. > + https://bugs.webkit.org/show_bug.cgi?id=93214 The first line should be the bug title and then the description should go underneath. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:457 > - ResourceRequest request = handle->firstRequest(); > + ResourceRequest newRequest = handle->firstRequest(); I like this change a lot! > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:468 > + if (shouldRedirectAsGET(message, newURL, crossOrigin) || newRequest.httpMethod() != message->method) { > + newRequest.setHTTPMethod("GET"); > + newRequest.setHTTPBody(0); > + newRequest.clearHTTPContentType(); > } Hrm. Shouldn't this restore the original method instead of just unconditionally using GET? In any case, why not roll this into shouldRedirectAsGET?

> Perhaps it makes sense to unskip this test on GTK+ as well? On GTK+, there is a bug in the DRT that prevents printing the expected output (https://bugs.webkit.org/show_bug.cgi?id=35300) Once this bug is solved, it should be fine to unskip it. > The first line should be the bug title and then the description should go underneath. I will change this when reposting the patch. > Hrm. Shouldn't this restore the original method instead of just unconditionally using GET? In any case, why not roll this into shouldRedirectAsGET? It's right that the check is a bit loose. In theory, we should check equality of the methods only if the soup request is a GET. But in practice for HTTP, the methods will be different only if a redirect enforces the verb to go from value to GET. Should it be tightened? Concerning shouldRedirectAsGET, the test was included in the previous patch. I received the feedback that it would be better to decouple the pure http logic (implemented in shouldRedirectAsGET) from this test which is related to the implementation strategy (always start from the original request).

*** Bug 109576 has been marked as a duplicate of this bug. ***

I worked on this same issue in bug 109576. I replaced the implementation of shouldRedirecAsGET() by a more simpler version, basically + if (((message->status_code == SOUP_STATUS_MOVED_PERMANENTLY) + || (message->status_code == SOUP_STATUS_FOUND) + || (message->status_code == SOUP_STATUS_SEE_OTHER)) + && (request.httpMethod() == "POST")) return true; Do we really need all the checks we have right now?

(In reply to comment #15) > Do we really need all the checks we have right now? Yes. WebKit's tests don't test all of the possibilities. Removing some of those cases would break web pages.

Created attachment 188352 [details] Patch

http://greenbytes.de/tech/tc/httpredirects/ and http://www.mnot.net/javascript/xmlhttprequest/#mr.HEAD.301 are good pointers for shouldRedirectAsGET logic. I plan to work on adding some webkit tests based on that work. HEAD redirections could be improved for instance.

Comment on attachment 188352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188352&action=review > LayoutTests/platform/efl/TestExpectations:763 > +webkit.org/b/93214 http/tests/loading/307-after-303-after-post.html [ Crash Pass Timeout ] Remove it also from Gtk's TestExpectations

(In reply to comment #19) > (From update of attachment 188352 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188352&action=review > > > LayoutTests/platform/efl/TestExpectations:763 > > +webkit.org/b/93214 http/tests/loading/307-after-303-after-post.html [ Crash Pass Timeout ] > > Remove it also from Gtk's TestExpectations The test is failing with Gtk/wk1 (DRT is not printing the initial resource URL correctly, see also bug 35300). For Gtk/wk2, the test is failing, but that may be my setup (additional favicon.ico lines are printed..). Are you suggesting to create an expected test result file to pass the tests?

(In reply to comment #20) > (In reply to comment #19) > > (From update of attachment 188352 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=188352&action=review > > > > > LayoutTests/platform/efl/TestExpectations:763 > > > +webkit.org/b/93214 http/tests/loading/307-after-303-after-post.html [ Crash Pass Timeout ] > > > > Remove it also from Gtk's TestExpectations > > The test is failing with Gtk/wk1 (DRT is not printing the initial resource URL correctly, see also bug 35300). > For Gtk/wk2, the test is failing, but that may be my setup (additional favicon.ico lines are printed..). > Are you suggesting to create an expected test result file to pass the tests? Yeah you're right, let's forget about WK1. Let's try to land this ASAP

Comment on attachment 188352 [details] Patch Clearing flags on attachment: 188352 Committed r143352: <http://trac.webkit.org/changeset/143352>

All reviewed patches have been landed. Closing bug.