Bug 49650 – REGRESSION: [Qt] QNetworkReply delivered by the unsupportedContent() signal does not contain downloaded data

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Page Loading

It's not possible to get any data except meta data from the reply submitted with the unsupportedContent(QNetworkReply*) Signal since Qt 4.7.

USECASE:

- configure a webview to forward unsupported content
- connect the signal with a propper slot
- open the webview
- klick on a download link (something with mime type "application/octet-stream" for example)
- if the debugging steps into the slot connected to the unsupported content signal only meta data are available (not bad so far)
- in the slot wait for finishing the reply (takes longer then usual as if no the reply does not have a connection anymore)
- after the reply finished reply->size() will return 0 and also reply->header(QNetworkRequest::ContentLengthHeader) will return 0

[Benjamin Poulain]

Could you please attach a simple test case so we can run in and just start to debug?

Created attachment 74109 [details]
3 src files containing a little usecase for the bug

Here is a little test case provided with workaround (commented out).

The problem is that this workaround only works for "GET" requests. But I expect some unsupportedContent after "POST" or "PUT" requests. Moreover I find it bad style to request twice the same just because the first request does not what it's expected to do.

Until Qt 4.6.3 this worked fine without workaround and all.

[Benjamin Poulain]

Moving to P1, quite a bad regression.

Downloading the package of http://qt.nokia.com/downloads/qt-creator-source-package does not work.
It might be a QNAM problem, I have no problem with KIO.

[Jan Erik Hanssen]

Initial investigation shows that the problem is related to the move from QueuedConnection to AutoConnection in QNetworkReplyHandler.cpp. The issue is that when DirectConnection is being used, QWebPage::unsupportedContent(QNetworkReply*) is being emitted in a slot connected to QTcpSocket::readyRead(). But if you open a modal dialog or just use a QEventLoop then the readyRead() signal emission won't return and thus the tcp packet is not ACK'ed.

I'm not really sure how to solve this, not even sure it can be solved in WebKit without going back to QueuedConnection.

[Benjamin Poulain]

Adding Holger, he knows those stuffs.

[Holger Freyther]

(In reply to comment #5)
> Adding Holger, he knows those stuffs.

Tricky. I don't have a fast answer and will need to take a look at it, I will try to find some time.

[Stephen]

I am probably having the same problem here. I am implementing my own download manager by handling QWebPage::unsupportContent(QNetworkReply *reply). If I add a modal dialog in the handler to obtain the download confirmation with the user, the reply object will be no longer return any data. reply->isFinished() will return true after that. 

If no dialog is shown and I directly proceed to download the file, everything is fine. 

The same problem happens if I add some time delay with app event loop processing in the middle, for example:

    int nMilSeconds=100;
    QTime dieTime = QTime::currentTime().addMSecs(nMilSeconds);
    while( QTime::currentTime() < dieTime )
    {
        qApp->sendPostedEvents();
        qApp->processEvents(QEventLoop::AllEvents,nMilSeconds);
    }

Jan, could you confirm whether this is the same problem as what you described? Have you guys have any update or workaround on this?

Thanks a lot.
Stephen.

[Jan Erik Hanssen]

(In reply to comment #7)
> Jan, could you confirm whether this is the same problem as what you described? Have you guys have any update or workaround on this?

That does sound like the same issue, yes.

If you control your local WebKit source then you could always revert to using QueuedConnection in QNetworkReplyHandler.cpp, I believe the change to AutoConnection was done for performance reasons and you should have no other issues with doing this than things getting a bit slower.

Another possible solution (though I can't say for sure this will work) could be to not open your dialog in a slot connected to unsupportContent(), but instead do a queued slot invokation (e.g. using a single-shot timer) and do the dialog execution there.

[Stephen]

Jan, I tried your proposal of using a queued slot but it didn't work. Even if I convert the modal dialog into a non-modal dialog and initiate the file download when the dialog is closed, the reply object gets closed. Only if I hook proper receiving slots to signals such as readyRead(),finished(), downloadProgress() first (i.e., we are actually starting the download) and then show a modal dialog, the download goes fine. I guess when the app event loop starts and there are no receiving slots for the reply connection, the connection is automatically closed. If there is way to postpone processing events and signals from the reply connection object while keep pumping other app events, that'd work as well. I am still pretty green to QT and am not aware of such advanced techniques. 

I really would like to inform the user first before actually burning the bandwidth. I hope you guys can find some effective to resolve this issue in the next Qt release. 

Thanks a lot.

[Benjamin Poulain]

(In reply to comment #9)
> Jan, I tried your proposal of using a queued slot [...]

This is no place to ask for support, especially to Jan Erik who is a benevolent contributor.

You are free to comment if want to help fixing this in WebKit, but please use the forum (http://developer.qt.nokia.com/forums/viewforum/21/ ) to get workaround and the like.

[Andreas Kling]

Taking and blocking 2.2 release on this.

[Andreas Kling]

Created attachment 86264 [details]
Proposed patch

[Alexis Menard (darktears)]

Comment on attachment 86264 [details]
Proposed patch

Looks good to me : r+

[Holger Freyther]

No unit test? sorry to be the pain.

[Benjamin Poulain]

Comment on attachment 86264 [details]
Proposed patch

What about testing with a custom QNam? This sounds like regression invitation otherwise. Some dude will come in two years, think this queued signal is legacy and remove it. :-D

[Andreas Kling]

Righto. This should be fixed in Qt, reported: http://bugreports.qt.nokia.com/browse/QTBUG-18718

Leaving this open to track progress.

[Luiz Agostini]

It seems to be a big problem that we should solve for QtWebKit-2.2.
Don't you think that we should land the proposed patch until the problem is properly solved in QNAM?

[Benjamin Poulain]

(In reply to comment #17)
> It seems to be a big problem that we should solve for QtWebKit-2.2.
> Don't you think that we should land the proposed patch until the problem is properly solved in QNAM?

You are right, we should do that.

I suggest to push that in trunk with a comment mentioning http://bugreports.qt.nokia.com/browse/QTBUG-18718
Then we cherry pick to 2.2.

[Andreas Kling]

(In reply to comment #18)
> (In reply to comment #17)
> > It seems to be a big problem that we should solve for QtWebKit-2.2.
> > Don't you think that we should land the proposed patch until the problem is properly solved in QNAM?
> 
> You are right, we should do that.
> 
> I suggest to push that in trunk with a comment mentioning http://bugreports.qt.nokia.com/browse/QTBUG-18718
> Then we cherry pick to 2.2.

Candycakes! I'll land it with a comment, then.

[Andreas Kling]

Committed r87797: <http://trac.webkit.org/changeset/87797>

[Holger Freyther]

Where is the unit/layout test?

[Ademar Reis]

Revision r87797 cherry-picked into qtwebkit-2.2 with commit 791c2c8 <http://gitorious.org/webkit/qtwebkit/commit/791c2c8>

[Andreas Kling]

(In reply to comment #21)
> Where is the unit/layout test?

There is none, see comment in ChangeLog:
No new tests since this doesn't fail for qrc:// or file:// URLs and our tests can't depend on http:// URLs.

[Holger Freyther]

(In reply to comment #23)

> There is none, see comment in ChangeLog:
> No new tests since this doesn't fail for qrc:// or file:// URLs and our tests can't depend on http:// URLs.

Well, the underlying question is if you think test cases are optional or not. I really would prefer to not have double standards, accept if a fellow reviewer says it is not testable, but for ordinary contributors ask them to come up with unit tests... it is a bit saddening.

[Benjamin Poulain]

(In reply to comment #24)
> > There is none, see comment in ChangeLog:
> > No new tests since this doesn't fail for qrc:// or file:// URLs and our tests can't depend on http:// URLs.
> 
> Well, the underlying question is if you think test cases are optional or not. I really would prefer to not have double standards, accept if a fellow reviewer says it is not testable, but for ordinary contributors ask them to come up with unit tests... it is a bit saddening.

Andreas and I share an office. He asked me to review that, I asked for test, and we discussed how we could test this. That is just not on the comments of this thread.

Given testing this requires building too much test infrastructure, I agreed to push the work around for now while the network team looks at QNAM.

It is just an exception for a workaround of a bug in Qt. I don't understand the drama. If I was unfair regarding testing for one of your patch, I appologize.

[Andreas Kling]

(In reply to comment #24)
> (In reply to comment #23)
> 
> > There is none, see comment in ChangeLog:
> > No new tests since this doesn't fail for qrc:// or file:// URLs and our tests can't depend on http:// URLs.
> 
> Well, the underlying question is if you think test cases are optional or not. I really would prefer to not have double standards, accept if a fellow reviewer says it is not testable, but for ordinary contributors ask them to come up with unit tests... it is a bit saddening.

If someone claims a change is untestable or especially nontrivial to test, I won't reject an otherwise correct patch over it. This is currently often the case with drag&drop bugs on Qt, since our DRT has major issues with d&d.

If you have a way of testing this particular change, I'm all ears.

[Holger Freyther]

(In reply to comment #25)

...
> Given testing this requires building too much test infrastructure, I agreed to push the work around for now while the network team looks at QNAM.
> 
> It is just an exception for a workaround of a bug in Qt. I don't understand the drama. If I was unfair regarding testing for one of your patch, I appologize.

Sorry, this might be a bit lengthy. So first of all I don't have any hard feelings and it is not me feeling treated unfairly in some other bug report or such. It is more a been there, done that moment. From the comments to this bug report I get the feeling (which of course can be wrong) is that you are rushing for a release and from my experience the sad truth is there are no such shortcuts.

E.g. when Simon and me were working on the first QtWebKit release and we met in person (Qt DevDays, FOSS.IN, CCC event) we were so happy to avoid hitting the bugtracker, giving each other review in place. It was at a time where we did not have the DumpRenderTree, or much unit tests for QWebView/QWebFrame/QWebPage. There have been many moments were I regret to not have created a bug, not having put enough context into the bug report or not having done a (better) test.

The next thing is that theory says one should not write stuff that one can not test (strategic defense initiative as the prime example), of course it is too late her right now. In any case even a not perfect test case helps us to progress...

So yes the hint here... don't rush, don't give in to pressure...

[Holger Freyther]

(In reply to comment #26)

> If you have a way of testing this particular change, I'm all ears.

Now, my own lazynes strucks... so this was not tired, you might have been there already...

a) Test that the unsupportedContent signal is a queued signal.

The original bug was introduced by me when I moved away from QueuedConnection to get better performance. So the idea (not for the bugfix but to avoid future regressions) would be to test (by creating a new EventLoop) that some of the API signals are queued, leave a comment pointing to this bug report... so next time a smart-ass like me coming around to play with the way a signal is connected needs to ack the test failure... Maybe you have decided against such a test, it would be nice to know why.

b) Create a simple http loading test... add a printf for the signal (when dumping frame load calls) inside the DRT for the unsupported content... now the vague part as I didn't try it yet... maybe add another printf inside FrameLoaderClientQt... and then a queued/non-queued signal would change the order of the printf...

c) Create a simple http loading test (platform specific), handle unsupported content by returning a unit value/string, use XML HTTP Request to load this this content... print the magic string coming from DRT for this unsupported content..

[Benjamin Poulain]

(In reply to comment #28)
> a) Test that the unsupportedContent signal is a queued signal.
> 
> The original bug was introduced by me when I moved away from QueuedConnection to get better performance. So the idea (not for the bugfix but to avoid future regressions) would be to test (by creating a new EventLoop) that some of the API signals are queued, leave a comment pointing to this bug report... so next time a smart-ass like me coming around to play with the way a signal is connected needs to ack the test failure... Maybe you have decided against such a test, it would be nice to know why.
> 
> b) Create a simple http loading test... add a printf for the signal (when dumping frame load calls) inside the DRT for the unsupported content... now the vague part as I didn't try it yet... maybe add another printf inside FrameLoaderClientQt... and then a queued/non-queued signal would change the order of the printf...

But the thing is the queueing is just an ugly workaround. That is not what we want to test, we want to make sure unsupported content behave as it should.


This is not the first time we have this kind of problem. We had some really weird timing issue with QNam over http. 
I just created https://bugs.webkit.org/show_bug.cgi?id=62012 following this thread. Pierre recently wrote a small HTTP server for testing some stuff, maybe he can pick this up.

[Dawit A.]

Unfortunately, this patch causes a regression in kdewebkit because we need the unsupportedContent signal to be delivered as early as possible. This breaks KDE KIO's put-slave-on-hold functionality that allows applications to pass an active network connection to each other.

[Ademar Reis]

The Queued-signal workaround has been reverted in r92479 (see Bug #63582) because of a regression spotted in KDE's custom QNAM feature.

I guess we have no choice but push for the real fix in Qt: http://bugreports.qt.nokia.com/browse/QTBUG-18718

[Dawit A.]

(In reply to comment #31)
> The Queued-signal workaround has been reverted in r92479 (see Bug #63582) because of a regression spotted in KDE's custom QNAM feature.
> 
> I guess we have no choice but push for the real fix in Qt: http://bugreports.qt.nokia.com/browse/QTBUG-18718

In the meantime, people with this problem can workaround it by connecting to the unsupportedContent(QNetworkReply*) signal using the Qt::QueuedConnection connection type.

[Ademar Reis]

Since this has to be fixed in Qt (we won't do anything in the QtWebKit side) and there's a workaround available on the user's side, shouldn't this bug be closed here as WONTFIX?

[Ademar Reis]

(In reply to comment #33)
> Since this has to be fixed in Qt (we won't do anything in the QtWebKit side) and there's a workaround available on the user's side, shouldn't this bug be closed here as WONTFIX?

Better ask for forgiveness than permission: closing bug as WONTFIX here. I'll keep track of the associated Qt Bug.

Bug 62012 is also of interest, it should allow us to test for this kind of problem in the future (there's a reference to this bug there).