Use a per object lock for signal/slots

That way we prevent the current possible race condition that may appears
while touching signals slot while object are moved to different threads.

For exemple, when we do  sender->threadData->mutex->lock();  the sender
can possibly be moved to another thread between the moment we get the
pointer to the threadData and the moment we aquire the lock.  We could
check if we locked the right mutex and retry otherwise, but that would
break if the threadData (and hence the mutex) get destroyed in between.

The per object mutex is implemented with a thread pool.
I'm not using the global QThreadPool because we might ends up locking
two of their mutex, and that would be dangerous if something else holds
a lock on the same mutex (possible deadlock)

Putting the mutex pool in a Q_GLOBAL_STATIC doesn't work as it might
result of the ppol being deleted before some other object in others
Q_GLOBAL_STATIC structures

There is no need to lock this mutex in moveToThread as this is safe.
When emiting a signal, we do not need to lock the thread data, as the
user must ensure that the object is not moved to a thread while emiting
a AutoConnection signal.

Reviewed-by: Brad

2 files changed, 65 insertions, 25 deletions

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 9f803fa..cfd8493 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -58,6 +58,7 @@
 #include <qsemaphore.h>
 
 #include <private/qorderedmutexlocker_p.h>
+#include <private/qmutexpool_p.h>
 
 #include <new>
 
@@ -91,12 +92,35 @@ static int *queuedConnectionTypes(const QList<QByteArray> &typeNames)
     return types;
 }
 
+QBasicAtomicPointer<QMutexPool> signalSlotMutexes = Q_BASIC_ATOMIC_INITIALIZER(0);
+QBasicAtomicInt objectCount = Q_BASIC_ATOMIC_INITIALIZER(0);
+
+/** \internal
+ * mutex to be locked when accessing the connectionlists or the senders list
+ */
+static QMutex *signalSlotLock(const QObject *o)
+{
+    if (!signalSlotMutexes) {
+        QMutexPool *mp = new QMutexPool;
+        if (!signalSlotMutexes.testAndSetOrdered(0, mp)) {
+            delete mp;
+        }
+    }
+    return signalSlotMutexes->get(o);
+}
+
 extern "C" Q_CORE_EXPORT void qt_addObject(QObject *)
 {
+    objectCount.ref();
 }
 
 extern "C" Q_CORE_EXPORT void qt_removeObject(QObject *)
 {
+    if(!objectCount.deref()) {
+        QMutexPool *old = signalSlotMutexes;
+        signalSlotMutexes.testAndSetAcquire(old, 0);
+        delete old;
+    }
 }
 
 QObjectPrivate::QObjectPrivate(int version)
@@ -223,7 +247,7 @@ bool QObjectPrivate::isSender(const QObject *receiver, const char *signal) const
     int signal_index = q->metaObject()->indexOfSignal(signal);
     if (signal_index < 0)
         return false;
-    QMutexLocker locker(&threadData->mutex);
+    QMutexLocker locker(signalSlotLock(q));
     if (connectionLists) {
         if (signal_index < connectionLists->count()) {
             const ConnectionList &connectionList = connectionLists->at(signal_index);
@@ -245,7 +269,7 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const
     int signal_index = q->metaObject()->indexOfSignal(signal);
     if (signal_index < 0)
         return returnValue;
-    QMutexLocker locker(&threadData->mutex);
+    QMutexLocker locker(signalSlotLock(q));
     if (connectionLists) {
         if (signal_index < connectionLists->count()) {
             const ConnectionList &connectionList = connectionLists->at(signal_index);
@@ -263,7 +287,7 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const
 QObjectList QObjectPrivate::senderList() const
 {
     QObjectList returnValue;
-    QMutexLocker locker(&threadData->mutex);
+    QMutexLocker locker(signalSlotLock(q_func()));
     for (int i = 0; i < senders.count(); ++i)
         returnValue << senders.at(i)->sender;
     return returnValue;
@@ -712,7 +736,7 @@ QObject::~QObject()
     emit destroyed(this);
 
     {
-        QMutexLocker locker(&d->threadData->mutex);
+        QMutexLocker locker(signalSlotLock(this));
 
         // set ref to zero to indicate that this object has been deleted
         if (d->currentSender != 0)
@@ -731,7 +755,7 @@ QObject::~QObject()
                         continue;
                     }
 
-                    QMutex *m = &c->receiver->d_func()->threadData->mutex;
+                    QMutex *m = signalSlotLock(c->receiver);
                     bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m);
                     c = connectionList[i];
                     if (c->receiver)
@@ -755,7 +779,7 @@ QObject::~QObject()
         for (int i = 0; i < d->senders.count(); ) {
             QObjectPrivate::Connection *s = d->senders[i];
 
-            QMutex *m = &s->sender->d_func()->threadData->mutex;
+            QMutex *m = signalSlotLock(s->sender);
             bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m);
             if (m < locker.mutex()) {
                 if (i >= d->senders.count() || s != d->senders[i]) {
@@ -765,11 +789,9 @@ QObject::~QObject()
                 }
             }
             s->receiver = 0;
-            if (s->sender) {
-                QObjectConnectionListVector *senderLists = s->sender->d_func()->connectionLists;
-                if (senderLists)
-                    senderLists->dirty = true;
-            }
+            QObjectConnectionListVector *senderLists = s->sender->d_func()->connectionLists;
+            if (senderLists)
+                senderLists->dirty = true;
 
             if (needToUnlock)
                 m->unlock();
@@ -2254,7 +2276,7 @@ QObject *QObject::sender() const
 {
     Q_D(const QObject);
 
-    QMutexLocker(&d->threadData->mutex);
+    QMutexLocker(signalSlotLock(this));
     if (!d->currentSender)
         return 0;
 
@@ -2310,7 +2332,7 @@ int QObject::receivers(const char *signal) const
         }
 
         Q_D(const QObject);
-        QMutexLocker locker(&d->threadData->mutex);
+        QMutexLocker locker(signalSlotLock(this));
         if (d->connectionLists) {
             if (signal_index < d->connectionLists->count()) {
                 const QObjectPrivate::ConnectionList &connectionList =
@@ -2757,8 +2779,8 @@ bool QMetaObject::connect(const QObject *sender, int signal_index,
     c->connectionType = type;
     c->argumentTypes = types;
 
-    QOrderedMutexLocker locker(&s->d_func()->threadData->mutex,
-                               &r->d_func()->threadData->mutex);
+    QOrderedMutexLocker locker(signalSlotLock(sender),
+                               signalSlotLock(receiver));
 
     s->d_func()->addConnection(signal_index, c);
     r->d_func()->senders.append(c);
@@ -2783,8 +2805,8 @@ bool QMetaObject::disconnect(const QObject *sender, int signal_index,
     QObject *s = const_cast<QObject *>(sender);
     QObject *r = const_cast<QObject *>(receiver);
 
-    QMutex *senderMutex = &s->d_func()->threadData->mutex;
-    QMutex *receiverMutex = r ? &r->d_func()->threadData->mutex : 0;
+    QMutex *senderMutex = signalSlotLock(sender);
+    QMutex *receiverMutex = receiver ? signalSlotLock(receiver) : 0;
     QOrderedMutexLocker locker(senderMutex, receiverMutex);
 
     QObjectConnectionListVector *connectionLists = s->d_func()->connectionLists;
@@ -2804,7 +2826,7 @@ bool QMetaObject::disconnect(const QObject *sender, int signal_index,
                 if (c->receiver
                     && (r == 0 || (c->receiver == r
                                    && (method_index < 0 || c->method == method_index)))) {
-                    QMutex *m = &c->receiver->d_func()->threadData->mutex;
+                    QMutex *m = signalSlotLock(c->receiver);
                     bool needToUnlock = false;
                     if (!receiverMutex && senderMutex != m) {
                         // need to relock this receiver and sender in the correct order
@@ -2831,7 +2853,7 @@ bool QMetaObject::disconnect(const QObject *sender, int signal_index,
             if (c->receiver
                 && (r == 0 || (c->receiver == r
                                && (method_index < 0 || c->method == method_index)))) {
-                QMutex *m = &c->receiver->d_func()->threadData->mutex;
+                QMutex *m = signalSlotLock(c->receiver);
                 bool needToUnlock = false;
                 if (!receiverMutex && senderMutex != m) {
                     // need to relock this receiver and sender in the correct order
@@ -2972,7 +2994,7 @@ static void blocking_activate(QObject *sender, int signal, QObjectPrivate::Conne
 #else
     QSemaphore semaphore;
     queued_activate(sender, signal, c, argv, &semaphore);
-    QMutex *mutex = &QThreadData::get2(sender->thread())->mutex;
+    QMutex *mutex = signalSlotLock(sender);
     mutex->unlock();
     semaphore.acquire();
     mutex->lock();
@@ -2992,7 +3014,7 @@ void QMetaObject::activate(QObject *sender, int from_signal_index, int to_signal
                                                          argv ? argv : empty_argv);
     }
 
-    QMutexLocker locker(&sender->d_func()->threadData->mutex);
+    QMutexLocker locker(signalSlotLock(sender));
     QThreadData *currentThreadData = QThreadData::current();
 
     QObjectConnectionListVector *connectionLists = sender->d_func()->connectionLists;
@@ -3344,7 +3366,7 @@ void QObject::dumpObjectInfo()
            objectName().isEmpty() ? "unnamed" : objectName().toLocal8Bit().data());
 
     Q_D(QObject);
-    QMutexLocker locker(&d->threadData->mutex);
+    QMutexLocker locker(signalSlotLock(this));
 
     // first, look for connections where this object is the sender
     qDebug("  SIGNALS OUT");
diff --git a/tests/auto/qobjectrace/tst_qobjectrace.cpp b/tests/auto/qobjectrace/tst_qobjectrace.cpp
index fcfd528..aa80534 100644
--- a/tests/auto/qobjectrace/tst_qobjectrace.cpp
+++ b/tests/auto/qobjectrace/tst_qobjectrace.cpp
@@ -43,6 +43,7 @@
 #include <QtCore>
 #include <QtTest/QtTest>
 
+
 enum { OneMinute = 60 * 1000, TwoMinutes = OneMinute * 2 };
 
 class tst_QObjectRace: public QObject
@@ -70,12 +71,18 @@ public:
 public slots:
     void theSlot()
     {
-        enum { step = 1000 };
+        enum { step = 35 };
         if ((++count % step) == 0) {
             QThread *nextThread = threads.at((count / step) % threads.size());
             moveToThread(nextThread);
         }
     }
+
+    void destroSlot() {
+        emit theSignal();
+    }
+signals:
+    void theSignal();
 };
 
 class RaceThread : public QThread
@@ -120,6 +127,10 @@ private slots:
         if (stopWatch.elapsed() >= OneMinute / 2)
 #endif
             quit();
+
+        QObject o;
+        connect(&o, SIGNAL(destroyed()) , object, SLOT(destroSlot()));
+        connect(object, SIGNAL(destroyed()) , &o, SLOT(deleteLater()));
     }
 };
 
@@ -138,10 +149,17 @@ void tst_QObjectRace::moveToThreadRace()
 
     for (int i = 0; i < ThreadCount; ++i)
         threads[i]->start();
-    QVERIFY(threads[0]->wait(TwoMinutes));
+
+    while(!threads[0]->isFinished()) {
+        QPointer<RaceObject> foo (object);
+        QObject o;
+        connect(&o, SIGNAL(destroyed()) , object, SLOT(destroSlot()));
+        connect(object, SIGNAL(destroyed()) , &o, SLOT(deleteLater()));
+        QTest::qWait(10);
+    }
     // the other threads should finish pretty quickly now
     for (int i = 1; i < ThreadCount; ++i)
-        QVERIFY(threads[i]->wait(30000));
+        QVERIFY(threads[i]->wait(300));
 
     for (int i = 0; i < ThreadCount; ++i)
         delete threads[i];