diff options
author | Ivan Solovev <ivan.solovev@qt.io> | 2021-05-12 16:58:09 +0200 |
---|---|---|
committer | Ivan Solovev <ivan.solovev@qt.io> | 2021-05-20 13:06:24 +0200 |
commit | 6cc553b1579f6448e2a2734bbb4348e30b6ff09e (patch) | |
tree | 58e67ac210ef9ac8372d951e9b9c2f172f6e21ac /tests/auto | |
parent | 668fb8be4396b944e6b0425f8ccbb5e284944c34 (diff) | |
download | qtlocation-6cc553b1579f6448e2a2734bbb4348e30b6ff09e.tar.gz |
QGeoAreaMonitorPolling: fix connection check logic
QGeoAreaMonitorPolling class overrides QObject::connectNotify() and
QObject::disconnectNotify() to check, if it has any connected objects
and decide, if it needs to start/stop position monitoring.
Previously it used QObject::isSignalConnected() to perform the check.
However this method locks a QObject's mutex, so it's not safe to call
it from QObject::disconnectNotify(), because the latter is called
from a QObject's destructor, which is also locking a QObject's mutex.
Both locks use signalSlotLock static method from qobject.cpp to
determine, which mutex to lock. The selection is made based on the
object's address. At some rare case this can lead to selecting the
same mutex. And as a result we get a deadlock, when trying to lock
the same mutex for the second time.
This patch updates the logic of the overridden methods in
QGeoAreaMonitorPolling. They do not use the
QObject::isSignalConnected() method, but implement a custom solution
to track the number of connections. This solution requires
synchronization, because connectNotify() and disconnectNotify() can
be triggered from different threads.
Fixes: QTBUG-93420
Fixes: QTBUG-91434
Change-Id: I45ba03e238edc136f1bb42ca6b11a528ab5c22e8
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>
Diffstat (limited to 'tests/auto')
-rw-r--r-- | tests/auto/qgeoareamonitor/CMakeLists.txt | 1 | ||||
-rw-r--r-- | tests/auto/qgeoareamonitor/logfilepositionsource.cpp | 13 | ||||
-rw-r--r-- | tests/auto/qgeoareamonitor/logfilepositionsource.h | 6 | ||||
-rw-r--r-- | tests/auto/qgeoareamonitor/positionconsumerthread.cpp | 73 | ||||
-rw-r--r-- | tests/auto/qgeoareamonitor/positionconsumerthread.h | 74 | ||||
-rw-r--r-- | tests/auto/qgeoareamonitor/tst_qgeoareamonitor.cpp | 75 |
6 files changed, 239 insertions, 3 deletions
diff --git a/tests/auto/qgeoareamonitor/CMakeLists.txt b/tests/auto/qgeoareamonitor/CMakeLists.txt index bc99de80..de39e942 100644 --- a/tests/auto/qgeoareamonitor/CMakeLists.txt +++ b/tests/auto/qgeoareamonitor/CMakeLists.txt @@ -7,6 +7,7 @@ qt_internal_add_test(tst_qgeoareamonitor SOURCES logfilepositionsource.cpp logfilepositionsource.h + positionconsumerthread.cpp positionconsumerthread.h tst_qgeoareamonitor.cpp PUBLIC_LIBRARIES Qt::Core diff --git a/tests/auto/qgeoareamonitor/logfilepositionsource.cpp b/tests/auto/qgeoareamonitor/logfilepositionsource.cpp index 4cfa4348..c7cdc93c 100644 --- a/tests/auto/qgeoareamonitor/logfilepositionsource.cpp +++ b/tests/auto/qgeoareamonitor/logfilepositionsource.cpp @@ -64,12 +64,18 @@ void LogFilePositionSource::startUpdates() if (interval < minimumUpdateInterval()) interval = minimumUpdateInterval(); - timer->start(interval); + if (!timer->isActive()) { + if (QMetaObject::invokeMethod(timer, "start", Q_ARG(int, interval))) + emit updatesStarted(); + } } void LogFilePositionSource::stopUpdates() { - timer->stop(); + if (timer->isActive()) { + if (QMetaObject::invokeMethod(timer, "stop")) + emit updatesStopped(); + } } void LogFilePositionSource::requestUpdate(int /*timeout*/) @@ -109,6 +115,9 @@ void LogFilePositionSource::readNextPosition() } } index++; + } else if (!noDataEmitted) { + emit noDataLeft(); + noDataEmitted = true; } } diff --git a/tests/auto/qgeoareamonitor/logfilepositionsource.h b/tests/auto/qgeoareamonitor/logfilepositionsource.h index 5d21b17a..b175f7e9 100644 --- a/tests/auto/qgeoareamonitor/logfilepositionsource.h +++ b/tests/auto/qgeoareamonitor/logfilepositionsource.h @@ -53,6 +53,11 @@ public: int minimumUpdateInterval() const override; Error error() const override; +signals: + void noDataLeft(); + void updatesStarted(); + void updatesStopped(); + public slots: virtual void startUpdates() override; virtual void stopUpdates() override; @@ -70,6 +75,7 @@ private: Error lastError = QGeoPositionInfoSource::NoError; const QList<QByteArray> &lines; qsizetype index = -1; + bool noDataEmitted = false; }; #endif diff --git a/tests/auto/qgeoareamonitor/positionconsumerthread.cpp b/tests/auto/qgeoareamonitor/positionconsumerthread.cpp new file mode 100644 index 00000000..094e4488 --- /dev/null +++ b/tests/auto/qgeoareamonitor/positionconsumerthread.cpp @@ -0,0 +1,73 @@ +/**************************************************************************** +** +** Copyright (C) 2021 The Qt Company Ltd. +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:GPL-EXCEPT$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3 as published by the Free Software +** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT +** included in the packaging of this file. Please review the following +** information to ensure the GNU General Public License requirements will +** be met: https://www.gnu.org/licenses/gpl-3.0.html. +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#include <QSignalSpy> +#include <QtPositioning/qgeoareamonitorsource.h> +#include "positionconsumerthread.h" + +PositionConsumerThread::PositionConsumerThread(QGeoAreaMonitorSource *source, QObject *parent) + : QThread(parent), m_source(source) +{ +} + +PositionConsumerThread::~PositionConsumerThread() +{ + stopProcessing(); + wait(); +} + +int PositionConsumerThread::detectedEnterCount() const +{ + QMutexLocker locker(&m_mutex); + return m_detectedEnterCount; +} + +int PositionConsumerThread::detectedExitCount() const +{ + QMutexLocker locker(&m_mutex); + return m_detectedExitCount; +} + +void PositionConsumerThread::stopProcessing() +{ + m_mutex.lock(); + m_waitCondition.wakeOne(); + m_mutex.unlock(); +} + +void PositionConsumerThread::run() +{ + QSignalSpy enterSpy(m_source, &QGeoAreaMonitorSource::areaEntered); + QSignalSpy exitSpy(m_source, &QGeoAreaMonitorSource::areaExited); + + m_mutex.lock(); + m_waitCondition.wait(&m_mutex); + m_detectedEnterCount = enterSpy.count(); + m_detectedExitCount = exitSpy.count(); + m_mutex.unlock(); +} diff --git a/tests/auto/qgeoareamonitor/positionconsumerthread.h b/tests/auto/qgeoareamonitor/positionconsumerthread.h new file mode 100644 index 00000000..acf5a933 --- /dev/null +++ b/tests/auto/qgeoareamonitor/positionconsumerthread.h @@ -0,0 +1,74 @@ +/**************************************************************************** +** +** Copyright (C) 2021 The Qt Company Ltd. +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:GPL-EXCEPT$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3 as published by the Free Software +** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT +** included in the packaging of this file. Please review the following +** information to ensure the GNU General Public License requirements will +** be met: https://www.gnu.org/licenses/gpl-3.0.html. +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#ifndef POSITIONCONSUMERTHREAD_H +#define POSITIONCONSUMERTHREAD_H + +#include <QThread> +#include <QMutex> +#include <QWaitCondition> + +QT_BEGIN_NAMESPACE +class QGeoAreaMonitorSource; +QT_END_NAMESPACE + +// This class is created to test the behavior of QGeoAreaMonitorPolling class, +// that reimplements the connectNotify() and disconnetNotify() methods, by +// triggering these methods from multiple threads. +// The thread creates two QSignalSpy instances, that connect to the signals of +// QGeoAreaMonitorSource. Once constructed, they trigger connectNotify(). +// Once destroyed, they trigger disconnectNotify. +// With the previous implementation of these overridden methods, that could lead +// to a deadlock in a rare case. +class PositionConsumerThread : public QThread +{ + Q_OBJECT +public: + explicit PositionConsumerThread(QGeoAreaMonitorSource *source, QObject *parent = nullptr); + ~PositionConsumerThread(); + + int detectedEnterCount() const; + int detectedExitCount() const; + +public slots: + void stopProcessing(); + +protected: + void run() override; + +private: + QGeoAreaMonitorSource *m_source; + + int m_detectedEnterCount = 0; + int m_detectedExitCount = 0; + + mutable QMutex m_mutex; + QWaitCondition m_waitCondition; +}; + +#endif // POSITIONCONSUMERTHREAD_H diff --git a/tests/auto/qgeoareamonitor/tst_qgeoareamonitor.cpp b/tests/auto/qgeoareamonitor/tst_qgeoareamonitor.cpp index ec0ec2b7..aad664a8 100644 --- a/tests/auto/qgeoareamonitor/tst_qgeoareamonitor.cpp +++ b/tests/auto/qgeoareamonitor/tst_qgeoareamonitor.cpp @@ -48,7 +48,7 @@ #include <QtPositioning/qgeorectangle.h> #include "logfilepositionsource.h" - +#include "positionconsumerthread.h" QT_USE_NAMESPACE #define UPDATE_INTERVAL 50 @@ -803,6 +803,79 @@ private slots: qInstallMessageHandler(0); QCOMPARE(tst_qgeoareamonitorinfo_debug, debugString); } + + void multipleThreads() + { + std::unique_ptr<QGeoAreaMonitorSource> obj( + QGeoAreaMonitorSource::createSource(QStringLiteral("positionpoll"), 0)); + QVERIFY(obj != nullptr); + QCOMPARE(obj->sourceName(), QStringLiteral("positionpoll")); + + QVERIFY(obj->activeMonitors().isEmpty()); + + LogFilePositionSource *source = new LogFilePositionSource(m_fileData, this); + source->setUpdateInterval(UPDATE_INTERVAL); + obj->setPositionInfoSource(source); + QCOMPARE(obj->positionInfoSource(), source); + + QSignalSpy noDataSpy(source, &LogFilePositionSource::noDataLeft); + QSignalSpy updatesStartedSpy(source, &LogFilePositionSource::updatesStarted); + QSignalSpy updatesStoppedSpy(source, &LogFilePositionSource::updatesStopped); + + // generate threads + const int threadCount = 10; + QList<PositionConsumerThread *> threads; + for (int i = 0; i < threadCount; ++i) { + auto threadObj = new PositionConsumerThread(obj.get(), this); + threadObj->start(); + threads.push_back(threadObj); + } + + // generate objects to monitor + QGeoAreaMonitorInfo infoRectangle("Rectangle"); + infoRectangle.setArea(QGeoRectangle(QGeoCoordinate(-27.65, 153.093), 0.2, 0.2)); + QVERIFY(infoRectangle.isValid()); + QVERIFY(obj->startMonitoring(infoRectangle)); + + QGeoAreaMonitorInfo infoCircle("Circle"); + infoCircle.setArea(QGeoCircle(QGeoCoordinate(-27.70, 153.093), 10000)); + QVERIFY(infoCircle.isValid()); + QVERIFY(obj->startMonitoring(infoCircle)); + + QGeoAreaMonitorInfo singleShot_enter("SingleShot_on_Entered"); + singleShot_enter.setArea(QGeoRectangle(QGeoCoordinate(-27.67, 153.093), 0.2, 0.2)); + QVERIFY(singleShot_enter.isValid()); + QVERIFY(obj->requestUpdate(singleShot_enter, + SIGNAL(areaEntered(QGeoAreaMonitorInfo, QGeoPositionInfo)))); + + QGeoAreaMonitorInfo singleShot_exit("SingleShot_on_Exited"); + singleShot_exit.setArea(QGeoRectangle(QGeoCoordinate(-27.70, 153.093), 0.2, 0.2)); + QVERIFY(singleShot_exit.isValid()); + QVERIFY(obj->requestUpdate(singleShot_exit, + SIGNAL(areaExited(QGeoAreaMonitorInfo, QGeoPositionInfo)))); + + // wait until we read all data + QTRY_COMPARE_WITH_TIMEOUT(noDataSpy.count(), 1, 5000); + + // first request all the threads to terminate + for (int i = 0; i < threadCount; ++i) + threads[i]->stopProcessing(); + + static const int Number_Of_Entered_Events = 6; + static const int Number_Of_Exited_Events = 5; + // wait until each thread is stopped, and compare the result values + for (int i = 0; i < threadCount; ++i) { + threads[i]->wait(); + QCOMPARE(threads[i]->detectedEnterCount(), Number_Of_Entered_Events); + QCOMPARE(threads[i]->detectedExitCount(), Number_Of_Exited_Events); + } + + // Verify that the source started and stopped updates only once. + // This is needed to check that the connection tracking logic in + // connectNotify()/disconnectNotify() is working properly. + QCOMPARE(updatesStartedSpy.count(), 1); + QCOMPARE(updatesStoppedSpy.count(), 1); + } }; |