From e5b8db6d4ecc244f7c35ea96a1f4e76fad47f1c4 Mon Sep 17 00:00:00 2001 From: Markus Goetz Date: Wed, 24 Jun 2009 15:59:49 +0200 Subject: QNetworkReplyImpl: Protect against recursive event loops This fixes a bug that occured together with a QProgressDialog. The signal emission was like: readyRead readyRead readyRead [...] readyRead finished readyRead Now finished should be properly at the ending of this sequence. Task-number: 256630 Reviewed-by: Thiago Macieira --- src/network/access/qnetworkreplyimpl.cpp | 36 +++++- src/network/access/qnetworkreplyimpl_p.h | 5 + tests/auto/auto.pro | 1 + .../qnetworkaccessmanager_and_qprogressdialog.pro | 5 + ...t_qnetworkaccessmanager_and_qprogressdialog.cpp | 130 +++++++++++++++++++++ 5 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 tests/auto/qnetworkaccessmanager_and_qprogressdialog/qnetworkaccessmanager_and_qprogressdialog.pro create mode 100644 tests/auto/qnetworkaccessmanager_and_qprogressdialog/tst_qnetworkaccessmanager_and_qprogressdialog.cpp diff --git a/src/network/access/qnetworkreplyimpl.cpp b/src/network/access/qnetworkreplyimpl.cpp index e8cb6ee..d903d03 100644 --- a/src/network/access/qnetworkreplyimpl.cpp +++ b/src/network/access/qnetworkreplyimpl.cpp @@ -54,6 +54,7 @@ QT_BEGIN_NAMESPACE inline QNetworkReplyImplPrivate::QNetworkReplyImplPrivate() : copyDevice(0), networkCache(0), cacheEnabled(false), cacheSaveDevice(0), + notificationHandlingPaused(false), bytesDownloaded(0), lastBytesDownloaded(-1), bytesUploaded(-1), state(Idle) { @@ -164,9 +165,11 @@ void QNetworkReplyImplPrivate::_q_copyReadyRead() lastBytesDownloaded = bytesDownloaded; QVariant totalSize = cookedHeaders.value(QNetworkRequest::ContentLengthHeader); + pauseNotificationHandling(); emit q->downloadProgress(bytesDownloaded, totalSize.isNull() ? Q_INT64_C(-1) : totalSize.toLongLong()); emit q->readyRead(); + resumeNotificationHandling(); } void QNetworkReplyImplPrivate::_q_copyReadChannelFinished() @@ -210,6 +213,9 @@ void QNetworkReplyImplPrivate::backendNotify(InternalNotifications notification) void QNetworkReplyImplPrivate::handleNotifications() { + if (notificationHandlingPaused) + return; + NotificationQueue current = pendingNotifications; pendingNotifications.clear(); @@ -248,6 +254,22 @@ void QNetworkReplyImplPrivate::handleNotifications() } } +// Do not handle the notifications while we are emitting downloadProgress +// or readyRead +void QNetworkReplyImplPrivate::pauseNotificationHandling() +{ + notificationHandlingPaused = true; +} + +// Resume notification handling +void QNetworkReplyImplPrivate::resumeNotificationHandling() +{ + Q_Q(QNetworkReplyImpl); + notificationHandlingPaused = false; + if (pendingNotifications.size() >= 1) + QCoreApplication::postEvent(q, new QEvent(QEvent::NetworkReplyUpdated)); +} + void QNetworkReplyImplPrivate::createCache() { // check if we can save and if we're allowed to @@ -318,8 +340,10 @@ void QNetworkReplyImplPrivate::consume(qint64 count) bytesUploaded += count; QVariant totalSize = request.header(QNetworkRequest::ContentLengthHeader); + pauseNotificationHandling(); emit q->uploadProgress(bytesUploaded, totalSize.isNull() ? Q_INT64_C(-1) : totalSize.toLongLong()); + resumeNotificationHandling(); } qint64 QNetworkReplyImplPrivate::nextDownstreamBlockSize() const @@ -367,12 +391,14 @@ void QNetworkReplyImplPrivate::feed(const QByteArray &data) QPointer qq = q; QVariant totalSize = cookedHeaders.value(QNetworkRequest::ContentLengthHeader); + pauseNotificationHandling(); emit q->downloadProgress(bytesDownloaded, totalSize.isNull() ? Q_INT64_C(-1) : totalSize.toLongLong()); emit q->readyRead(); // hopefully we haven't been deleted here if (!qq.isNull()) { + resumeNotificationHandling(); // do we still have room in the buffer? if (nextDownstreamBlockSize() > 0) backendNotify(QNetworkReplyImplPrivate::NotifyDownstreamReadyWrite); @@ -409,19 +435,25 @@ void QNetworkReplyImplPrivate::finished() state = Finished; pendingNotifications.clear(); + pauseNotificationHandling(); QVariant totalSize = cookedHeaders.value(QNetworkRequest::ContentLengthHeader); - if (bytesDownloaded != lastBytesDownloaded || totalSize.isNull()) + if (bytesDownloaded != lastBytesDownloaded || totalSize.isNull()) { emit q->downloadProgress(bytesDownloaded, bytesDownloaded); - if (bytesUploaded == -1 && outgoingData) + } + if (bytesUploaded == -1 && outgoingData) { emit q->uploadProgress(0, 0); + } + resumeNotificationHandling(); completeCacheSave(); // note: might not be a good idea, since users could decide to delete us // which would delete the backend too... // maybe we should protect the backend + pauseNotificationHandling(); emit q->readChannelFinished(); emit q->finished(); + resumeNotificationHandling(); } void QNetworkReplyImplPrivate::error(QNetworkReplyImpl::NetworkError code, const QString &errorMessage) diff --git a/src/network/access/qnetworkreplyimpl_p.h b/src/network/access/qnetworkreplyimpl_p.h index 32bfa4c..65b3679 100644 --- a/src/network/access/qnetworkreplyimpl_p.h +++ b/src/network/access/qnetworkreplyimpl_p.h @@ -129,6 +129,9 @@ public: void setup(QNetworkAccessManager::Operation op, const QNetworkRequest &request, QIODevice *outgoingData); void setNetworkCache(QAbstractNetworkCache *networkCache); + + void pauseNotificationHandling(); + void resumeNotificationHandling(); void backendNotify(InternalNotifications notification); void handleNotifications(); void createCache(); @@ -156,6 +159,8 @@ public: QIODevice *cacheSaveDevice; NotificationQueue pendingNotifications; + bool notificationHandlingPaused; + QUrl urlForLastAuthentication; #ifndef QT_NO_NETWORKPROXY QNetworkProxy lastProxyAuthentication; diff --git a/tests/auto/auto.pro b/tests/auto/auto.pro index 1cdb840..a215daa 100644 --- a/tests/auto/auto.pro +++ b/tests/auto/auto.pro @@ -207,6 +207,7 @@ SUBDIRS += _networkselftest \ qnetworkproxy \ qnetworkrequest \ qnetworkreply \ + qnetworkaccessmanager_and_qprogressdialog \ qnumeric \ qobject \ qobjectrace \ diff --git a/tests/auto/qnetworkaccessmanager_and_qprogressdialog/qnetworkaccessmanager_and_qprogressdialog.pro b/tests/auto/qnetworkaccessmanager_and_qprogressdialog/qnetworkaccessmanager_and_qprogressdialog.pro new file mode 100644 index 0000000..7ed5b07 --- /dev/null +++ b/tests/auto/qnetworkaccessmanager_and_qprogressdialog/qnetworkaccessmanager_and_qprogressdialog.pro @@ -0,0 +1,5 @@ +load(qttest_p4) +SOURCES += tst_qnetworkaccessmanager_and_qprogressdialog.cpp +QT += network + + diff --git a/tests/auto/qnetworkaccessmanager_and_qprogressdialog/tst_qnetworkaccessmanager_and_qprogressdialog.cpp b/tests/auto/qnetworkaccessmanager_and_qprogressdialog/tst_qnetworkaccessmanager_and_qprogressdialog.cpp new file mode 100644 index 0000000..62e4ce5 --- /dev/null +++ b/tests/auto/qnetworkaccessmanager_and_qprogressdialog/tst_qnetworkaccessmanager_and_qprogressdialog.cpp @@ -0,0 +1,130 @@ +/**************************************************************************** +** +** Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies). +** Contact: Nokia Corporation (qt-info@nokia.com) +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL$ +** No Commercial Usage +** This file contains pre-release code and may not be distributed. +** You may use this file in accordance with the terms and conditions +** contained in the either Technology Preview License Agreement or the +** Beta Release License Agreement. +** +** GNU Lesser General Public License Usage +** Alternatively, this file may be used under the terms of the GNU Lesser +** General Public License version 2.1 as published by the Free Software +** Foundation and appearing in the file LICENSE.LGPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU Lesser General Public License version 2.1 requirements +** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. +** +** In addition, as a special exception, Nokia gives you certain +** additional rights. These rights are described in the Nokia Qt LGPL +** Exception version 1.0, included in the file LGPL_EXCEPTION.txt in this +** package. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3.0 as published by the Free Software +** Foundation and appearing in the file LICENSE.GPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU General Public License version 3.0 requirements will be +** met: http://www.gnu.org/copyleft/gpl.html. +** +** If you are unsure which license is appropriate for your use, please +** contact the sales department at http://www.qtsoftware.com/contact. +** $QT_END_LICENSE$ +** +****************************************************************************/ + + +#include +#include +#include +#include +#include +#include +#include + +#include "../network-settings.h" + + +class tst_QNetworkAccessManager_And_QProgressDialog : public QObject +{ +Q_OBJECT +private slots: + void downloadCheck(); +}; + +class DownloadCheckWidget : public QWidget +{ + Q_OBJECT; +public: + DownloadCheckWidget(QWidget *parent = 0) : QWidget(parent) + , progressDlg(this), netmanager(this) + , lateReadyRead(true) + { + progressDlg.setRange(1, 100); + QMetaObject::invokeMethod(this, "go", Qt::QueuedConnection); + } + bool lateReadyRead; +public slots: + void go() + { + QNetworkReply *reply = netmanager.get( + QNetworkRequest( + QUrl("http://" + QtNetworkSettings::serverName() + "/qtest/bigfile") + )); + connect(reply, SIGNAL(downloadProgress(qint64, qint64)), + this, SLOT(dataReadProgress(qint64, qint64))); + connect(reply, SIGNAL(readyRead()), + this, SLOT(dataReadyRead())); + connect(reply, SIGNAL(finished()), this, SLOT(finishedFromReply())); + + progressDlg.exec(); + } + void dataReadProgress(qint64 done, qint64 total) + { + QNetworkReply *reply = qobject_cast(sender()); + progressDlg.setMaximum(total); + progressDlg.setValue(done); + } + void dataReadyRead() + { + QNetworkReply *reply = qobject_cast(sender()); + lateReadyRead = true; + } + void finishedFromReply() + { + QNetworkReply *reply = qobject_cast(sender()); + lateReadyRead = false; + reply->deleteLater(); + QTestEventLoop::instance().exitLoop(); + } + + +private: + QProgressDialog progressDlg; + QNetworkAccessManager netmanager; +}; + +void tst_QNetworkAccessManager_And_QProgressDialog::downloadCheck() +{ + DownloadCheckWidget widget; + widget.show(); + // run and exit on finished() + QTestEventLoop::instance().enterLoop(10); + QVERIFY(!QTestEventLoop::instance().timeout()); + // run some more to catch the late readyRead() (or: to not catch it) + QTestEventLoop::instance().enterLoop(1); + QVERIFY(QTestEventLoop::instance().timeout()); + // the following fails when a readyRead() was received after finished() + QVERIFY(!widget.lateReadyRead); +} + + + +QTEST_MAIN(tst_QNetworkAccessManager_And_QProgressDialog) +#include "tst_qnetworkaccessmanager_and_qprogressdialog.moc" -- cgit v0.12