QSignalSpy: fix data race between wait() and emit from another thread

Detected by TSAN in tst_QThread::terminateAndPrematureDestruction()
but better have a dedicated unittest, with values emitted by the signal
and recorded in the spy.

Pick-to: 6.5
Change-Id: I141d47b0ea0b63188f8a4f9d056f72df3457bda5
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit c837cd75936cbeeb898dd5808edb9dfaf716a76e)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 3b428d6f28a3664220d4c7eef8f8ee1779d206e2)

src/testlib/qsignalspy.h

@@ -10,6 +10,7 @@
 #include <QtCore/qmetaobject.h>
 #include <QtTest/qtesteventloop.h>
 #include <QtCore/qvariant.h>
+#include <QtCore/qmutex.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -43,11 +44,11 @@ public:
             return;
         }
 
+        initArgs(mo->method(sigIndex), obj);
         if (!connectToSignal(obj, sigIndex))
             return;
 
         sig = ba;
-        initArgs(mo->method(sigIndex), obj);
     }
 
 #ifdef Q_QDOC
@@ -73,21 +74,23 @@ public:
         if (!isSignalMetaMethodValid(signalMetaMethod))
             return;
 
+        initArgs(mo->method(sigIndex), obj);
         if (!connectToSignal(obj, sigIndex))
             return;
 
         sig = signalMetaMethod.methodSignature();
-        initArgs(mo->method(sigIndex), obj);
     }
 #endif // Q_QDOC
 
     QSignalSpy(const QObject *obj, const QMetaMethod &signal)
         : m_waiting(false)
     {
-        if (isObjectValid(obj) && isSignalMetaMethodValid(signal) &&
-            connectToSignal(obj, signal.methodIndex())) {
-            sig = signal.methodSignature();
+        if (isObjectValid(obj) && isSignalMetaMethodValid(signal)) {
             initArgs(signal, obj);
+            if (!connectToSignal(obj, signal.methodIndex()))
+                return;
+
+            sig = signal.methodSignature();
         }
     }
 
@@ -99,10 +102,15 @@ public:
 
     bool wait(std::chrono::milliseconds timeout = std::chrono::seconds{5})
     {
+        QMutexLocker locker(&m_mutex);
         Q_ASSERT(!m_waiting);
         const qsizetype origCount = size();
         m_waiting = true;
+        locker.unlock();
+
         m_loop.enterLoop(timeout);
+
+        locker.relock();
         m_waiting = false;
         return size() > origCount;
     }
@@ -157,14 +165,17 @@ private:
 
     void initArgs(const QMetaMethod &member, const QObject *obj)
     {
+        QMutexLocker locker(&m_mutex);
         args.reserve(member.parameterCount());
         for (int i = 0; i < member.parameterCount(); ++i) {
             QMetaType tp = member.parameterMetaType(i);
             if (!tp.isValid() && obj) {
+                locker.unlock();
                 void *argv[] = { &tp, &i };
                 QMetaObject::metacall(const_cast<QObject*>(obj),
                                       QMetaObject::RegisterMethodArgumentMetaType,
                                       member.methodIndex(), argv);
+                locker.relock();
             }
             if (!tp.isValid()) {
                 qWarning("QSignalSpy: Unable to handle parameter '%s' of type '%s' of method '%s',"
@@ -179,6 +190,7 @@ private:
 
     void appendArgs(void **a)
     {
+        QMutexLocker locker(&m_mutex);
         QList<QVariant> list;
         list.reserve(args.size());
         for (int i = 0; i < args.size(); ++i) {
@@ -190,8 +202,10 @@ private:
         }
         append(list);
 
-        if (m_waiting)
+        if (m_waiting) {
+            locker.unlock();
             m_loop.exitLoop();
+        }
     }
 
     // the full, normalized signal name
@@ -201,6 +215,7 @@ private:
 
     QTestEventLoop m_loop;
     bool m_waiting;
+    static inline QMutex m_mutex; // protects m_waiting, args and the QList base class, between appendArgs() and wait()
 };
 
 QT_END_NAMESPACE

tests/auto/testlib/qsignalspy/tst_qsignalspy.cpp

@@ -6,9 +6,11 @@
 #include <QSignalSpy>
 #include <QTimer>
 
-
 #include <qdatetime.h>
 
+using namespace std::chrono_literals;
+using namespace Qt::StringLiterals;
+
 class tst_QSignalSpy : public QObject
 {
     Q_OBJECT
@@ -48,6 +50,8 @@ private slots:
 
     void spyOnMetaMethod_invalid();
     void spyOnMetaMethod_invalid_data();
+
+    void signalSpyDoesNotRaceOnCrossThreadSignal();
 };
 
 struct CustomType {};
@@ -485,5 +489,31 @@ void tst_QSignalSpy::spyOnMetaMethod_invalid_data()
         << QObject::staticMetaObject.method(QObject::staticMetaObject.indexOfMethod("deleteLater()"));
 }
 
+class EmitSignal_Thread : public QThread
+{
+    Q_OBJECT
+public:
+    void run() override
+    {
+        emit valueChanged(42, u"is the answer"_s);
+    }
+
+Q_SIGNALS:
+    void valueChanged(int value, const QString &str);
+};
+
+void tst_QSignalSpy::signalSpyDoesNotRaceOnCrossThreadSignal()
+{
+    EmitSignal_Thread thread;
+    QSignalSpy valueChangedSpy(&thread, &EmitSignal_Thread::valueChanged);
+    QVERIFY(valueChangedSpy.isValid());
+
+    thread.start();
+    QVERIFY(valueChangedSpy.wait(5s));
+    QCOMPARE(valueChangedSpy[0][0].toInt(), 42);
+    QCOMPARE(valueChangedSpy[0][1].toString(), u"is the answer"_s);
+    QVERIFY(thread.wait(5s));
+}
+
 QTEST_MAIN(tst_QSignalSpy)
 #include "tst_qsignalspy.moc"