qobject: Do not destroy slot objects inside a lock

This prevents deadlocks in case the destructor re-enters.
(Example: a functor containing a QSharedPointer of a QObject)
This also fixes a leaked slot object in disconnectHelper.

Change-Id: Ia939790e3b54e64067b99540974306b4808a77f2
Reviewed-by: Olivier Goffart <ogoffart@woboq.com>

2 files changed, 77 insertions, 15 deletions

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 02794e9fe3..33e2adf5ba 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -816,6 +816,14 @@ QObject::~QObject()
                         m->unlock();
 
                     connectionList.first = c->nextConnectionList;
+
+                    // The destroy operation must happen outside the lock
+                    if (c->isSlotObject) {
+                        locker.unlock();
+                        c->slotObj->destroyIfLastRef();
+                        c->isSlotObject = false;
+                        locker.relock();
+                    }
                     c->deref();
                 }
             }
@@ -3135,6 +3143,13 @@ bool QMetaObjectPrivate::disconnectHelper(QObjectPrivate::Connection *c,
 
             c->receiver = 0;
 
+            if (c->isSlotObject) {
+                senderMutex->unlock();
+                c->slotObj->destroyIfLastRef();
+                c->isSlotObject = false;
+                senderMutex->lock();
+            }
+
             success = true;
 
             if (disconnectType == DisconnectOne)
@@ -4363,16 +4378,19 @@ bool QObject::disconnect(const QMetaObject::Connection &connection)
 
     QMutex *senderMutex = signalSlotLock(c->sender);
     QMutex *receiverMutex = signalSlotLock(c->receiver);
-    QOrderedMutexLocker locker(senderMutex, receiverMutex);
 
-    QObjectConnectionListVector *connectionLists = QObjectPrivate::get(c->sender)->connectionLists;
-    Q_ASSERT(connectionLists);
-    connectionLists->dirty = true;
+    {
+        QOrderedMutexLocker locker(senderMutex, receiverMutex);
 
-    *c->prev = c->next;
-    if (c->next)
-        c->next->prev = c->prev;
-    c->receiver = 0;
+        QObjectConnectionListVector *connectionLists = QObjectPrivate::get(c->sender)->connectionLists;
+        Q_ASSERT(connectionLists);
+        connectionLists->dirty = true;
+
+        *c->prev = c->next;
+        if (c->next)
+            c->next->prev = c->prev;
+        c->receiver = 0;
+    }
 
     // destroy the QSlotObject, if possible
     if (c->isSlotObject) {
diff --git a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
index d16369de02..1cdf39018b 100644
--- a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
+++ b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
@@ -5726,27 +5726,44 @@ void tst_QObject::connectFunctorOverloads()
 #endif
 }
 
+class GetSenderObject : public QObject
+{
+    Q_OBJECT
+public:
+    QObject *accessSender() { return sender(); }
+
+public Q_SLOTS:
+    void triggerSignal() { Q_EMIT aSignal(); }
+
+Q_SIGNALS:
+    void aSignal();
+};
+
 static int countedStructObjectsCount = 0;
 struct CountedStruct
 {
-    CountedStruct() { ++countedStructObjectsCount; }
-    CountedStruct(const CountedStruct &) { ++countedStructObjectsCount; }
+    CountedStruct() : sender(Q_NULLPTR) { ++countedStructObjectsCount; }
+    CountedStruct(GetSenderObject *sender) : sender(sender) { ++countedStructObjectsCount; }
+    CountedStruct(const CountedStruct &o) : sender(o.sender) { ++countedStructObjectsCount; }
     CountedStruct &operator=(const CountedStruct &) { return *this; }
-    ~CountedStruct() { --countedStructObjectsCount; }
-    void operator()() const {}
+    // accessSender here allows us to check if there's a deadlock
+    ~CountedStruct() { --countedStructObjectsCount; if (sender != Q_NULLPTR) (void)sender->accessSender(); }
+    void operator()() const { }
+
+    GetSenderObject *sender;
 };
 
 void tst_QObject::disconnectDoesNotLeakFunctor()
 {
     QCOMPARE(countedStructObjectsCount, 0);
     {
+        GetSenderObject obj;
         QMetaObject::Connection c;
         {
-            CountedStruct s;
+            CountedStruct s(&obj);
             QCOMPARE(countedStructObjectsCount, 1);
-            QTimer timer;
 
-            c = connect(&timer, &QTimer::timeout, s);
+            c = connect(&obj, &GetSenderObject::aSignal, s);
             QVERIFY(c);
             QCOMPARE(countedStructObjectsCount, 2);
             QVERIFY(QObject::disconnect(c));
@@ -5797,6 +5814,33 @@ void tst_QObject::disconnectDoesNotLeakFunctor()
     }
     QCOMPARE(countedStructObjectsCount, 0);
     {
+        QTimer *timer = new QTimer;
+        QEventLoop e;
+
+        connect(timer, &QTimer::timeout, CountedStruct());
+        QCOMPARE(countedStructObjectsCount, 1); // only one instance, in Qt internals
+        timer->deleteLater();
+        connect(timer, &QObject::destroyed, &e, &QEventLoop::quit, Qt::QueuedConnection);
+        e.exec();
+        QCOMPARE(countedStructObjectsCount, 0); // functor being destroyed
+    }
+    QCOMPARE(countedStructObjectsCount, 0);
+    {
+        GetSenderObject obj;
+
+        connect(&obj, &GetSenderObject::aSignal, CountedStruct(&obj));
+        QCOMPARE(countedStructObjectsCount, 1);
+    }
+    QCOMPARE(countedStructObjectsCount, 0);
+    {
+        GetSenderObject obj;
+
+        connect(&obj, &GetSenderObject::aSignal, CountedStruct(&obj));
+        QCOMPARE(countedStructObjectsCount, 1);
+        QObject::disconnect(&obj, &GetSenderObject::aSignal, 0, 0);
+    }
+    QCOMPARE(countedStructObjectsCount, 0);
+    {
 #if defined(Q_COMPILER_LAMBDA)
         CountedStruct s;
         QCOMPARE(countedStructObjectsCount, 1);