Bug 88961

Summary: [GTK][EFL] http/tests/misc/redirect-to-about-blank.html is failing
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Dan Winship <danw>
Status: RESOLVED FIXED    
Severity: Normal CC: danw, dglazkov, d-r, gustavo, gyuyoung.kim, gyuyoung.kim, laszlo.gombos, lucas.de.marchi, mrobinson, pnormand, rakuco, sw0524.lee, webkit.review.bot, zan
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 104528    
Attachments:
Description Flags
Patch
none
Patch
none
patch - handle redirection inside WebKit
gustavo: review+, gustavo: commit-queue-
updated for comments none

Description Chris Dumez 2012-06-12 22:50:36 PDT
http/tests/misc/redirect-to-about-blank.html is failing after r120145 (Bug 88760).

It is likely caused by the libsoup update to v2.39.2 and not by the httpOnly cookie fix itself. I'll investigate it.
Comment 1 Chris Dumez 2012-06-12 23:00:29 PDT
Skipping test for GTK and EFL port in Bug 88962. Will unskip them later with a proper fix.
Comment 2 Chris Dumez 2012-06-13 00:13:39 PDT
For some reason, our restartedCallback does not get called even though our gotHeadersCallback gets called with a 302 code.
Comment 3 Zan Dobersek 2012-06-13 01:58:33 PDT
I went on and bisected libsoup changes, narrowing down the offending commit to this:
http://git.gnome.org/browse/libsoup/commit/?id=9effb5ca942412ecde9242c745f2df6da80853a3

Brave new world it is.
Comment 4 Chris Dumez 2012-06-13 03:36:21 PDT
(In reply to comment #3)
> I went on and bisected libsoup changes, narrowing down the offending commit to this:
> http://git.gnome.org/browse/libsoup/commit/?id=9effb5ca942412ecde9242c745f2df6da80853a3
> 
> Brave new world it is.

Thanks for bisecting Zan :)
I hope Dan can help on figuring out what goes wrong.
Comment 5 Dan Winship 2012-06-13 04:03:46 PDT
Oops. I thought I had filed a bug about this.

It's not actually a regression; ResourceHandleSoup has never been able to redirect from http: to about:, it's just that with old libsoup it failed in a way that made the test case believe it had succeeded, and with new libsoup it fails in a slightly different way that causes the test case to fail. (If you load misc/redirect-to-about-blank.html under GtkLauncher with old libsoup, you'll see that the test actually ends up with an error page in the iframe, not about:blank. And then it claims to have succeeded...)


The specific behavior change is that with old libsoup, when it got a redirect to a non-http URI, it would change the status of the SoupMessage to SOUP_STATUS_MALFORMED, which would then cause ResourceHandleSoup to generate an error page (which apparently has a URL of "about:blank", causing the test to succeed). With new libsoup, when it gets a redirect it can't follow, it just returns the response as-is, with the existing 302 (or whatever) status code (meaning the test ends up with redirect-to-about-blank.php in the iframe). The reason for the change is that leaving the 302 status code there allows for the possibility of webkit deciding to redirect the response itself (which is probably also necessary for fixing various other redirect-related test failures).

(See also https://bugzilla.gnome.org/show_bug.cgi?id=656684#c36)
Comment 6 Chris Dumez 2012-06-13 06:46:05 PDT
Created attachment 147308 [details]
Patch

I have unsure this is the correct way to fix it but at least it works and it does not seem to cause any regressions.
For now, the patch only fixes EFL port and does not unskip the associated test (because the patch to skip it has not landed yet).

Could someone please tell me if this is correct? and provide some guidance if it isn't.
Comment 7 Chris Dumez 2012-06-13 07:00:15 PDT
Created attachment 147309 [details]
Patch

Rebase on master and unskip associated test.
Comment 8 Martin Robinson 2012-06-13 07:50:47 PDT
Comment on attachment 147309 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147309&action=review

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:276
> +    if (coreResponse.httpStatusCode() / 100 == 3) {

I think it's clearer here to just do < 400 && > 200.

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:281
> +        String redirectUrl = coreResponse.httpHeaderField("Location");
> +        if (!redirectUrl.isEmpty())
> +            ewk_frame_uri_set(m_frame, redirectUrl.utf8().data());
> +        return;
> +    }

Would it be possible to put this work-around into the platform layer?
Comment 9 Dan Winship 2012-06-13 08:18:16 PDT
(In reply to comment #8)
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:276
> > +    if (coreResponse.httpStatusCode() / 100 == 3) {
> 
> I think it's clearer here to just do < 400 && > 200.

if (SOUP_STATUS_IS_REDIRECTION(coreResponse.httpStatusCode())

> Would it be possible to put this work-around into the platform layer?

Yes, it should happen in ResourceHandleSoup.

But you can't just always redirect every 3xx response; there's some reason why libsoup didn't apply the redirect itself, so webkit needs to make its own policy decision before applying the redirect itself. In this particular case (redirect-to-about-blank), it's that libsoup won't do a redirect to a non-http: URI, so you should check Location, and if it's non-http:, do the redirect yourself, but not otherwise.
Comment 10 Chris Dumez 2012-06-13 23:57:21 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:276
> > > +    if (coreResponse.httpStatusCode() / 100 == 3) {
> > 
> > I think it's clearer here to just do < 400 && > 200.
> 
> if (SOUP_STATUS_IS_REDIRECTION(coreResponse.httpStatusCode())
> 
> > Would it be possible to put this work-around into the platform layer?
> 
> Yes, it should happen in ResourceHandleSoup.
> 
> But you can't just always redirect every 3xx response; there's some reason why libsoup didn't apply the redirect itself, so webkit needs to make its own policy decision before applying the redirect itself. In this particular case (redirect-to-about-blank), it's that libsoup won't do a redirect to a non-http: URI, so you should check Location, and if it's non-http:, do the redirect yourself, but not otherwise.

I'm not sure how to achieve this in ResourceHandleSoup. I know where and how to detect the redirect to non-http case. However, I don't know what I should do when I detect this. I'm not familiar with this part of the code.
Comment 11 Dominik Röttsches (drott) 2012-10-30 07:55:27 PDT
Taking a look.
Comment 12 Dan Winship 2013-01-05 13:42:36 PST
Created attachment 181443 [details]
patch - handle redirection inside WebKit

This also fixes bug 61122. It also *almost* fixes bug 35300, but not quite; we're doing the right requests now, but DRT doesn't print out exactly what the test wants. (Actually, it might pass on EFL. Not sure.)

[So should I dup 61122 to this one?]

I tried to not touch anything related to HTTP authentication, other than what was needed to split redirection and authentication into separate codepaths. There are probably simplifications possible from here.
Comment 13 WebKit Review Bot 2013-01-05 14:44:33 PST
Comment on attachment 181443 [details]
patch - handle redirection inside WebKit

Attachment 181443 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15741201

New failing tests:
fast/frames/removal-before-attach-crash.html
Comment 14 Dan Winship 2013-01-07 06:32:09 PST
Comment on attachment 181443 [details]
patch - handle redirection inside WebKit

Bad reviewbot. This patch only modifies ResourceHandleSoup, and so could not be the cause of a chromium test failure.
Comment 15 Gyuyoung Kim 2013-01-07 20:06:09 PST
Comment on attachment 181443 [details]
patch - handle redirection inside WebKit

View in context: https://bugs.webkit.org/attachment.cgi?id=181443&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:394
> +#define MAX_REDIRECTS 20

According to WebKit coding style, WebKit prefers const to #define.
Comment 16 Gustavo Noronha (kov) 2013-01-09 05:48:37 PST
Comment on attachment 181443 [details]
patch - handle redirection inside WebKit

View in context: https://bugs.webkit.org/attachment.cgi?id=181443&action=review

LGTM other than Gyuyoung Kim's nit and mine. I think it makes sense to close 61122 as a dup as well as 105015, since it's obsolete now like you said.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:401
> +    // Some 3xx status codes aren't actually redirects

Missing period.
Comment 17 Martin Robinson 2013-01-09 09:17:26 PST
Comment on attachment 181443 [details]
patch - handle redirection inside WebKit

View in context: https://bugs.webkit.org/attachment.cgi?id=181443&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:453
> +    if (message->method != SOUP_METHOD_GET) {
> +        bool forceGET = false;
> +        if (newURL.protocolIsInHTTPFamily()) {
> +            switch (message->status_code) {
> +            case SOUP_STATUS_SEE_OTHER:
> +                forceGET = true;
> +                break;
> +            case SOUP_STATUS_FOUND:
> +            case SOUP_STATUS_MOVED_PERMANENTLY:
> +                if (message->method == SOUP_METHOD_POST)
> +                    forceGET = true;
> +                break;
> +            }
> +        } else
> +            forceGET = true;
> +
> +        if (crossOrigin && message->method == SOUP_METHOD_DELETE)
> +            forceGET = true;
> +
> +        if (forceGET) {
> +            request.setHTTPMethod("GET");
> +            request.setHTTPBody(0);
> +            request.clearHTTPContentType();
> +        }
> +    }

I think it makes sense to put this into a helper function for the benefit of readability. Something like shouldForceGetDuringRedirect. That way you can use early returns and it will be a lot easier to follow the logic.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:474
> +    cleanupSoupRequestOperation(handle);
> +    if (createSoupRequestAndMessageForHandle(handle, request, true)) {

Maybe cleanupSoupRequestOperation should be part of createSoupRequestAndMessageForHandle now?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:478
> +        d->client()->cannotShowURL(handle);

It'd probably be slightly more "WebKit-y" to do an early return with the error case here. Not a big deal.
Comment 18 Dan Winship 2013-01-09 12:17:17 PST
Created attachment 181967 [details]
updated for comments

> I think it makes sense to put this into a helper function for the benefit of readability.

not totally sure that this was an improvement, given the completely random assortment of parameters we have to pass to it...

> Maybe cleanupSoupRequestOperation should be part of createSoupRequestAndMessageForHandle now?

Well, the other place that calls createSoupRequestAndMessageForHandle() doesn't need it...
Comment 19 WebKit Review Bot 2013-01-09 14:05:11 PST
Comment on attachment 181967 [details]
updated for comments

Clearing flags on attachment: 181967

Committed r139239: <http://trac.webkit.org/changeset/139239>
Comment 20 WebKit Review Bot 2013-01-09 14:05:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Dan Winship 2013-01-11 07:22:24 PST
*** Bug 61122 has been marked as a duplicate of this bug. ***