Fixed a QTimer::singleShot() crash when a functor callback is used

If QTimer::singleShot() is used with a functor callback and a context
object with different thread affinity than the caller, a crash can
occur. If the context object's thread is scheduled before
connecting to QCoreApplication::aboutToQuit(), the timer has a change
to fire and QSingleShotTimer::timerEvent() will delete the
QSingleShotTimer object making the this pointer used in the
connection invalid. This can occur relatively often if an interval
of 0 is used.

Making the moveToThread() call the last thing in the constructor
ensures that the constructor gets to run to completion before the
timer has a chance to fire.

Task-number: QTBUG-48700
Change-Id: Iab73d02933635821b8d1ca1ff3d53e92eca85834
Reviewed-by: Olivier Goffart (Woboq GmbH) <ogoffart@woboq.com>

2 files changed, 26 insertions, 7 deletions

diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp
index b9109a96aa..af9a1be6ab 100644
--- a/src/corelib/kernel/qtimer.cpp
+++ b/src/corelib/kernel/qtimer.cpp
@@ -278,15 +278,10 @@ QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObj
 {
     timerId = startTimer(msec, timerType);
     if (r && thread() != r->thread()) {
-        // We need the invocation to happen in the receiver object's thread.
-        // So, move QSingleShotTimer to the correct thread. Before that occurs, we
-        // shall remove the parent from the object.
+        // Avoid leaking the QSingleShotTimer instance in case the application exits before the timer fires
+        connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);
         setParent(0);
         moveToThread(r->thread());
-
-        // Given we're also parentless now, we should take defence against leaks
-        // in case the application quits before we expire.
-        connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);
     }
 }
 
diff --git a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
index 1dc358bd97..b34a3a6beb 100644
--- a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
+++ b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
@@ -72,6 +72,7 @@ private slots:
     void singleShotStaticFunctionZeroTimeout();
     void recurseOnTimeoutAndStopTimer();
     void singleShotToFunctors();
+    void crossThreadSingleShotToFunctor();
 
     void dontBlockEvents();
     void postedEventsShouldNotStarveTimers();
@@ -877,5 +878,28 @@ void tst_QTimer::postedEventsShouldNotStarveTimers()
     QVERIFY(timerHelper.count > 5);
 }
 
+struct DummyFunctor {
+    void operator()() {}
+};
+
+void tst_QTimer::crossThreadSingleShotToFunctor()
+{
+    // We're testing for crashes here, so the test simply running to
+    // completion is considered a success
+    QThread t;
+    t.start();
+
+    QObject* o = new QObject();
+    o->moveToThread(&t);
+
+    for (int i = 0; i < 10000; i++) {
+        QTimer::singleShot(0, o, DummyFunctor());
+    }
+
+    t.quit();
+    t.wait();
+    delete o;
+}
+
 QTEST_MAIN(tst_QTimer)
 #include "tst_qtimer.moc"