Bug 36690 – [Qt] LayoutTests/http/tests/appcache/resource-redirect-2.html failed and skipped

• Status: CLOSED
• Resolution: FIXED
• Component: New Bugs

[Chang Shu]

The above test timed out on QtLinux.

Created attachment 51960 [details]
Patch helps in detetcting http redirect loops in Qt

[WebKit Review Bot]

Attachment 51960 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/qt/QNetworkReplyHandler.cpp:252:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 52035 [details]
Re-submitting patch according to style rules

[Eric Seidel (no email)]

Comment on attachment 52035 [details]
Re-submitting patch according to style rules

LayoutTests ChangeLog is bogus.

Created attachment 52447 [details]
Patch file adhereing to  style rules

[Eric Seidel (no email)]

Comment on attachment 52447 [details]
Patch file adhereing to  style rules

Tab in the ChangeLog will prevent this from being committed.  I'm surprised the style bot didn't catch that.

This looks sane, although I'm surprised the Qt network layer doesn't do this for you.

[Laszlo Gombos]

Comment on attachment 52447 [details]
Patch file adhereing to  style rules

r- as this now has to be reworked after fix for 37097 is landed. The same bug (missing feature) cause both xhr and app cache failures. 

Bug 37097 is using 10 as a redirect limit, I'm ok with that instead of 16 proposed as part of this patch.

Krithiga, can you rewor your patch so that it works on top of patch for bug 37097. I think your error handling code and the Skipped list update is still needed.

[Chang Shu]

(In reply to comment #7)
> (From update of attachment 52447 [details])
> r- as this now has to be reworked after fix for 37097 is landed. The same bug
> (missing feature) cause both xhr and app cache failures. 
> 
> Bug 37097 is using 10 as a redirect limit, I'm ok with that instead of 16
> proposed as part of this patch.
> 
> Krithiga, can you rewor your patch so that it works on top of patch for bug
> 37097. I think your error handling code and the Skipped list update is still
> needed.

Thanks for finding this out for us, Laszlo.

[Robert Hogan]

(In reply to comment #6)
> (From update of attachment 52447 [details])
> Tab in the ChangeLog will prevent this from being committed.  I'm surprised the
> style bot didn't catch that.
> 
> This looks sane, although I'm surprised the Qt network layer doesn't do this
> for you.

QtNetwork is pretty agnostic on this score. It passes back redirects to the client by setting an attribute in the QNetworkRequest:


    \value RedirectionTargetAttribute
        Replies only, type: QVariant::Url (no default)
        If present, it indicates that the server is redirecting the
        request to a different URL. The Network Access API does not by
        default follow redirections: it's up to the application to
        determine if the requested redirection should be allowed,
        according to its security policies.
        The returned URL might be relative. Use QUrl::resolved()
        to create an absolute URL out of it.

So if QtWebKit wants to respect redirects it needs to sanity check them as well.

[Chang Shu]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 52447 [details] [details])

> So if QtWebKit wants to respect redirects it needs to sanity check them as
> well.

Thanks for the clarification. This confirms the correctness of webkit approach.

Created attachment 52654 [details]
Patch file for appcache layout tests that can be removed from the skipped list.

New patch file containing the changes to layout skipped tests and corresponding change log.

[Laszlo Gombos]

ChangeLog has a small text alignment issue, otherwise looks good. Krithiga, can you fix it ?

Created attachment 52659 [details]
new patch file following tab rules for Change log

[Laszlo Gombos]

Comment on attachment 52659 [details]
new patch file following tab rules for Change log

lgtm, r+.

[WebKit Commit Bot]

Comment on attachment 52659 [details]
new patch file following tab rules for Change log

Clearing flags on attachment: 52659

Committed r57175: <http://trac.webkit.org/changeset/57175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r57175 cherry-picked into qtwebkit-2.0 with commit f8d7b7e06126519131d47d84f5d8cc9f0873b9fb

[Csaba Osztrogonác]

(In reply to comment #17)
> Revision r57175 cherry-picked into qtwebkit-2.0 with commit f8d7b7e06126519131d47d84f5d8cc9f0873b9fb

This make http/tests/appcache/fallback.html crash 
on the qtwebkit-2.0 bot. Have you got any idea why? 

I tried against Qt 4.6.0, 4.6.2 and 4.7.0, but
they didn't solve this crash.