WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(4.70 KB, patch)
2013-02-11 09:06 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(6.00 KB, patch)
2013-02-14 07:55 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 186569
[details]
Patch
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
Created
attachment 187594
[details]
Patch
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
Created
attachment 188352
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug