Bug 70683 – [Qt] Change HTTP redirection behavior to match Firefox/Chromium

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Qt

[Dawit A.]

QtWebKit currently does not handle the 303 redirection response from a server correctly. According to RFC 2616 section 10.3.4, a 303 redirect should always be converted into a GET request before it is retrieved again. However, QtWebKit does not do that for both PUT and DELETE requests.

Actually what QtWebKit is currently doing, though very adherent to the spec, won't work with lots of broken servers out in the wild. That is because many server implementations incorrectly return a 301 or 302 response when they should actually be returing a 303. As a result, browsers like Chromium and Firefox treat all redirection response codes the same and simply convert all requests into a GET operation on redirection.

Though I personally believe that QtWebKit's behavior should be changed to match Firefox for maximum compatability, the attached patch only addresses the broken behavior of the 303 redirection.

[Dawit A.]

Created attachment 112094 [details]
proposed patch I

Fix the 303 redirection bug for PUT and DELETE requests.

[Dawit A.]

Created attachment 112097 [details]
proposed patch II

Change the redirection behavior to match that of Firefox and Chromium for all requests except HEAD.

[Dawit A.]

Created attachment 112115 [details]
proposed patch II (Updated)

Added missing changelog information.

[Dawit A.]

Created attachment 112116 [details]
proposed patch I (Updated)

Added missing changelog information.

[Dawit A.]

A site where you can test redirection behavior:

http://www.mnot.net/javascript/xmlhttprequest/

[Simon Hausmann]

Comment on attachment 112115 [details]
proposed patch II (Updated)

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

Generally I would prefer this approach to the other one. But that's just MHO, I'm not a real expert on this particular topic.

> Source/WebCore/ChangeLog:13
> +        No new tests required as existing redirection tests should cover this
> +        scenario as well.

Hm, this _is_ touchy code. I think that this patch should add a (http) layout test to cover this. If there exists a test already, then it should be unskipped.

[Dawit A.]

(In reply to comment #6)
> (From update of attachment 112115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112115&action=review
> 
> Generally I would prefer this approach to the other one. But that's just MHO, I'm not a real expert on this particular topic.

So do I and I do not think any particular exerptise is required for addressing this particular problem. The RFC on the topic, RFC 2616 section 10.3, is clear as a day light and so is the violation by the other browsers. One would only have to vist the wonderful test site mentioned in comment #5 with QtTestBrowser, Firefox and Chromium to see the differences in behavior. I think addressing that should be a no brainer since QtWebKit should be as close to these widely deployed browsers as possible.

Since we seem to agree this is the best approach, I will change this bug's title to match this patch.

> > Source/WebCore/ChangeLog:13
> > +        No new tests required as existing redirection tests should cover this
> > +        scenario as well.
> 
> Hm, this _is_ touchy code. I think that this patch should add a (http) layout test to cover this. If there exists a test already, then it should be unskipped.

There is already a layout test that was added due to another bug I reported a while back and I do not think it is skipped. See 

https://bugs.webkit.org/show_bug.cgi?id=60440#c24

However, I am unsure if the tests cover all the HTTP operations or just POST.

[Simon Hausmann]

(In reply to comment #7)
> Since we seem to agree this is the best approach, I will change this bug's title to match this patch.
> 
> > > Source/WebCore/ChangeLog:13
> > > +        No new tests required as existing redirection tests should cover this
> > > +        scenario as well.
> > 
> > Hm, this _is_ touchy code. I think that this patch should add a (http) layout test to cover this. If there exists a test already, then it should be unskipped.
> 
> There is already a layout test that was added due to another bug I reported a while back and I do not think it is skipped. See 
> 
> https://bugs.webkit.org/show_bug.cgi?id=60440#c24
> 
> However, I am unsure if the tests cover all the HTTP operations or just POST.

Let me try to clarify: What I'd like to see is a test that is executed and that fails unless your fix is applied :)

[Simon Hausmann]

Comment on attachment 112115 [details]
proposed patch II (Updated)

r- . This is unfortunately rather touchy code and the patch should be accompanied with either a change to the skip list to unskip a test that is currently failing because of the absence of this patch _or_ a new layout test is added.

[Simon Hausmann]

Comment on attachment 112116 [details]
proposed patch I (Updated)

Taking this one also out of the review queue. See https://bugs.webkit.org/show_bug.cgi?id=70683#c8

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.