RESOLVED FIXED 93214
[EFL][DRT] http/tests/loading/307-after-303-after-post.html times out
https://bugs.webkit.org/show_bug.cgi?id=93214
Summary [EFL][DRT] http/tests/loading/307-after-303-after-post.html times out
Joone Hur
Reported 2012-08-05 18:19:26 PDT
http/tests/loading/307-after-303-after-post.html times out on EFL64bit-debug-WK1
Attachments
Patch (3.57 KB, patch)
2013-02-05 00:57 PST, youenn fablet
no flags
Patch (4.70 KB, patch)
2013-02-11 09:06 PST, youenn fablet
no flags
Patch (6.00 KB, patch)
2013-02-14 07:55 PST, youenn fablet
no flags
Raphael Kubo da Costa (:rakuco)
Comment 1 2012-09-25 03:42:34 PDT
For the record: as of r129481, it is failing on the WK2 bot with [ Failure Timeout Pass ].
youenn fablet
Comment 2 2013-02-05 00:57:37 PST
Dan Winship
Comment 3 2013-02-05 06:45:43 PST
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?
youenn fablet
Comment 4 2013-02-08 07:31:56 PST
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.
Dan Winship
Comment 5 2013-02-08 10:12:46 PST
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.
Dan Winship
Comment 6 2013-02-08 11:07:04 PST
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.
Martin Robinson
Comment 7 2013-02-08 11:35:35 PST
(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?
youenn fablet
Comment 8 2013-02-10 09:51:36 PST
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?
Dan Winship
Comment 9 2013-02-10 10:51:39 PST
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...)
youenn fablet
Comment 10 2013-02-11 09:06:39 PST
youenn fablet
Comment 11 2013-02-11 09:09:26 PST
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.
Martin Robinson
Comment 12 2013-02-11 10:01:56 PST
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?
youenn fablet
Comment 13 2013-02-11 10:37:15 PST
> 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).
Dan Winship
Comment 14 2013-02-12 06:44:24 PST
*** Bug 109576 has been marked as a duplicate of this bug. ***
Sergio Villar Senin
Comment 15 2013-02-12 07:29:25 PST
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?
Dan Winship
Comment 16 2013-02-12 07:57:20 PST
(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.
youenn fablet
Comment 17 2013-02-14 07:55:23 PST
youenn fablet
Comment 18 2013-02-14 08:00:52 PST
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.
Sergio Villar Senin
Comment 19 2013-02-14 08:30:35 PST
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
youenn fablet
Comment 20 2013-02-15 00:12:34 PST
(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?
Sergio Villar Senin
Comment 21 2013-02-18 03:19:03 PST
(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
WebKit Review Bot
Comment 22 2013-02-19 10:29:52 PST
Comment on attachment 188352 [details] Patch Clearing flags on attachment: 188352 Committed r143352: <http://trac.webkit.org/changeset/143352>
WebKit Review Bot
Comment 23 2013-02-19 10:29:58 PST
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.