QTimer: delete pending single shot timers when receiver's thread dies

QSingleShotTimer instances are usually destroyed when they receive their
QTimerEvent, and otherwise are children of the creating thread's event
dispatcher so that they do get destroyed even if they never fired.

However, if the receiver object lives in a different thread than the
thread that started the timer, then we move the QSST instance to
that thread to make sure that it starts in the right thread. We then
reparented the QSST instance to nullptr (it can't continue to be a
child of the creating thread's event dispatcher), and connected to
QApp::aboutToQuit as a fall-back in case the timer never fires.

This has two problems: if the timer never fires (e.g. because the
receiver's thread stopped before the timeout), then we created a
soft leak (until aboutToQuit). And since the QSST instance was
moved to the receiver's thread, the connection to aboutToQuit()
is always queued, and also never got processed if the thread was
stopped before (especially since we connected to deleteLater(),
which would require another event processing cycle). So in
practice, we ended up with a hard leak.

To fix this, we have to reparent the QSST instance to the event
dispatcher of the receiver's thread. We can do that reliably
once the receiver thread processes events. Simplify the code to
replace a meta-call with a posted event to ourselves, receiving
which starts the timer (or immediately fires timeout if it has
taken long enough to get there). To avoid memory leaks in the
unlikely case that this posted event never gets delivered (e.g.
because the thread is shutting down already), give that event
ownership of the QSST instance until the timer is started.

This turns out to be leaking anyway on most platforms, which
requires further investigation.

To be able to reparent safely away from a parent in a different
thread, clear the sendChildEvents flag first.

Amends 87535e4e4337596066258e361143cb9906e89512.

Fixes: QTBUG-135636
Pick-to: 6.8
Change-Id: I8188160d54cfb63cb1765c5de8a6c0728dabb7e5
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 96ef0004111b47cec239846b169942bbc885c181)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/corelib/kernel/qsingleshottimer.cpp

@@ -7,6 +7,7 @@
 #include "qcoreapplication.h"
 #include "qmetaobject_p.h"
 #include "private/qnumeric_p.h"
+#include "qthread.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -41,41 +42,49 @@ void QSingleShotTimer::startTimerForReceiver(Duration interval, Qt::TimerType ti
                                              const QObject *receiver)
 {
     if (receiver && receiver->thread() != thread()) {
-        // Avoid leaking the QSingleShotTimer instance in case the application exits before the
+        QObjectPrivate *d_ptr = QObjectPrivate::get(this);
-        // timer fires
+        d_ptr->sendChildEvents = false;
-        connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this,
+
-                &QObject::deleteLater);
         setParent(nullptr);
         moveToThread(receiver->thread());
 
-        QDeadlineTimer deadline(interval, timerType);
+        QCoreApplication::postEvent(this,
-        auto invokable = [this, deadline, timerType] {
+                                    new StartTimerEvent(this, QDeadlineTimer(interval, timerType)));
-            if (deadline.hasExpired()) {
+        // the event owns "this" and is handled concurrently, so unsafe to
-                Q_EMIT timeout();
+        // access "this" beyond this point
-            } else {
-                timer.start(deadline.remainingTimeAsDuration(), timerType, this);
-            }
-        };
-        QMetaObject::invokeMethod(this, invokable, Qt::QueuedConnection);
     } else {
         timer.start(interval, timerType, this);
     }
 }
 
-void QSingleShotTimer::timerEvent(QTimerEvent *)
+void QSingleShotTimer::timerFinished()
 {
-    // need to kill the timer _before_ we emit timeout() in case the
-    // slot connected to timeout calls processEvents()
-    timer.stop();
-
     Q_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
-    // and explicitly delete...
     delete this;
 }
 
+void QSingleShotTimer::timerEvent(QTimerEvent *event)
+{
+    if (event->id() == Qt::TimerId::Invalid) {
+        StartTimerEvent *startTimerEvent = static_cast<StartTimerEvent *>(event);
+        startTimerEvent->timer.release();
+        const QDeadlineTimer &deadline = startTimerEvent->deadline;
+        if (deadline.hasExpired()) {
+            timerFinished();
+        } else {
+            timer.start(deadline.remainingTimeAsDuration(), deadline.timerType(), this);
+            // we are now definitely in a thread that has an event dispatcher
+            setParent(QThread::currentThread()->eventDispatcher());
+        }
+    } else {
+        // need to kill the timer _before_ we emit timeout() in case the
+        // slot connected to timeout calls processEvents()
+        timer.stop();
+
+        timerFinished();
+    }
+}
+
 QT_END_NAMESPACE
 
 #include "moc_qsingleshottimer_p.cpp"

src/corelib/kernel/qsingleshottimer_p.h

@@ -19,6 +19,8 @@
 #include <QtCore/qobject.h>
 #include <QtCore/qabstracteventdispatcher.h>
 #include <QtCore/qbasictimer.h>
+#include <QtCore/qcoreevent.h>
+#include <QtCore/qdeadlinetimer.h>
 #include <QtCore/qnamespace.h>
 
 QT_BEGIN_NAMESPACE
@@ -27,7 +29,7 @@ namespace QtPrivate {
 class QSlotObjectBase;
 }
 
-class QSingleShotTimer : public QObject
+class Q_AUTOTEST_EXPORT QSingleShotTimer : public QObject
 {
     Q_OBJECT
 
@@ -49,6 +51,18 @@ Q_SIGNALS:
     void timeout();
 
 private:
+    class StartTimerEvent : public QTimerEvent
+    {
+    public:
+        StartTimerEvent(QSingleShotTimer *timer, const QDeadlineTimer &deadline)
+            : QTimerEvent(Qt::TimerId::Invalid), timer(timer), deadline(deadline)
+        {}
+
+        std::unique_ptr<QSingleShotTimer> timer;
+        QDeadlineTimer deadline;
+    };
+
+    void timerFinished();
     void timerEvent(QTimerEvent *) override;
 };
 

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

@@ -23,6 +23,7 @@
 #include <QSignalSpy>
 #include <QtTest/private/qpropertytesthelper_p.h>
 
+#include <QtCore/private/qsingleshottimer_p.h>
 #include <qtimer.h>
 #include <qthread.h>
 #include <qelapsedtimer.h>
@@ -84,6 +85,10 @@ private slots:
     void singleShot_static();
     void crossThreadSingleShotToFunctor_data();
     void crossThreadSingleShotToFunctor();
+#ifdef QT_BUILD_INTERNAL
+    void crossThreadSingleShotDestruction_data();
+    void crossThreadSingleShotDestruction();
+#endif
     void timerOrder();
     void timerOrder_data();
     void timerOrderBackgroundThread();
@@ -1236,6 +1241,94 @@ void tst_QTimer::crossThreadSingleShotToFunctor()
     QCOMPARE(DummyFunctor::callThread, &t);
 }
 
+#ifdef QT_BUILD_INTERNAL
+class SingleShotReceiver : public QObject
+{
+    Q_OBJECT
+public:
+    SingleShotReceiver() = default;
+
+    bool timeout = false;
+
+public slots:
+    void timerFired() {
+        timeout = true;
+        thread()->quit();
+    }
+};
+
+void tst_QTimer::crossThreadSingleShotDestruction_data()
+{
+    QTest::addColumn<std::chrono::milliseconds>("timeout");
+    QTest::addColumn<std::chrono::milliseconds>("threadWaitTime");
+
+    // No point in testing zero timeouts, we don't create a QSingleShotTimer
+    // for those anyway.
+    QTest::addRow("1ms") << 1ms << 10ms;
+    QTest::addRow("1s") << 1000ms << 10ms;
+}
+
+void tst_QTimer::crossThreadSingleShotDestruction()
+{
+    QFETCH(std::chrono::milliseconds, timeout);
+    QFETCH(std::chrono::milliseconds, threadWaitTime);
+
+    static bool deadTimerDestroyed;
+    class SingleShotTimer : public QSingleShotTimer
+    {
+    public:
+        using QSingleShotTimer::QSingleShotTimer;
+        ~SingleShotTimer() override { deadTimerDestroyed = true; }
+    };
+
+    {
+        QThread t;
+        std::unique_ptr<SingleShotReceiver> receiver(new SingleShotReceiver());
+        receiver->moveToThread(&t);
+
+        // create the timer before the thread is running. Otherwise, the
+        // assignment to QPointer might happen after the timer already fired and
+        // destroyed itself, leaving QPointer dangling.
+        QPointer<QSingleShotTimer> timer = new QSingleShotTimer(timeout, Qt::CoarseTimer,
+                                                                receiver.get(), SLOT(timerFired()));
+        t.start();
+
+        const bool threadQuit = t.wait(threadWaitTime);
+
+        // If we waited long enough, then the timer should have fired, quit
+        // the thread, and self-destructed; otherwise the wait will have timed
+        // out and the timer is still alive.
+        QCOMPARE(threadQuit, receiver->timeout);
+        QCOMPARE(threadQuit, !timer);
+        deadTimerDestroyed = threadQuit;
+
+        if (!threadQuit) {
+            t.quit();
+
+            // now that the thread is definitely dead, start a new timer that is
+            // never even going to get started. It should still be destroyed, as
+            // it will be (temporarily) owned by the event that we post to start
+            // the timer in the target thread.
+            // Note: we can't use QPointer here, as the thread is already
+            // running.
+            (void)new SingleShotTimer(timeout, Qt::CoarseTimer, receiver.get(), SLOT(timerFired()));
+            QVERIFY(t.wait());
+            // the timer should never have fired
+            QVERIFY(!receiver->timeout);
+        }
+
+        // the timer objects should be destroyed by now in all cases
+        QVERIFY(!timer);
+    }
+
+# if defined(Q_OS_DARWIN) || defined(Q_OS_WIN) || defined(Q_OS_QNX) || defined(DISABLE_GLIB)|| defined(QT_NO_GLIB) || defined(QT_GUI_LIB)
+    QEXPECT_FAIL("1s", "Events posted to a thread after event loop exit are leaking.",
+                 Continue);
+# endif
+    QVERIFY(deadTimerDestroyed);
+}
+#endif
+
 void tst_QTimer::callOnTimeout()
 {
     QTimer timer;