[EFL][DRT] http/tests/loading/307-after-303-after-post.html times out

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Joone Hur]

http/tests/loading/307-after-303-after-post.html times out on EFL64bit-debug-WK1

[Raphael Kubo da Costa (:rakuco)]

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

[youenn fablet]

Created attachment 186569 [details]
Patch

[Dan Winship]

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]

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]

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]

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]

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

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]

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]

Created attachment 187594 [details]
Patch

[youenn fablet]

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 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]

> 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]

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

[Sergio Villar Senin]

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]

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

Created attachment 188352 [details]
Patch

[youenn fablet]

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 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]

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

(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 on attachment 188352 [details]
Patch

Clearing flags on attachment: 188352

Committed r143352: <http://trac.webkit.org/changeset/143352>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.