QThread: fix a race condition when destroying or restarting thread from finished()

Since we do not keep the mutex locked in QThreadPrivate::finish,
We could have races if the thread is destroyed or restarted from
another thread while we are still in that function

This solve tst_QCoreApplication::deliverInDefinedOrder on mac

Regression since a43583e0221311b7fe666726a

Reviewed-by: Brad

5 files changed, 68 insertions, 3 deletions

diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp
index 69b70cb4d2..71a4896027 100644
--- a/src/corelib/thread/qthread.cpp
+++ b/src/corelib/thread/qthread.cpp
@@ -173,7 +173,8 @@ void QAdoptedThread::run()
 */
 
 QThreadPrivate::QThreadPrivate(QThreadData *d)
-    : QObjectPrivate(), running(false), finished(false), terminated(false), exited(false), returnCode(-1),
+    : QObjectPrivate(), running(false), finished(false), terminated(false),
+      isInFinish(false), exited(false), returnCode(-1),
       stackSize(0), priority(QThread::InheritPriority), data(d)
 {
 #if defined (Q_OS_UNIX)
@@ -403,6 +404,11 @@ QThread::~QThread()
     Q_D(QThread);
     {
         QMutexLocker locker(&d->mutex);
+        if (d->isInFinish) {
+            locker.unlock();
+            wait();
+            locker.relock();
+        }
         if (d->running && !d->finished)
             qWarning("QThread: Destroyed while thread is still running");
 
diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h
index d816aefdc3..51c6991d0d 100644
--- a/src/corelib/thread/qthread_p.h
+++ b/src/corelib/thread/qthread_p.h
@@ -126,6 +126,7 @@ public:
     bool running;
     bool finished;
     bool terminated;
+    bool isInFinish; //when in QThreadPrivate::finish
 
     bool exited;
     int returnCode;
diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp
index 3a3c2bbd31..b674ab8b55 100644
--- a/src/corelib/thread/qthread_unix.cpp
+++ b/src/corelib/thread/qthread_unix.cpp
@@ -341,7 +341,7 @@ void QThreadPrivate::finish(void *arg)
     QMutexLocker locker(&d->mutex);
 #endif
 
-
+    d->isInFinish = true;
     d->priority = QThread::InheritPriority;
     bool terminated = d->terminated;
     void *data = &d->data->tls;
@@ -371,6 +371,7 @@ void QThreadPrivate::finish(void *arg)
     d->running = false;
     d->finished = true;
 
+    d->isInFinish = false;
     d->thread_done.wakeAll();
 }
 
@@ -548,6 +549,10 @@ void QThread::start(Priority priority)
 {
     Q_D(QThread);
     QMutexLocker locker(&d->mutex);
+
+    if (d->isInFinish)
+        d->thread_done.wait(locker.mutex());
+
     if (d->running)
         return;
 
diff --git a/src/corelib/thread/qthread_win.cpp b/src/corelib/thread/qthread_win.cpp
index 3706da8434..74fedd3880 100644
--- a/src/corelib/thread/qthread_win.cpp
+++ b/src/corelib/thread/qthread_win.cpp
@@ -324,6 +324,7 @@ void QThreadPrivate::finish(void *arg, bool lockAnyway)
     QThreadPrivate *d = thr->d_func();
 
     QMutexLocker locker(lockAnyway ? &d->mutex : 0);
+    d->isInFinish = true;
     d->priority = QThread::InheritPriority;
     bool terminated = d->terminated;
     void **tls_data = reinterpret_cast<void **>(&d->data->tls);
@@ -348,7 +349,7 @@ void QThreadPrivate::finish(void *arg, bool lockAnyway)
 
     d->running = false;
     d->finished = true;
-
+    d->isInFinish = false;
 
     if (!d->waiters) {
         CloseHandle(d->handle);
@@ -405,6 +406,12 @@ void QThread::start(Priority priority)
     Q_D(QThread);
     QMutexLocker locker(&d->mutex);
 
+    if (d->isInFinish) {
+        locker.unlock();
+        wait();
+        locker.relock();
+    }
+
     if (d->running)
         return;
 
diff --git a/tests/auto/qthread/tst_qthread.cpp b/tests/auto/qthread/tst_qthread.cpp
index 1e14b6a03a..01f080fcff 100644
--- a/tests/auto/qthread/tst_qthread.cpp
+++ b/tests/auto/qthread/tst_qthread.cpp
@@ -109,6 +109,8 @@ private slots:
     void connectThreadFinishedSignalToObjectDeleteLaterSlot();
     void wait2();
     void wait3_slowDestructor();
+    void destroyFinishRace();
+    void startFinishRace();
 
     void stressTest();
 };
@@ -1067,5 +1069,49 @@ void tst_QThread::wait3_slowDestructor()
     QVERIFY(thread.wait(one_minute));
 }
 
+void tst_QThread::destroyFinishRace()
+{
+    class Thread : public QThread { void run() {} };
+    for (int i = 0; i < 15; i++) {
+        Thread *thr = new Thread;
+        connect(thr, SIGNAL(finished()), thr, SLOT(deleteLater()));
+        QWeakPointer<QThread> weak(static_cast<QThread*>(thr));
+        thr->start();
+        while (weak) {
+            qApp->processEvents();
+            qApp->processEvents();
+            qApp->processEvents();
+            qApp->processEvents();
+        }
+    }
+}
+
+void tst_QThread::startFinishRace()
+{
+    class Thread : public QThread {
+    public:
+        Thread() : i (50) {}
+        void run() {
+            i--;
+            if (!i) disconnect(this, SIGNAL(finished()), 0, 0);
+        }
+        int i;
+    };
+    for (int i = 0; i < 15; i++) {
+        Thread thr;
+        connect(&thr, SIGNAL(finished()), &thr, SLOT(start()));
+        thr.start();
+        while (!thr.isFinished() || thr.i != 0) {
+            qApp->processEvents();
+            qApp->processEvents();
+            qApp->processEvents();
+            qApp->processEvents();
+        }
+        QCOMPARE(thr.i, 0);
+    }
+}
+
+
+
 QTEST_MAIN(tst_QThread)
 #include "tst_qthread.moc"