[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#804883: QNAM does not report correct NetworkAccessibility (was: Regression: OwnCloud client fails to reconnect when network starts up)



Control: -1 retitle QNAM does not report correct NetworkAccessibility

[ Resending this to the Debian Qt/KDE maintainers as it only made it to
the Owncloud maintainers list. Sorry about my confusion how the BTS
would handle this. This is about Debian bug #804883 ]

Hi

Some context for the QT maintainers: This is about a bug in
owncloud-client which fails to reconnect to the server if the network
connection is lost. This is quite annoying because after every suspend
of your machine you have to restart the client because it does not
reconnect.

This bug in onwcloud-client is caused by a bug in QT. The upstream
bugreport is https://bugreports.qt.io/browse/QTBUG-46323.

There is a patch merged upstream, but it's not in any released version
yet: https://codereview.qt-project.org/#/c/121724/

I locally compiled QT with the fix applied and can confirm that this
fixes the oncloud-client issue for me. To be able to apply the patch I
had to pull in a second patch as a dependency:
https://bugreports.qt.io/browse/QTBUG-47471
https://codereview.qt-project.org/#/c/123087/2

The patch to the Debian package I used is attached. Thanks for
considering this until a fixed version is released upstream. It fixes a
very annoying bug in owncloud-client.

Gaudenz

From bf47964d9f8902dbdaae310786ae8118e2d20fb0 Mon Sep 17 00:00:00 2001
From: Gaudenz Steinlin <gaudenz@debian.org>
Date: Mon, 16 Nov 2015 15:31:37 +0100
Subject: [PATCH] Add patches to fix QT bugs #46323 and #47471

Closes: #804883
---
 debian/patches/qtbug-46323.patch | 233 +++++++++++++++++++++++++++++++++++++++
 debian/patches/qtbug-47471.patch | 146 ++++++++++++++++++++++++
 debian/patches/series            |   4 +
 3 files changed, 383 insertions(+)
 create mode 100644 debian/patches/qtbug-46323.patch
 create mode 100644 debian/patches/qtbug-47471.patch

diff --git a/debian/patches/qtbug-46323.patch b/debian/patches/qtbug-46323.patch
new file mode 100644
index 0000000..fcc444e
--- /dev/null
+++ b/debian/patches/qtbug-46323.patch
@@ -0,0 +1,233 @@
+From bb281eea179d50a413f4ec1ff172d27ee48d3a41 Mon Sep 17 00:00:00 2001
+From: Lorn Potter <lorn.potter@gmail.com>
+Date: Fri, 17 Jul 2015 15:32:23 +1000
+Subject: [PATCH] Make sure networkAccessibilityChanged is emitted
+
+Task-number: QTBUG-46323
+Change-Id: I8297072b62763136f457ca6ae15282d1c22244f4
+Reviewed-by: Timo Jyrinki <timo.jyrinki@canonical.com>
+Reviewed-by: Alex Blasche <alexander.blasche@theqtcompany.com>
+---
+ src/network/access/qnetworkaccessmanager.cpp       | 70 +++++++++++++++-------
+ src/network/access/qnetworkaccessmanager_p.h       | 14 ++++-
+ .../tst_qnetworkaccessmanager.cpp                  | 31 +++++-----
+ 3 files changed, 77 insertions(+), 38 deletions(-)
+
+diff --git a/src/network/access/qnetworkaccessmanager.cpp b/src/network/access/qnetworkaccessmanager.cpp
+index 84931cb..f9e9513 100644
+--- a/src/network/access/qnetworkaccessmanager.cpp
++++ b/src/network/access/qnetworkaccessmanager.cpp
+@@ -278,7 +278,8 @@ static void ensureInitialized()
+ 
+     \snippet code/src_network_access_qnetworkaccessmanager.cpp 4
+ 
+-    Network requests can be reenabled again by calling
++    Network requests can be re-enabled again, and this property will resume to
++    reflect the actual device state by calling
+ 
+     \snippet code/src_network_access_qnetworkaccessmanager.cpp 5
+ 
+@@ -467,16 +468,12 @@ QNetworkAccessManager::QNetworkAccessManager(QObject *parent)
+     qRegisterMetaType<QSharedPointer<char> >();
+ 
+ #ifndef QT_NO_BEARERMANAGEMENT
+-    if (!d->networkSessionRequired) {
+-        // if a session is required, we track online state through
+-        // the QNetworkSession's signals
+-        connect(&d->networkConfigurationManager, SIGNAL(onlineStateChanged(bool)),
+-                SLOT(_q_onlineStateChanged(bool)));
+-    }
+-    // we would need all active configurations to check for
+-    // d->networkConfigurationManager.isOnline(), which is asynchronous
+-    // and potentially expensive. We can just check the configuration here
+-    d->online = (d->networkConfiguration.state() & QNetworkConfiguration::Active);
++    // if a session is required, we track online state through
++    // the QNetworkSession's signals if a request is already made.
++    // we need to track current accessibility state by default
++    //
++    connect(&d->networkConfigurationManager, SIGNAL(onlineStateChanged(bool)),
++            SLOT(_q_onlineStateChanged(bool)));
+ #endif
+ }
+ 
+@@ -946,7 +943,8 @@ QNetworkConfiguration QNetworkAccessManager::activeConfiguration() const
+ void QNetworkAccessManager::setNetworkAccessible(QNetworkAccessManager::NetworkAccessibility accessible)
+ {
+     Q_D(QNetworkAccessManager);
+-    d->defaultAccessControl = false;
++
++    d->defaultAccessControl = accessible == NotAccessible ? false : true;
+ 
+     if (d->networkAccessible != accessible) {
+         NetworkAccessibility previous = networkAccessible();
+@@ -965,6 +963,10 @@ void QNetworkAccessManager::setNetworkAccessible(QNetworkAccessManager::NetworkA
+ QNetworkAccessManager::NetworkAccessibility QNetworkAccessManager::networkAccessible() const
+ {
+     Q_D(const QNetworkAccessManager);
++
++    if (d->networkConfiguration.state().testFlag(QNetworkConfiguration::Undefined))
++        return UnknownAccessibility;
++
+     if (d->networkSessionRequired) {
+         QSharedPointer<QNetworkSession> networkSession(d->getNetworkSession());
+         if (networkSession) {
+@@ -1622,32 +1624,56 @@ void QNetworkAccessManagerPrivate::_q_networkSessionStateChanged(QNetworkSession
+     if (online) {
+         if (state != QNetworkSession::Connected && state != QNetworkSession::Roaming) {
+             online = false;
+-            networkAccessible = QNetworkAccessManager::NotAccessible;
+-            emit q->networkAccessibleChanged(networkAccessible);
++            if (networkAccessible != QNetworkAccessManager::NotAccessible) {
++                networkAccessible = QNetworkAccessManager::NotAccessible;
++                emit q->networkAccessibleChanged(networkAccessible);
++            }
+         }
+     } else {
+         if (state == QNetworkSession::Connected || state == QNetworkSession::Roaming) {
+             online = true;
+             if (defaultAccessControl)
+-                networkAccessible = QNetworkAccessManager::Accessible;
+-            emit q->networkAccessibleChanged(networkAccessible);
++                if (networkAccessible != QNetworkAccessManager::Accessible) {
++                    networkAccessible = QNetworkAccessManager::Accessible;
++                    emit q->networkAccessibleChanged(networkAccessible);
++                }
+         }
+     }
+ }
+ 
+ void QNetworkAccessManagerPrivate::_q_onlineStateChanged(bool isOnline)
+ {
+-    // if the user set a config, we only care whether this one is active.
++   Q_Q(QNetworkAccessManager);
++   // if the user set a config, we only care whether this one is active.
+     // Otherwise, this QNAM is online if there is an online config.
+     if (customNetworkConfiguration) {
+         online = (networkConfiguration.state() & QNetworkConfiguration::Active);
+     } else {
+-        if (isOnline && online != isOnline) {
+-            networkSessionStrongRef.clear();
+-            networkSessionWeakRef.clear();
++        if (online != isOnline) {
++            if (isOnline) {
++                networkSessionStrongRef.clear();
++                networkSessionWeakRef.clear();
++            }
++            online = isOnline;
++        }
++    }
++    if (online) {
++        if (defaultAccessControl) {
++            if (networkAccessible != QNetworkAccessManager::Accessible) {
++                networkAccessible = QNetworkAccessManager::Accessible;
++                emit q->networkAccessibleChanged(networkAccessible);
++            }
++        }
++    } else if (networkConfiguration.state().testFlag(QNetworkConfiguration::Undefined)) {
++        if (networkAccessible != QNetworkAccessManager::UnknownAccessibility) {
++            networkAccessible = QNetworkAccessManager::UnknownAccessibility;
++            emit q->networkAccessibleChanged(networkAccessible);
++        }
++    } else {
++        if (networkAccessible != QNetworkAccessManager::NotAccessible) {
++            networkAccessible = QNetworkAccessManager::NotAccessible;
++            emit q->networkAccessibleChanged(networkAccessible);
+         }
+-
+-        online = isOnline;
+     }
+ }
+ 
+diff --git a/src/network/access/qnetworkaccessmanager_p.h b/src/network/access/qnetworkaccessmanager_p.h
+index c715da0..54ae114 100644
+--- a/src/network/access/qnetworkaccessmanager_p.h
++++ b/src/network/access/qnetworkaccessmanager_p.h
+@@ -78,7 +78,6 @@ public:
+           customNetworkConfiguration(false),
+           networkSessionRequired(networkConfigurationManager.capabilities()
+                                  & QNetworkConfigurationManager::NetworkSessionRequired),
+-          networkAccessible(QNetworkAccessManager::Accessible),
+           activeReplyCount(0),
+           online(false),
+           initializeSession(true),
+@@ -86,7 +85,18 @@ public:
+           cookieJarCreated(false),
+           defaultAccessControl(true),
+           authenticationManager(QSharedPointer<QNetworkAccessAuthenticationManager>::create())
+-    { }
++    {
++#ifndef QT_NO_BEARERMANAGEMENT
++        // we would need all active configurations to check for
++        // d->networkConfigurationManager.isOnline(), which is asynchronous
++        // and potentially expensive. We can just check the configuration here
++        online = (networkConfiguration.state().testFlag(QNetworkConfiguration::Active));
++        if (online)
++            networkAccessible = QNetworkAccessManager::Accessible;
++        else
++            networkAccessible = QNetworkAccessManager::NotAccessible;
++#endif
++    }
+     ~QNetworkAccessManagerPrivate();
+ 
+     void _q_replyFinished();
+diff --git a/tests/auto/network/access/qnetworkaccessmanager/tst_qnetworkaccessmanager.cpp b/tests/auto/network/access/qnetworkaccessmanager/tst_qnetworkaccessmanager.cpp
+index b4e4b9c..8ecb57d 100644
+--- a/tests/auto/network/access/qnetworkaccessmanager/tst_qnetworkaccessmanager.cpp
++++ b/tests/auto/network/access/qnetworkaccessmanager/tst_qnetworkaccessmanager.cpp
+@@ -74,6 +74,10 @@ void tst_QNetworkAccessManager::networkAccessible()
+     // if there is no session, we cannot know in which state we are in
+     QNetworkAccessManager::NetworkAccessibility initialAccessibility =
+             manager.networkAccessible();
++
++    if (initialAccessibility == QNetworkAccessManager::UnknownAccessibility)
++          QSKIP("Unknown accessibility", SkipAll);
++
+     QCOMPARE(manager.networkAccessible(), initialAccessibility);
+ 
+     manager.setNetworkAccessible(QNetworkAccessManager::NotAccessible);
+@@ -94,29 +98,28 @@ void tst_QNetworkAccessManager::networkAccessible()
+     QCOMPARE(manager.networkAccessible(), initialAccessibility);
+ 
+     QNetworkConfigurationManager configManager;
+-    bool sessionRequired = (configManager.capabilities()
+-                            & QNetworkConfigurationManager::NetworkSessionRequired);
+     QNetworkConfiguration defaultConfig = configManager.defaultConfiguration();
+     if (defaultConfig.isValid()) {
+         manager.setConfiguration(defaultConfig);
+ 
+-        // the accessibility has not changed if no session is required
+-        if (sessionRequired) {
++        QCOMPARE(spy.count(), 0);
++
++        if (defaultConfig.state().testFlag(QNetworkConfiguration::Active))
++            QCOMPARE(manager.networkAccessible(), QNetworkAccessManager::Accessible);
++        else
++            QCOMPARE(manager.networkAccessible(), QNetworkAccessManager::NotAccessible);
++
++        manager.setNetworkAccessible(QNetworkAccessManager::NotAccessible);
++
++        if (defaultConfig.state().testFlag(QNetworkConfiguration::Active)) {
+             QCOMPARE(spy.count(), 1);
+-            QCOMPARE(spy.takeFirst().at(0).value<QNetworkAccessManager::NetworkAccessibility>(),
+-                     QNetworkAccessManager::Accessible);
++            QCOMPARE(QNetworkAccessManager::NetworkAccessibility(spy.takeFirst().at(0).toInt()),
++                     QNetworkAccessManager::NotAccessible);
+         } else {
+             QCOMPARE(spy.count(), 0);
+         }
+-        QCOMPARE(manager.networkAccessible(), QNetworkAccessManager::Accessible);
+-
+-        manager.setNetworkAccessible(QNetworkAccessManager::NotAccessible);
+-
+-        QCOMPARE(spy.count(), 1);
+-        QCOMPARE(QNetworkAccessManager::NetworkAccessibility(spy.takeFirst().at(0).toInt()),
+-                 QNetworkAccessManager::NotAccessible);
+-        QCOMPARE(manager.networkAccessible(), QNetworkAccessManager::NotAccessible);
+     }
++    QCOMPARE(manager.networkAccessible(), QNetworkAccessManager::NotAccessible);
+ #endif
+ }
+ 
+-- 
+2.6.2
+
diff --git a/debian/patches/qtbug-47471.patch b/debian/patches/qtbug-47471.patch
new file mode 100644
index 0000000..2588678
--- /dev/null
+++ b/debian/patches/qtbug-47471.patch
@@ -0,0 +1,146 @@
+From f98c2ef27a4f6fa3b7e9c35cf7896abc4b22816b Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Sebastian=20L=C3=B6sch?= <sebastian.loesch@governikus.com>
+Date: Mon, 10 Aug 2015 12:47:04 +0200
+Subject: [PATCH] Abort underlying socket when aborting QNetworkReply
+
+If we abort a connection in QNetworkReply::encrypted the underlying
+socket gets flushed. This patch fixes that no data will be transmitted
+after someone called abort().
+
+Change-Id: I59306e69cb9f2e1421b324e11947375130e52135
+Task-number: QTBUG-47471
+Reviewed-by: Markus Goetz (Woboq GmbH) <markus@woboq.com>
+Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
+---
+ src/network/access/qhttpnetworkconnection.cpp        |  9 +++++++--
+ src/network/access/qhttpnetworkconnectionchannel.cpp | 20 ++++++++++++++++++++
+ src/network/access/qhttpnetworkconnectionchannel_p.h |  1 +
+ src/network/access/qhttpnetworkreply.cpp             | 11 +++++++++++
+ src/network/access/qhttpnetworkreply_p.h             |  6 +++++-
+ src/network/access/qhttpthreaddelegate.cpp           |  1 +
+ 6 files changed, 45 insertions(+), 3 deletions(-)
+
+diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp
+index b7d17be..f810df5 100644
+--- a/src/network/access/qhttpnetworkconnection.cpp
++++ b/src/network/access/qhttpnetworkconnection.cpp
+@@ -835,8 +835,13 @@ void QHttpNetworkConnectionPrivate::removeReply(QHttpNetworkReply *reply)
+             // if HTTP mandates we should close
+             // or the reply is not finished yet, e.g. it was aborted
+             // we have to close that connection
+-            if (reply->d_func()->isConnectionCloseEnabled() || !reply->isFinished())
+-                channels[i].close();
++            if (reply->d_func()->isConnectionCloseEnabled() || !reply->isFinished()) {
++                if (reply->isAborted()) {
++                    channels[i].abort();
++                } else {
++                    channels[i].close();
++                }
++            }
+ 
+             QMetaObject::invokeMethod(q, "_q_startNextRequest", Qt::QueuedConnection);
+             return;
+diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp
+index 477cba2..0820a8d 100644
+--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
++++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
+@@ -205,6 +205,26 @@ void QHttpNetworkConnectionChannel::close()
+ }
+ 
+ 
++void QHttpNetworkConnectionChannel::abort()
++{
++    if (!socket)
++        state = QHttpNetworkConnectionChannel::IdleState;
++    else if (socket->state() == QAbstractSocket::UnconnectedState)
++        state = QHttpNetworkConnectionChannel::IdleState;
++    else
++        state = QHttpNetworkConnectionChannel::ClosingState;
++
++    // pendingEncrypt must only be true in between connected and encrypted states
++    pendingEncrypt = false;
++
++    if (socket) {
++        // socket can be 0 since the host lookup is done from qhttpnetworkconnection.cpp while
++        // there is no socket yet.
++        socket->abort();
++    }
++}
++
++
+ bool QHttpNetworkConnectionChannel::sendRequest()
+ {
+     Q_ASSERT(!protocolHandler.isNull());
+diff --git a/src/network/access/qhttpnetworkconnectionchannel_p.h b/src/network/access/qhttpnetworkconnectionchannel_p.h
+index 37ad6c9..87329b7 100644
+--- a/src/network/access/qhttpnetworkconnectionchannel_p.h
++++ b/src/network/access/qhttpnetworkconnectionchannel_p.h
+@@ -157,6 +157,7 @@ public:
+ 
+     void init();
+     void close();
++    void abort();
+ 
+     bool sendRequest();
+ 
+diff --git a/src/network/access/qhttpnetworkreply.cpp b/src/network/access/qhttpnetworkreply.cpp
+index 80f3670..b744a99 100644
+--- a/src/network/access/qhttpnetworkreply.cpp
++++ b/src/network/access/qhttpnetworkreply.cpp
+@@ -247,6 +247,17 @@ char* QHttpNetworkReply::userProvidedDownloadBuffer()
+     return d->userProvidedDownloadBuffer;
+ }
+ 
++void QHttpNetworkReply::abort()
++{
++    Q_D(QHttpNetworkReply);
++    d->state = QHttpNetworkReplyPrivate::Aborted;
++}
++
++bool QHttpNetworkReply::isAborted() const
++{
++    return d_func()->state == QHttpNetworkReplyPrivate::Aborted;
++}
++
+ bool QHttpNetworkReply::isFinished() const
+ {
+     return d_func()->state == QHttpNetworkReplyPrivate::AllDoneState;
+diff --git a/src/network/access/qhttpnetworkreply_p.h b/src/network/access/qhttpnetworkreply_p.h
+index 0fe298d..46b6541 100644
+--- a/src/network/access/qhttpnetworkreply_p.h
++++ b/src/network/access/qhttpnetworkreply_p.h
+@@ -121,6 +121,9 @@ public:
+     void setUserProvidedDownloadBuffer(char*);
+     char* userProvidedDownloadBuffer();
+ 
++    void abort();
++
++    bool isAborted() const;
+     bool isFinished() const;
+ 
+     bool isPipeliningUsed() const;
+@@ -205,7 +208,8 @@ public:
+         SPDYSYNSent,
+         SPDYUploading,
+         SPDYHalfClosed,
+-        SPDYClosed
++        SPDYClosed,
++        Aborted
+     } state;
+ 
+     QHttpNetworkRequest request;
+diff --git a/src/network/access/qhttpthreaddelegate.cpp b/src/network/access/qhttpthreaddelegate.cpp
+index be6fa01..e4931db 100644
+--- a/src/network/access/qhttpthreaddelegate.cpp
++++ b/src/network/access/qhttpthreaddelegate.cpp
+@@ -396,6 +396,7 @@ void QHttpThreadDelegate::abortRequest()
+     qDebug() << "QHttpThreadDelegate::abortRequest() thread=" << QThread::currentThreadId() << "sync=" << synchronous;
+ #endif
+     if (httpReply) {
++        httpReply->abort();
+         delete httpReply;
+         httpReply = 0;
+     }
+-- 
+2.6.2
+
diff --git a/debian/patches/series b/debian/patches/series
index 540ebe0..77ee405 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -3,6 +3,10 @@ set_positionautomatic.diff
 set_WA_OutsideWSRange_for_native_widgets.patch
 bsd_volumeinfo.diff
 hurd_forkfd.diff
+qtbug-47471.patch
+qtbug-46323.patch
+
+# Will be no longer needed with Qt 5.7
 mips_no_atomic.diff
 detect_64bit_atomic.diff
 
-- 
2.6.2

Attachment: signature.asc
Description: PGP signature


Reply to: