Bug 59070 – [Qt] Fix QNetworkReplyWrapper to not depend on QNetworkReply::isFinished() method

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

[Caio Marcelo de Oliveira Filho]

[Qt] Fix QNetworkReplyWrapper to not depend on QNetworkReply::isFinished() method

[Caio Marcelo de Oliveira Filho]

Created attachment 90483 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Luiz, could you take a look at this?

I've decided to make receiveMetaDataWhenFinished() to avoid depending on the order of the connections.

[Caio Marcelo de Oliveira Filho]

Created attachment 90491 [details]
patch v2, rebased

[Andreas Kling]

Comment on attachment 90491 [details]
patch v2, rebased

r=me. Sucky, but unavoidable.

[Luiz Agostini]

I don't like it that much. It is too complex.

A crazy idea: why don't we take advantage of the fact that QNetworkReply is a QObject and store a dynamic property on it to keep the flag that we need?
I think that if we just create a slot in QNRW that sets that dynamic property on QNR and connect it in QNRW constructor it is done (take care to never disconnect it). The flag will be in the QNR object only (as it should) and we can avoid all other connections, parameters, flags that are replicated in two classes, ...

I don't think that it is a problem to trust on the signal emission sequence because it is a well known feature of Qt that is well documented.

Anyway, I think that with the proposed patch we might have problems with synchronous requests. Are layout tests ok?

[Caio Marcelo de Oliveira Filho]

Luiz, thanks for looking into this :-)

(In reply to comment #5)
> I don't like it that much. It is too complex.
> 
> A crazy idea: why don't we take advantage of the fact that QNetworkReply is a QObject and store a dynamic property on it to keep the flag that we need?
> I think that if we just create a slot in QNRW that sets that dynamic property on QNR and connect it in QNRW constructor it is done (take care to never disconnect it). The flag will be in the QNR object only (as it should) and we can avoid all other connections, parameters, flags that are replicated in two classes, ...

I see your point. I will try to implement this idea today and we can see how it goes. :-)   


> I don't think that it is a problem to trust on the signal emission sequence because it is a well known feature of Qt that is well documented.

OK. I do trust the sequence, I was avoiding depend on the connect lines having to respect a strict order, but I guess a comment would be enough.


> Anyway, I think that with the proposed patch we might have problems with synchronous requests. Are layout tests ok?

What kind of problems? Both wrapper and sniffer are per-reply, so one reply will not affect others. In practice, this shouldn't change anything for non-fake replies -- which is the case for layout tests.

How can I test the synchronous requests? Do we have autotests for that or only in layout tests?

[Caio Marcelo de Oliveira Filho]

The dynamic approach indeed looks better and avoids complicating the logic.

> > Anyway, I think that with the proposed patch we might have problems with synchronous requests. Are layout tests ok?
> 
> What kind of problems? Both wrapper and sniffer are per-reply, so one reply will not affect others. In practice, this shouldn't change anything for non-fake replies -- which is the case for layout tests.

Ok. I've found the problems, manually tracking the finished works only if we can catch the signal. That's where the fake replies heuristic of emitting finished() after a 0-timer come from, they need to give time for the  :-)

What I plan to do is to make our isFinished check both the dynamic property AND the m_reply property. This way we catch asynchronous and synchronous loads for Qt network replies, and also asynchronous loads for fake network replies.

If I understand correct, until Qt 4.8 (where we can set finished) we can't really have autotests for synchronous load with custom replies. Makes sense?

 
> How can I test the synchronous requests? Do we have autotests for that or only in layout tests?

I've found LayoutTests/http. Are there others?

[Caio Marcelo de Oliveira Filho]

(In reply to comment #7)

> Ok. I've found the problems, manually tracking the finished works only if we can catch the signal. That's where the fake replies heuristic of emitting finished() after a 0-timer come from, they need to give time for the  :-)

...give time for the signal connections. :-)

[Caio Marcelo de Oliveira Filho]

Created attachment 91603 [details]
patch v3, using dynamic property

[Luiz Agostini]

Comment on attachment 91603 [details]
patch v3, using dynamic property

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:663
> -    if (m_loadType == SynchronousLoad && m_replyWrapper->reply()->isFinished()) {
> +    if (m_loadType == SynchronousLoad && m_replyWrapper->isFinished()) {
>          m_replyWrapper->synchronousLoad();
>          // If supported, a synchronous request will be finished at this point, no need to hook up the signals.
>          return;

For synchronous requests we should call m_replyWrapper->synchronousLoad() and return anyway.
My suggestion now is that we do not check for isFinished at this point and that we call QNetworkReplyWrapper::setFinished() from QNetworkReplyWrapper::synchronousLoad().
Then we may avoid using QNetworkReply::isFinished() completely.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.h:84
> +    bool isFinished() const { return m_reply->isFinished() || m_reply->property("_q_isFinished").toBool(); }

How can we trust on QNetworkReply::isFinished()? We should have a solution that does not rely on it.

> Source/WebCore/platform/network/qt/QtMIMETypeSniffer.h:44
> +    bool isReplyFinished() const { return m_reply->isFinished() || m_reply->property("_q_isFinished").toBool(); }

The same here.

[Caio Marcelo de Oliveira Filho]

Created attachment 91986 [details]
patch v4, use fact that synchronous loads are always finished

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/qt/QNetworkReplyHandler.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


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

[Caio Marcelo de Oliveira Filho]

Created attachment 91988 [details]
patch v4 (second take), use the fact that synchronous loads are always finished

[Luiz Agostini]

Comment on attachment 91988 [details]
patch v4 (second take), use the fact that synchronous loads are always finished

LGTM

[Andreas Kling]

Comment on attachment 91988 [details]
patch v4 (second take), use the fact that synchronous loads are always finished

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:311
> +    // QNetworkReply, the QNetworkReply::isFinished() is never true, so we need to keep track

s/the//

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:312
> +    // ourselves. This limitation is fixed in 4.8, but we'll still have applications that doesn't

s/doesn't/don't/

[Caio Marcelo de Oliveira Filho]

Created attachment 92013 [details]
patch v5, better comment

[Eric Seidel (no email)]

Comment on attachment 92013 [details]
patch v5, better comment

Rejecting attachment 92013 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Caio Marcelo de Oliveira Filho]

Commited: http://trac.webkit.org/changeset/85734