diff --git a/src/corelib/kernel/qsingleshottimer.cpp b/src/corelib/kernel/qsingleshottimer.cpp index 9ec3ced8ac5..8ae04993ad2 100644 --- a/src/corelib/kernel/qsingleshottimer.cpp +++ b/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 - // timer fires - connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, - &QObject::deleteLater); + QObjectPrivate *d_ptr = QObjectPrivate::get(this); + d_ptr->sendChildEvents = false; + setParent(nullptr); moveToThread(receiver->thread()); - QDeadlineTimer deadline(interval, timerType); - auto invokable = [this, deadline, timerType] { - if (deadline.hasExpired()) { - Q_EMIT timeout(); - } else { - timer.start(deadline.remainingTimeAsDuration(), timerType, this); - } - }; - QMetaObject::invokeMethod(this, invokable, Qt::QueuedConnection); + QCoreApplication::postEvent(this, + new StartTimerEvent(this, QDeadlineTimer(interval, timerType))); + // the event owns "this" and is handled concurrently, so unsafe to + // access "this" beyond this point } 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(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" diff --git a/src/corelib/kernel/qsingleshottimer_p.h b/src/corelib/kernel/qsingleshottimer_p.h index e7fc70975eb..bdc02ab0b85 100644 --- a/src/corelib/kernel/qsingleshottimer_p.h +++ b/src/corelib/kernel/qsingleshottimer_p.h @@ -19,6 +19,8 @@ #include #include #include +#include +#include #include 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 timer; + QDeadlineTimer deadline; + }; + + void timerFinished(); void timerEvent(QTimerEvent *) override; }; diff --git a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp index e2485e8fd3e..91415c3dbbf 100644 --- a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp +++ b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -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("timeout"); + QTest::addColumn("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 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 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;