Fix a race that occurred as we unlock the mutex to destroy the functor in ~QObject

When we unlock the mutex, we need to take in account that the Connection
pointed by 'node' may be destroyed in another thread while it is unlocked
Doing  'node->prev = &node'  will make sure that 'node' is actually
updated when it is destroyed.

Setting isSlotObject under the mutex is safer and ensure that no other
thread will attempt to deref the object.

The regression was introduced in 5885b8f775998c30d53f40b7f368c5f6364e6df4

tst_qobjectrace was updated to catch races arising when we are
connecting with function pointers.

Change-Id: Ia0d11ae8df563dad97eb86993a786b579b28cd03
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@digia.com>
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

2 files changed, 73 insertions, 23 deletions

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 6ed3b30917..777f95ef6d 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2013 Digia Plc and/or its subsidiary(-ies).
+** Copyright (C) 2013 Olivier Goffart <ogoffart@woboq.com>
 ** Contact: http://www.qt-project.org/legal
 **
 ** This file is part of the QtCore module of the Qt Toolkit.
@@ -852,9 +853,9 @@ QObject::~QObject()
 
                     // The destroy operation must happen outside the lock
                     if (c->isSlotObject) {
+                        c->isSlotObject = false;
                         locker.unlock();
                         c->slotObj->destroyIfLastRef();
-                        c->isSlotObject = false;
                         locker.relock();
                     }
                     c->deref();
@@ -869,7 +870,15 @@ QObject::~QObject()
             d->connectionLists = 0;
         }
 
-        // disconnect all senders
+        /* Disconnect all senders:
+         * This loop basically just does
+         *     for (node = d->senders; node; node = node->next) { ... }
+         *
+         * We need to temporarily unlock the receiver mutex to destroy the functors or to lock the
+         * sender's mutex. And when the mutex is released, node->next might be destroyed by another
+         * thread. That's why we set node->prev to &node, that way, if node is destroyed, node will
+         * be updated.
+         */
         QObjectPrivate::Connection *node = d->senders;
         while (node) {
             QObject *sender = node->sender;
@@ -882,6 +891,8 @@ QObject::~QObject()
             bool needToUnlock = QOrderedMutexLocker::relock(signalSlotMutex, m);
             //the node has maybe been removed while the mutex was unlocked in relock?
             if (!node || node->sender != sender) {
+                // We hold the wrong mutex
+                Q_ASSERT(needToUnlock);
                 m->unlock();
                 continue;
             }
@@ -901,11 +912,12 @@ QObject::~QObject()
                 m->unlock();
 
             if (slotObj) {
+                if (node)
+                    node->prev = &node;
                 locker.unlock();
                 slotObj->destroyIfLastRef();
                 locker.relock();
             }
-
         }
     }
 
@@ -3186,9 +3198,9 @@ bool QMetaObjectPrivate::disconnectHelper(QObjectPrivate::Connection *c,
             c->receiver = 0;
 
             if (c->isSlotObject) {
+                c->isSlotObject = false;
                 senderMutex->unlock();
                 c->slotObj->destroyIfLastRef();
-                c->isSlotObject = false;
                 senderMutex->lock();
             }
 
diff --git a/tests/auto/other/qobjectrace/tst_qobjectrace.cpp b/tests/auto/other/qobjectrace/tst_qobjectrace.cpp
index ab05c64fe5..71a90e83f7 100644
--- a/tests/auto/other/qobjectrace/tst_qobjectrace.cpp
+++ b/tests/auto/other/qobjectrace/tst_qobjectrace.cpp
@@ -47,6 +47,12 @@
 enum { OneMinute = 60 * 1000,
        TwoMinutes = OneMinute * 2 };
 
+
+struct Functor
+{
+    void operator()() const {};
+};
+
 class tst_QObjectRace: public QObject
 {
     Q_OBJECT
@@ -122,11 +128,7 @@ signals:
 private slots:
     void checkStopWatch()
     {
-#if defined(Q_OS_WINCE) || defined(Q_OS_VXWORKS)
-        if (stopWatch.elapsed() >= OneMinute / 2)
-#else
-        if (stopWatch.elapsed() >= OneMinute)
-#endif
+        if (stopWatch.elapsed() >= 5000)
             quit();
 
         QObject o;
@@ -188,16 +190,34 @@ class MyObject : public QObject
         void signal7();
 };
 
+namespace {
+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()) };
 
+typedef void (MyObject::*PMFType)();
+const PMFType _slotsPMF[] = { &MyObject::slot1, &MyObject::slot2, &MyObject::slot3,
+                              &MyObject::slot4, &MyObject::slot5, &MyObject::slot6,
+                              &MyObject::slot7 };
+
+const PMFType _signalsPMF[] = { &MyObject::signal1, &MyObject::signal2, &MyObject::signal3,
+                                &MyObject::signal4, &MyObject::signal5, &MyObject::signal6,
+                                &MyObject::signal7 };
+
+}
 
 class DestroyThread : public QThread
 {
     Q_OBJECT
-    QObject **objects;
+    MyObject **objects;
     int number;
 
 public:
-    void setObjects(QObject **o, int n)
+    void setObjects(MyObject **o, int n)
     {
         objects = o;
         number = n;
@@ -206,8 +226,29 @@ public:
     }
 
     void run() {
-        for(int i = 0; i < number; i++)
+        for (int i = number-1; i >= 0; --i) {
+            /* Do some more connection and disconnection between object in this thread that have not been destroyed yet */
+
+            const int nAlive = i+1;
+            connect   (objects[((i+1)*31) % nAlive], _signals[(12*i)%7], objects[((i+2)*37) % nAlive],  _slots[(15*i+2)%7] );
+            disconnect(objects[((i+1)*31) % nAlive], _signals[(12*i)%7], objects[((i+2)*37) % nAlive],  _slots[(15*i+2)%7] );
+
+            connect   (objects[((i+4)*41) % nAlive], _signalsPMF[(18*i)%7], objects[((i+5)*43) % nAlive],  _slotsPMF[(19*i+2)%7] );
+            disconnect(objects[((i+4)*41) % nAlive], _signalsPMF[(18*i)%7], objects[((i+5)*43) % nAlive],  _slotsPMF[(19*i+2)%7] );
+
+            QMetaObject::Connection c = connect(objects[((i+5)*43) % nAlive], _signalsPMF[(9*i+1)%7], Functor());
+            disconnect(c);
+
+            disconnect(objects[i], _signalsPMF[(10*i+5)%7], 0, 0);
+            disconnect(objects[i], _signals[(11*i+6)%7], 0, 0);
+
+            disconnect(objects[i], 0, objects[(i*17+6) % nAlive], 0);
+            if (i%4 == 1) {
+                disconnect(objects[i], 0, 0, 0);
+            }
+
             delete objects[i];
+        }
     }
 };
 
@@ -216,27 +257,24 @@ public:
 
 void tst_QObjectRace::destroyRace()
 {
-    enum { ThreadCount = 10, ObjectCountPerThread = 733,
+    enum { ThreadCount = 10, ObjectCountPerThread = 2777,
            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];
+    MyObject *objects[ObjectCount];
     for (int i = 0; i < ObjectCount; ++i)
         objects[i] = new MyObject;
 
 
-    for (int i = 0; i < ObjectCount * 11; ++i) {
+    for (int i = 0; i < ObjectCount * 17; ++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] );
+
+        connect(objects[(i*67) % ObjectCount], _signalsPMF[(2*i)%7],
+                objects[((i+1)*71) % ObjectCount],  _slotsPMF[(3*i+2)%7] );
+        connect(objects[((i+3)*73) % ObjectCount], _signalsPMF[(5*i+4)%7],
+                objects[((i+5)*79) % ObjectCount],  Functor() );
     }
 
     DestroyThread *threads[ThreadCount];