Bug 88961 – [GTK][EFL] http/tests/misc/redirect-to-about-blank.html is failing

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Skipping test for GTK and EFL port in Bug 88962. Will unskip them later with a proper fix.

[Chris Dumez]

For some reason, our restartedCallback does not get called even though our gotHeadersCallback gets called with a 302 code.

[Zan Dobersek]

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.

[Chris Dumez]

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

[Dan Winship]

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)

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 147309 [details]
Patch

Rebase on master and unskip associated test.

[Martin Robinson]

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?

[Dan Winship]

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

[Chris Dumez]

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

[Dominik Röttsches (drott)]

Taking a look.

[Dan Winship]

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.

[WebKit Review Bot]

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

[Dan Winship]

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.

[Gyuyoung Kim]

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.

[Gustavo Noronha (kov)]

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.

[Martin Robinson]

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.

[Dan Winship]

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

[WebKit Review Bot]

Comment on attachment 181967 [details]
updated for comments

Clearing flags on attachment: 181967

Committed r139239: <http://trac.webkit.org/changeset/139239>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dan Winship]

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