Fix race condition in ~QObject

while using QOrderedMutexLocker::relock we might unlock our mutex
protecting the 'senders' list for a short moment. Another thread may then
modify or remove element on the list.
Therefore, we need to recheck the consistency of the list once we did
that.

Also, we cannot call removeSender because that will remove every
connections, making impossible for another object destroyed in the same
time to clean up the senders list, so call derefSender instead.

Reviewed-by: Brad

3 files changed, 100 insertions, 20 deletions

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 05015c0..f1a1eb5 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -345,18 +345,6 @@ void QObjectPrivate::derefSender(QObject *sender, int signal)
     // Q_ASSERT_X(false, "QObjectPrivate::derefSender", "sender not found");
 }
 
-void QObjectPrivate::removeSender(QObject *sender, int signal)
-{
-    for (int i = 0; i < senders.count(); ++i) {
-        Sender &s = senders[i];
-        if (s.sender == sender && s.signal == signal) {
-            senders.removeAt(i);
-            return;
-        }
-    }
-    // Q_ASSERT_X(false, "QObjectPrivate::removeSender", "sender not found");
-}
-
 QObjectPrivate::Sender *QObjectPrivate::setCurrentSender(QObject *receiver,
                                                          Sender *sender)
 {
@@ -790,7 +778,7 @@ QObject::~QObject()
                     bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m);
                     c = &connectionList[i];
                     if (c->receiver)
-                        c->receiver->d_func()->removeSender(this, signal);
+                        c->receiver->d_func()->derefSender(this, signal);
                     if (needToUnlock)
                         m->unlock();
 
@@ -811,18 +799,22 @@ QObject::~QObject()
         }
 
         // disconnect all senders
-        for (int i = 0; i < d->senders.count(); ++i) {
+        for (int i = 0; i < d->senders.count(); ) {
             QObjectPrivate::Sender *s = &d->senders[i];
-            if (!s->sender)
-                continue;
 
             QMutex *m = &s->sender->d_func()->threadData->mutex;
             bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m);
-            s = &d->senders[i];
-            if (s->sender)
-                s->sender->d_func()->removeReceiver(s->signal, this);
+            if (m < locker.mutex()) {
+                if (i >= d->senders.count() || s != &d->senders[i]) {
+                    if (needToUnlock)
+                        m->unlock();
+                    continue;
+                }
+            }
+            s->sender->d_func()->removeReceiver(s->signal, this);
             if (needToUnlock)
                 m->unlock();
+            ++i;
         }
 
         d->senders.clear();
diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h
index b324334..0eed938 100644
--- a/src/corelib/kernel/qobject_p.h
+++ b/src/corelib/kernel/qobject_p.h
@@ -163,7 +163,6 @@ public:
     QList<Sender> senders;
     void refSender(QObject *sender, int signal);
     void derefSender(QObject *sender, int signal);
-    void removeSender(QObject *sender, int signal);
 
     static Sender *setCurrentSender(QObject *receiver,
                                     Sender *sender);
diff --git a/tests/auto/qobjectrace/tst_qobjectrace.cpp b/tests/auto/qobjectrace/tst_qobjectrace.cpp
index 0c88f29..fcfd528 100644
--- a/tests/auto/qobjectrace/tst_qobjectrace.cpp
+++ b/tests/auto/qobjectrace/tst_qobjectrace.cpp
@@ -50,6 +50,7 @@ class tst_QObjectRace: public QObject
     Q_OBJECT
 private slots:
     void moveToThreadRace();
+    void destroyRace();
 };
 
 class RaceObject : public QObject
@@ -147,5 +148,93 @@ void tst_QObjectRace::moveToThreadRace()
     delete object;
 }
 
+
+class MyObject : public QObject
+{   Q_OBJECT
+    public slots:
+        void slot1() { emit signal1(); }
+        void slot2() { emit signal2(); }
+        void slot3() { emit signal3(); }
+        void slot4() { emit signal4(); }
+        void slot5() { emit signal5(); }
+        void slot6() { emit signal6(); }
+        void slot7() { emit signal7(); }
+    signals:
+        void signal1();
+        void signal2();
+        void signal3();
+        void signal4();
+        void signal5();
+        void signal6();
+        void signal7();
+};
+
+
+
+class DestroyThread : public QThread
+{
+    Q_OBJECT
+    QObject **objects;
+    int number;
+
+public:
+    void setObjects(QObject **o, int n)
+    {
+        objects = o;
+        number = n;
+        for(int i = 0; i < number; i++)
+            objects[i]->moveToThread(this);
+    }
+
+    void run() {
+        for(int i = 0; i < number; i++)
+            delete objects[i];
+    }
+};
+
+void tst_QObjectRace::destroyRace()
+{
+    enum { ThreadCount = 10, ObjectCountPerThread = 733, 
+           ObjectCount = ThreadCount * ObjectCountPerThread };
+
+    const char *_slots[] = { SLOT(slot1()) , SLOT(slot2()) , SLOT(slot3()),
+                             SLOT(slot4()) , SLOT(slot5()) , SLOT(slot6()),
+                             SLOT(slot7()) };
+
+    const char *_signals[] = { SIGNAL(signal1()), SIGNAL(signal2()), SIGNAL(signal3()),
+                               SIGNAL(signal4()), SIGNAL(signal5()), SIGNAL(signal6()),
+                               SIGNAL(signal7()) };
+
+    QObject *objects[ObjectCount];
+    for (int i = 0; i < ObjectCount; ++i)
+        objects[i] = new MyObject;
+
+
+    for (int i = 0; i < ObjectCount * 11; ++i) {
+        connect(objects[(i*13) % ObjectCount], _signals[(2*i)%7],
+                objects[((i+2)*17) % ObjectCount],  _slots[(3*i+2)%7] );
+        connect(objects[((i+6)*23) % ObjectCount], _signals[(5*i+4)%7],
+                objects[((i+8)*41) % ObjectCount],  _slots[(i+6)%7] );
+    }
+
+    DestroyThread *threads[ThreadCount];
+    for (int i = 0; i < ThreadCount; ++i) {
+        threads[i] = new DestroyThread;
+        threads[i]->setObjects(objects + i*ObjectCountPerThread, ObjectCountPerThread);
+    }
+
+    for (int i = 0; i < ThreadCount; ++i)
+        threads[i]->start();
+
+    QVERIFY(threads[0]->wait(TwoMinutes));
+    // the other threads should finish pretty quickly now
+    for (int i = 1; i < ThreadCount; ++i)
+        QVERIFY(threads[i]->wait(3000));
+
+    for (int i = 0; i < ThreadCount; ++i)
+        delete threads[i];
+}
+
+
 QTEST_MAIN(tst_QObjectRace)
 #include "tst_qobjectrace.moc"