Bug 60144 – [Qt] QNetworkReplyHandler processes redirect in the wrong slot

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

QNetworkReplyHandler needs to do the processing of the RedirectionTargetAttribute in the QNetworkReply's finished() slot, not in the metaDataChanged() slot.
The reason for this is, the redirect might send payload data, and if you already do the new request in metaDataChanged() slot, the connection cannot be reused and a new tcp connection has to be established.

[Pierre Rossi]

Created attachment 100973 [details]
patch

[Benjamin Poulain]

Comment on attachment 100973 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +

You should add a description explaining what the patch does and how.

[Benjamin Poulain]

Caio and Luiz, I think you worked on this before. Any comment?

The change looks sane to me.

[Pierre Rossi]

Created attachment 101156 [details]
With description.

[Luiz Agostini]

Comment on attachment 101156 [details]
With description.

I think that with this change sendResponseIfNeeded could get called twice, what would be wrong.
Anyway, to be more confident, I would like to see a layout test that receives a redirection response with content, and that the content of the response is splited in chunks with some delay between them.

Although I am not a formal reviewer, for now I will r- because of the lack of the test.

[Markus Goetz]

Change looks good to me.

Even if sendResponseIfNeeded() would get called twice it protects itself against this (last time I checked).

If no layout test regresses I'd say go for it.

[Luiz Agostini]

(In reply to comment #6)
> Change looks good to me.
> 
> Even if sendResponseIfNeeded() would get called twice it protects itself against this (last time I checked).

It does not.
> 
> If no layout test regresses I'd say go for it.

The problem is not about sendResponseIfNeeded() itself. It is the ResourceHandleClient::didReceiveResponse that should not be called for the redirection response.

With this change ResourceHandleClient::didReceiveResponse will be called notifying a 307 for example, and then it will be called again for the new response received from the second request. ResourceHandleClient::didReceiveResponse should be called only once for the last response received, the one that is not a redirection.

Even if sendResponseIfNeeded was protecting itself against been called more then once, it would be called only with the first response, the redirection response, what would be wrong.

I still think that we should have a new layout test for this issue, because it is not a simple refactoring, and it would be nice to run the layout tests in debug mode to be sure that we are not hitting any assertion.

[Luiz Agostini]

Comment on attachment 101156 [details]
With description.

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:-277
> -    m_redirectionTargetUrl = m_reply->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl();
> -    if (m_redirectionTargetUrl.isValid()) {
> -        QueueLocker lock(m_queue);
> -        m_queue->push(&QNetworkReplyHandler::sendResponseIfNeeded);
> -        m_queue->push(&QNetworkReplyHandler::finish);
> -        return;
> -    }

This change is not good. As you are not returning from the method when m_redirectionTargetUrl is valid the response receiving will continue normally. The contents of the message will eventually go through mime sniffing and sendResponseIfNeeded will eventually get called before QNetworkReplyWrapper::setFinished. This is not right and will not postpone the sending of the redirected request.

What you should do is to check m_redirectionTargetUrl.isValid() and return without doing anything if it is true. This way the only slot that will remain connected is setFinished and nothing will happen before QNetworkReply::finished.

It could be even better to have a dedicated slot with a descriptive name that would be connected to QNetworkReply::finished only in redirections. This slot would just execute 

    m_queue->push(&QNetworkReplyHandler::sendResponseIfNeeded);
    m_queue->push(&QNetworkReplyHandler::finish);

unconditionally.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:314
> +    m_redirectionTargetUrl = m_reply->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl();
> +    if (m_redirectionTargetUrl.isValid()) {
> +        QueueLocker lock(m_queue);
> +        m_queue->push(&QNetworkReplyHandler::sendResponseIfNeeded);
> +        m_queue->push(&QNetworkReplyHandler::finish);
> +        return;
> +    }

I would prefer to have the dedicated slot described above.

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