QTimer: fix new-style slot invocation for receiver in another thread

For single-shot timers, at least. QSingleShotTimer had either an
optimization or the only way to make new-style slot invocations by
storing the QSlotObject pointer and calling it directly. Instead of
doing that, let's just actually connect and let QObject handle the
actual delivery.

[ChangeLog][QtCore][QTimer] Fixed a bug that caused slots connected to
single-slot timers using the new-style connection mechanism to be
delivered in the wrong thread.

Fixes: QTBUG-112162
Pick-to: 5.15 6.2 6.5
Change-Id: Idd5e1bb52be047d7b4fffffd174eadb227ab65ee
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
Reviewed-by: Ahmad Samir <a.samirh78@gmail.com>

2 files changed, 21 insertions, 23 deletions

diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp
index 73f0fb8fee..9c074a064f 100644
--- a/src/corelib/kernel/qtimer.cpp
+++ b/src/corelib/kernel/qtimer.cpp
@@ -7,10 +7,11 @@
 
 #include "qabstracteventdispatcher.h"
 #include "qcoreapplication.h"
-#include "qobject_p.h"
-#include "qthread.h"
 #include "qcoreapplication_p.h"
+#include "qmetaobject_p.h"
+#include "qobject_p.h"
 #include "qproperty_p.h"
+#include "qthread.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -249,9 +250,6 @@ class QSingleShotTimer : public QObject
 {
     Q_OBJECT
     int timerId;
-    bool hasValidReceiver;
-    QPointer<const QObject> receiver;
-    QtPrivate::QSlotObjectBase *slotObj;
 public:
     ~QSingleShotTimer();
     QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char * m);
@@ -264,16 +262,23 @@ protected:
 };
 
 QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char *member)
-    : QObject(QAbstractEventDispatcher::instance()), hasValidReceiver(true), slotObj(nullptr)
+    : QObject(QAbstractEventDispatcher::instance())
 {
     timerId = startTimer(std::chrono::milliseconds{msec}, timerType);
     connect(this, SIGNAL(timeout()), r, member);
 }
 
 QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, QtPrivate::QSlotObjectBase *slotObj)
-    : QObject(QAbstractEventDispatcher::instance()), hasValidReceiver(r), receiver(r), slotObj(slotObj)
+    : QObject(QAbstractEventDispatcher::instance())
 {
     timerId = startTimer(std::chrono::milliseconds{msec}, timerType);
+
+    int signal_index = QMetaObjectPrivate::signalOffset(&staticMetaObject);
+    Q_ASSERT(QMetaObjectPrivate::signal(&staticMetaObject, signal_index).name() == "timeout");
+    QObjectPrivate::connectImpl(this, signal_index, r ? r : this, nullptr, slotObj,
+                                Qt::AutoConnection, nullptr, &staticMetaObject);
+
+    // ### Why is this here? Why doesn't the case above need it?
     if (r && thread() != r->thread()) {
         // Avoid leaking the QSingleShotTimer instance in case the application exits before the timer fires
         connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);
@@ -286,8 +291,6 @@ QSingleShotTimer::~QSingleShotTimer()
 {
     if (timerId > 0)
         killTimer(timerId);
-    if (slotObj)
-        slotObj->destroyIfLastRef();
 }
 
 void QSingleShotTimer::timerEvent(QTimerEvent *)
@@ -298,17 +301,7 @@ void QSingleShotTimer::timerEvent(QTimerEvent *)
         killTimer(timerId);
     timerId = -1;
 
-    if (slotObj) {
-        // If the receiver was destroyed, skip this part
-        if (Q_LIKELY(!receiver.isNull() || !hasValidReceiver)) {
-            // We allocate only the return type - we previously checked the function had
-            // no arguments.
-            void *args[1] = { nullptr };
-            slotObj->call(const_cast<QObject*>(receiver.data()), args);
-        }
-    } else {
-        emit timeout();
-    }
+    emit timeout();
 
     // we would like to use delete later here, but it feels like a
     // waste to post a new event to handle this event, so we just unset the flag
diff --git a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
index e2bde37a7e..5c39fc5d91 100644
--- a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
+++ b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
@@ -1006,18 +1006,21 @@ void tst_QTimer::postedEventsShouldNotStarveTimers()
 }
 
 struct DummyFunctor {
-    void operator()() {}
+    static QThread *callThread;
+    void operator()() { callThread = QThread::currentThread(); }
 };
+QThread *DummyFunctor::callThread = nullptr;
 
 void tst_QTimer::crossThreadSingleShotToFunctor()
 {
-    // We're testing for crashes here, so the test simply running to
-    // completion is considered a success
+    // We're also testing for crashes here, so the test simply running to
+    // completion is part of the success
     QThread t;
     t.start();
 
     QObject* o = new QObject();
     o->moveToThread(&t);
+    DummyFunctor::callThread = nullptr;
 
     for (int i = 0; i < 10000; i++) {
         QTimer::singleShot(0, o, DummyFunctor());
@@ -1026,6 +1029,8 @@ void tst_QTimer::crossThreadSingleShotToFunctor()
     t.quit();
     t.wait();
     delete o;
+
+    QCOMPARE(DummyFunctor::callThread, &t);
 }
 
 void tst_QTimer::callOnTimeout()