QThreadPrivate::finish should not keep mutex locked when calling signals

This fix the deadlock shown in the new test wait3_slowDestructor

Add a test for QThread::wait(timeout)

Task-number: QTBUG-15030
Reviewed-by: Joao
Reviewed-by: Brad

3 files changed, 114 insertions, 28 deletions

diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp
index f508c0a..f409256 100644
--- a/src/corelib/thread/qthread_unix.cpp
+++ b/src/corelib/thread/qthread_unix.cpp
@@ -333,40 +333,44 @@ void QThreadPrivate::finish(void *arg)
 {
     QThread *thr = reinterpret_cast<QThread *>(arg);
     QThreadPrivate *d = thr->d_func();
+
 #ifdef Q_OS_SYMBIAN
-    if (lockAnyway)
+    QMutexLocker locker(lockAnyway ? &d->mutex : 0);
+#else
+    QMutexLocker locker(&d->mutex);
 #endif
-        d->mutex.lock();
+
 
     d->priority = QThread::InheritPriority;
-    d->running = false;
-    d->finished = true;
-    if (d->terminated)
+    bool terminated = d->terminated;
+    void *data = &d->data->tls;
+    locker.unlock();
+    if (terminated)
         emit thr->terminated();
-    d->terminated = false;
     emit thr->finished();
     QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
+    QThreadStorageData::finish((void **)data);
+    locker.relock();
+    d->terminated = false;
 
-    if (d->data->eventDispatcher) {
-        d->data->eventDispatcher->closingDown();
-        QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher;
+    QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher;
+    if (eventDispatcher) {
         d->data->eventDispatcher = 0;
+        locker.unlock();
+        eventDispatcher->closingDown();
         delete eventDispatcher;
+        locker.relock();
     }
 
-    void *data = &d->data->tls;
-    QThreadStorageData::finish((void **)data);
-
     d->thread_id = 0;
 #ifdef Q_OS_SYMBIAN
     if (closeNativeHandle)
         d->data->symbian_thread_handle.Close();
 #endif
+    d->running = false;
+    d->finished = true;
+
     d->thread_done.wakeAll();
-#ifdef Q_OS_SYMBIAN
-    if (lockAnyway)
-#endif
-        d->mutex.unlock();
 }
 
 
diff --git a/src/corelib/thread/qthread_win.cpp b/src/corelib/thread/qthread_win.cpp
index 4a967ed..3706da8 100644
--- a/src/corelib/thread/qthread_win.cpp
+++ b/src/corelib/thread/qthread_win.cpp
@@ -323,25 +323,32 @@ void QThreadPrivate::finish(void *arg, bool lockAnyway)
     QThread *thr = reinterpret_cast<QThread *>(arg);
     QThreadPrivate *d = thr->d_func();
 
-    if (lockAnyway)
-        d->mutex.lock();
+    QMutexLocker locker(lockAnyway ? &d->mutex : 0);
     d->priority = QThread::InheritPriority;
-    d->running = false;
-    d->finished = true;
-    if (d->terminated)
+    bool terminated = d->terminated;
+    void **tls_data = reinterpret_cast<void **>(&d->data->tls);
+    locker.unlock();
+    if (terminated)
         emit thr->terminated();
-    d->terminated = false;
     emit thr->finished();
     QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
+    QThreadStorageData::finish(tls_data);
+    locker.relock();
+
+    d->terminated = false;
 
-    if (d->data->eventDispatcher) {
-        d->data->eventDispatcher->closingDown();
-        QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher;
+    QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher;
+    if (eventDispatcher) {
         d->data->eventDispatcher = 0;
+        locker.unlock();
+        eventDispatcher->closingDown();
         delete eventDispatcher;
+        locker.relock();
     }
 
-    QThreadStorageData::finish(reinterpret_cast<void **>(&d->data->tls));
+    d->running = false;
+    d->finished = true;
+
 
     if (!d->waiters) {
         CloseHandle(d->handle);
@@ -350,8 +357,6 @@ void QThreadPrivate::finish(void *arg, bool lockAnyway)
 
     d->id = 0;
 
-    if (lockAnyway)
-        d->mutex.unlock();
 }
 
 /**************************************************************************
diff --git a/tests/auto/qthread/tst_qthread.cpp b/tests/auto/qthread/tst_qthread.cpp
index f290a2b..49c3576 100644
--- a/tests/auto/qthread/tst_qthread.cpp
+++ b/tests/auto/qthread/tst_qthread.cpp
@@ -107,6 +107,8 @@ private slots:
 
     void QTBUG13810_exitAndStart();
     void connectThreadFinishedSignalToObjectDeleteLaterSlot();
+    void wait2();
+    void wait3_slowDestructor();
 
     void stressTest();
 };
@@ -976,6 +978,7 @@ void tst_QThread::QTBUG13810_exitAndStart()
     QCOMPARE(sync1.m_prop, 89);
 }
 
+
 void tst_QThread::connectThreadFinishedSignalToObjectDeleteLaterSlot()
 {
     QThread thread;
@@ -990,5 +993,79 @@ void tst_QThread::connectThreadFinishedSignalToObjectDeleteLaterSlot()
     QVERIFY(p.isNull());
 }
 
+class Waiting_Thread : public QThread
+{
+public:
+    enum { WaitTime = 800 };
+    QMutex mutex;
+    QWaitCondition cond1;
+    QWaitCondition cond2;
+
+    void run()
+    {
+        QMutexLocker locker(&mutex);
+        cond1.wait(&mutex);
+        cond2.wait(&mutex, WaitTime);
+    }
+};
+
+void tst_QThread::wait2()
+{
+    QElapsedTimer timer;
+    Waiting_Thread thread;
+    thread.start();
+    timer.start();
+    QVERIFY(!thread.wait(Waiting_Thread::WaitTime));
+    qint64 elapsed = timer.elapsed();
+
+    QVERIFY(elapsed >= Waiting_Thread::WaitTime);
+    //QVERIFY(elapsed < Waiting_Thread::WaitTime * 1.4);
+
+    timer.start();
+    thread.cond1.wakeOne();
+    QVERIFY(thread.wait(/*Waiting_Thread::WaitTime * 1.4*/));
+    elapsed = timer.elapsed();
+    QVERIFY(elapsed >= Waiting_Thread::WaitTime);
+    //QVERIFY(elapsed < Waiting_Thread::WaitTime * 1.4);
+}
+
+
+class SlowSlotObject : public QObject {
+    Q_OBJECT
+public:
+    QMutex mutex;
+    QWaitCondition cond;
+public slots:
+    void slowSlot() {
+        QMutexLocker locker(&mutex);
+        cond.wait(&mutex);
+    }
+};
+
+void tst_QThread::wait3_slowDestructor()
+{
+    SlowSlotObject slow;
+    QThread thread;
+    QObject::connect(&thread, SIGNAL(finished()), &slow, SLOT(slowSlot()), Qt::DirectConnection);
+
+    enum { WaitTime = 1800 };
+    QElapsedTimer timer;
+
+    thread.start();
+    thread.quit();
+    //the quit function will cause the thread to finish and enter the slowSlot that is blocking
+
+    timer.start();
+    QVERIFY(!thread.wait(Waiting_Thread::WaitTime));
+    qint64 elapsed = timer.elapsed();
+
+    QVERIFY(elapsed >= Waiting_Thread::WaitTime);
+    //QVERIFY(elapsed < Waiting_Thread::WaitTime * 1.4);
+
+    slow.cond.wakeOne();
+    //now the thread shoud finish quickly
+    QVERIFY(thread.wait(one_minute));
+}
+
 QTEST_MAIN(tst_QThread)
 #include "tst_qthread.moc"