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>

8 files changed, 272 insertions, 25 deletions

diff --git a/src/plugins/position/positionpoll/qgeoareamonitor_polling.cpp b/src/plugins/position/positionpoll/qgeoareamonitor_polling.cpp
index fbe26d63..e69516c8 100644
--- a/src/plugins/position/positionpoll/qgeoareamonitor_polling.cpp
+++ b/src/plugins/position/positionpoll/qgeoareamonitor_polling.cpp
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2021 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtPositioning module of the Qt Toolkit.
@@ -188,7 +188,7 @@ public:
 
         bool signalsConnected = false;
         foreach (const QGeoAreaMonitorPolling *client, registeredClients) {
-            if (client->signalsAreConnected) {
+            if (client->hasConnections()) {
                 signalsConnected = true;
                 break;
             }
@@ -316,9 +316,7 @@ private:
 
 Q_GLOBAL_STATIC(QGeoAreaMonitorPollingPrivate, pollingPrivate)
 
-
-QGeoAreaMonitorPolling::QGeoAreaMonitorPolling(QObject *parent)
-    : QGeoAreaMonitorSource(parent), signalsAreConnected(false)
+QGeoAreaMonitorPolling::QGeoAreaMonitorPolling(QObject *parent) : QGeoAreaMonitorSource(parent)
 {
     d = pollingPrivate();
     d->registerClient(this);
@@ -377,6 +375,12 @@ int QGeoAreaMonitorPolling::idForSignal(const char *signal)
     return mo->indexOfSignal(sig.constData());
 }
 
+bool QGeoAreaMonitorPolling::hasConnections() const
+{
+    // This method is internal and requires the mutex to be already locked.
+    return signalConnections > 0;
+}
+
 bool QGeoAreaMonitorPolling::requestUpdate(const QGeoAreaMonitorInfo &monitor, const char *signal)
 {
     if (!monitor.isValid())
@@ -443,24 +447,25 @@ QGeoAreaMonitorSource::AreaMonitorFeatures QGeoAreaMonitorPolling::supportedArea
     return {};
 }
 
-void QGeoAreaMonitorPolling::connectNotify(const QMetaMethod &/*signal*/)
+void QGeoAreaMonitorPolling::connectNotify(const QMetaMethod &signal)
 {
-    if (!signalsAreConnected &&
-        (isSignalConnected(areaEnteredSignal()) ||
-         isSignalConnected(areaExitedSignal())) )
-    {
-        signalsAreConnected = true;
-        d->checkStartStop();
+    QMutexLocker locker(&connectionMutex);
+    if (signal == areaEnteredSignal() || signal == areaExitedSignal()) {
+        const bool alreadyConnected = hasConnections();
+        signalConnections++;
+        if (!alreadyConnected)
+            d->checkStartStop();
     }
 }
 
-void QGeoAreaMonitorPolling::disconnectNotify(const QMetaMethod &/*signal*/)
+void QGeoAreaMonitorPolling::disconnectNotify(const QMetaMethod &signal)
 {
-    if (!isSignalConnected(areaEnteredSignal()) &&
-        !isSignalConnected(areaExitedSignal()))
-    {
-        signalsAreConnected = false;
-        d->checkStartStop();
+    QMutexLocker locker(&connectionMutex);
+    if (signal == areaEnteredSignal() || signal == areaExitedSignal()) {
+        if (hasConnections())
+            signalConnections--;
+        if (!hasConnections())
+            d->checkStartStop();
     }
 }
 
diff --git a/src/plugins/position/positionpoll/qgeoareamonitor_polling.h b/src/plugins/position/positionpoll/qgeoareamonitor_polling.h
index 4690c333..38a685d4 100644
--- a/src/plugins/position/positionpoll/qgeoareamonitor_polling.h
+++ b/src/plugins/position/positionpoll/qgeoareamonitor_polling.h
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2021 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtPositioning module of the Qt Toolkit.
@@ -42,7 +42,7 @@
 
 #include <QtPositioning/qgeoareamonitorsource.h>
 #include <QtPositioning/qgeopositioninfosource.h>
-
+#include <QtCore/qmutex.h>
 
 /**
  *  QGeoAreaMonitorPolling
@@ -74,8 +74,6 @@ public :
 
     inline bool isValid() { return positionInfoSource(); }
 
-    bool signalsAreConnected;
-
 private Q_SLOTS:
     void positionError(QGeoPositionInfoSource::Error error);
     void timeout(const QGeoAreaMonitorInfo &monitor);
@@ -84,11 +82,19 @@ private Q_SLOTS:
 private:
     QGeoAreaMonitorPollingPrivate* d;
     QGeoAreaMonitorSource::Error lastError = QGeoAreaMonitorSource::NoError;
+    friend class QGeoAreaMonitorPollingPrivate;
+
+    int signalConnections = 0;
+    // connectNotify() and disconnectNotify() can be called from a different
+    // thread, so we need to synchronize the access to signalConnections
+    QMutex connectionMutex;
 
     void connectNotify(const QMetaMethod &signal) override;
     void disconnectNotify(const QMetaMethod &signal) override;
 
     int idForSignal(const char *signal);
+
+    bool hasConnections() const;
 };
 
 #endif // QGEOAREAMONITORPOLLING_H
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);
+    }
 };