QTimer: optimize single shot timers that cross thread

Amend 4d90c4e74a by clarifying why
moving the QSingleShotObject to the receiver's thread is a good
idea. Move that logic into a separate function and use that also
for the string-based connection.

Optimize the implementation by delaying the timer creation until
after we have moved the QSingleShotTimer object to the target
thread, using a queued metacall if needed to create the timer.
This avoids the creation of the timer in the wrong thread and
then the recreation of the timer in the new thread when QObject
handles QEvent::ThreadChange.

To avoid that the timer is created when it has already expired in
real time, use a deadline timer and fire timeout immediately when
it has expired by the time we get the metacall.

Since the timerId might now not be initialized in the constructor,
initialize it to -1.

Augment the crossThreadSingleShotToFunctor test function by
deliberately not starting the thread until the deadline expired.

[ChangeLog][Core][QTimer] Single-shot timers with a string-based
connection are now started in the receiver's thread, not in the
thread that creates the timer.

Task-number: QTBUG-112162
Change-Id: I3fc94c34c21d9f644da41a2e4c2da479980b8580
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

src/corelib/kernel/qtimer.cpp

@@ -8,6 +8,7 @@
 #include "qabstracteventdispatcher.h"
 #include "qcoreapplication.h"
 #include "qcoreapplication_p.h"
+#include "qdeadlinetimer.h"
 #include "qmetaobject_p.h"
 #include "qobject_p.h"
 #include "qproperty_p.h"
@@ -249,12 +250,14 @@ void QTimer::timerEvent(QTimerEvent *e)
 class QSingleShotTimer : public QObject
 {
     Q_OBJECT
-    int timerId;
+    int timerId = -1;
 public:
     ~QSingleShotTimer();
     QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char * m);
     QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, QtPrivate::QSlotObjectBase *slotObj);
 
+    void startTimerForReceiver(int msec, Qt::TimerType timerType, const QObject *receiver);
+
 Q_SIGNALS:
     void timeout();
 protected:
@@ -264,27 +267,20 @@ protected:
 QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char *member)
     : QObject(QAbstractEventDispatcher::instance())
 {
-    timerId = startTimer(std::chrono::milliseconds{msec}, timerType);
     connect(this, SIGNAL(timeout()), r, member);
+
+    startTimerForReceiver(msec, timerType, r);
 }
 
 QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, QtPrivate::QSlotObjectBase *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);
-        setParent(nullptr);
-        moveToThread(r->thread());
-    }
+    startTimerForReceiver(msec, timerType, r);
 }
 
 QSingleShotTimer::~QSingleShotTimer()
@@ -293,6 +289,32 @@ QSingleShotTimer::~QSingleShotTimer()
         killTimer(timerId);
 }
 
+/*
+    Move the timer, and the dispatching and handling of the timer event, into
+    the same thread as where it will be handled, so that it fires reliably even
+    if the thread that set up the timer is busy.
+*/
+void QSingleShotTimer::startTimerForReceiver(int msec, Qt::TimerType timerType, const QObject *receiver)
+{
+    if (receiver && receiver->thread() != thread()) {
+        // Avoid leaking the QSingleShotTimer instance in case the application exits before the timer fires
+        connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);
+        setParent(nullptr);
+        moveToThread(receiver->thread());
+
+        QDeadlineTimer deadline(std::chrono::milliseconds{msec}, timerType);
+        QMetaObject::invokeMethod(this, [=]{
+            if (deadline.hasExpired())
+                emit timeout();
+            else
+                timerId = startTimer(std::chrono::milliseconds{deadline.remainingTime()}, timerType);
+        }, Qt::QueuedConnection);
+    } else {
+        timerId = startTimer(std::chrono::milliseconds{msec}, timerType);
+    }
+}
+
+
 void QSingleShotTimer::timerEvent(QTimerEvent *)
 {
     // need to kill the timer _before_ we emit timeout() in case the

tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp

@@ -1031,11 +1031,22 @@ void tst_QTimer::crossThreadSingleShotToFunctor()
     DummyFunctor::callThread = nullptr;
 
     QThread t;
-    t.start();
-
     std::unique_ptr<QObject> o(new QObject());
     o->moveToThread(&t);
 
+    QTimer::singleShot(timeout, o.get(), DummyFunctor());
+
+    // wait enough time for the timer to have timed out before the timer
+    // could be start in the receiver's thread.
+    QTest::qWait(10 + timeout * 10);
+    t.start();
+    t.wait();
+    QCOMPARE(DummyFunctor::callThread, &t);
+
+    // continue with a stress test - the calling thread is busy, the
+    // timer should still fire and no crashes.
+    DummyFunctor::callThread = nullptr;
+    t.start();
     for (int i = 0; i < 10000; i++)
         QTimer::singleShot(timeout, o.get(), DummyFunctor());