Fix deadlock on disconnectNotify() called from ~QObject

Normally, disconnectNotify() is called at the end of QObject::disconnect
and all the locks have been dropped. That is not the case for the
QObject destructor, so we need to deal with the fact that it there may
be some locks held.

I didn't catch this issue during testing because it depends on the
pointer addresses of the object being destroyed and that of the
QDBusAbstractInterface sender object, as we use one global, non-
recursive mutex pool. For the same reason, this patch is not testable.

The fix is simple: we don't need to remove the relay rules immediately.
It's ok for them to happen later, since the worst case scenario is that
we'll receive a few more signals than we have objects to deliver them
to. If that happens, we'll do a little more work than we have to. But in
the normal case, the amount of work is the same and we get the benefit
of returning more quickly from the destructor. What's more, if the
QDBusAbstractInterface object also gets destroyed, the events are
deleted and QDBusConnectionPrivate will clean everything up.

Task-number: QTBUG-48410
Change-Id: I42e7ef1a481840699a8dffff1406b789ba5217b3
Reviewed-by: Olivier Goffart (Woboq GmbH) <ogoffart@woboq.com>

3 files changed, 55 insertions, 13 deletions

diff --git a/src/dbus/qdbusabstractinterface.cpp b/src/dbus/qdbusabstractinterface.cpp
index 0f097517b7..d63a317612 100644
--- a/src/dbus/qdbusabstractinterface.cpp
+++ b/src/dbus/qdbusabstractinterface.cpp
@@ -35,6 +35,7 @@
 #include "qdbusabstractinterface.h"
 #include "qdbusabstractinterface_p.h"
 
+#include <qcoreapplication.h>
 #include <qthread.h>
 
 #include "qdbusargument.h"
@@ -51,6 +52,29 @@
 
 QT_BEGIN_NAMESPACE
 
+namespace {
+// ### Qt6: change to a regular QEvent (customEvent)
+// We need to use a QMetaCallEvent here because we can't override customEvent() in
+// Qt 5. Since QDBusAbstractInterface is meant to be derived from, the vtables of
+// classes in generated code will have a pointer to QObject::customEvent instead
+// of to QDBusAbstractInterface::customEvent.
+// See solution in Patch Set 1 of this change in the Qt Gerrit servers.
+// (https://codereview.qt-project.org/#/c/126384/1)
+class DisconnectRelayEvent : public QMetaCallEvent
+{
+public:
+    DisconnectRelayEvent(QObject *sender, const QMetaMethod &m)
+        : QMetaCallEvent(0, 0, Q_NULLPTR, sender, m.methodIndex())
+    {}
+
+    void placeMetaCall(QObject *object) Q_DECL_OVERRIDE
+    {
+        QDBusAbstractInterface *iface = static_cast<QDBusAbstractInterface *>(object);
+        QDBusAbstractInterfacePrivate::finishDisconnectNotify(iface, signalId());
+    }
+};
+}
+
 static QDBusError checkIfValid(const QString &service, const QString &path,
                                const QString &interface, bool isDynamic, bool isPeer)
 {
@@ -604,22 +628,38 @@ void QDBusAbstractInterface::disconnectNotify(const QMetaMethod &signal)
     if (!d->isValid)
         return;
 
+    // disconnection is just resource freeing, so it can be delayed;
+    // let's do that later, after all the QObject mutexes have been unlocked.
+    QCoreApplication::postEvent(this, new DisconnectRelayEvent(this, signal));
+}
+
+/*!
+    \internal
+    Continues the disconnect notification from above.
+*/
+void QDBusAbstractInterfacePrivate::finishDisconnectNotify(QDBusAbstractInterface *ptr, int signalId)
+{
+    QDBusAbstractInterfacePrivate *d = ptr->d_func();
     QDBusConnectionPrivate *conn = d->connectionPrivate();
-    if (conn && signal.isValid() && !isSignalConnected(signal))
-        return conn->disconnectRelay(d->service, d->path, d->interface,
-                                     this, signal);
     if (!conn)
         return;
 
-    // wildcard disconnecting, we need to figure out which of our signals are
-    // no longer connected to anything
-    const QMetaObject *mo = metaObject();
-    int midx = QObject::staticMetaObject.methodCount();
-    const int end = mo->methodCount();
-    for ( ; midx < end; ++midx) {
-        QMetaMethod mm = mo->method(midx);
-        if (mm.methodType() == QMetaMethod::Signal && !isSignalConnected(mm))
-            conn->disconnectRelay(d->service, d->path, d->interface, this, mm);
+    const QMetaObject *mo = ptr->metaObject();
+    QMetaMethod signal = signalId >= 0 ? mo->method(signalId) : QMetaMethod();
+    if (signal.isValid()) {
+        if (!ptr->isSignalConnected(signal))
+            return conn->disconnectRelay(d->service, d->path, d->interface,
+                                         ptr, signal);
+    } else {
+        // wildcard disconnecting, we need to figure out which of our signals are
+        // no longer connected to anything
+        int midx = QObject::staticMetaObject.methodCount();
+        const int end = mo->methodCount();
+        for ( ; midx < end; ++midx) {
+            QMetaMethod mm = mo->method(midx);
+            if (mm.methodType() == QMetaMethod::Signal && !ptr->isSignalConnected(mm))
+                conn->disconnectRelay(d->service, d->path, d->interface, ptr, mm);
+        }
     }
 }
 
diff --git a/src/dbus/qdbusabstractinterface_p.h b/src/dbus/qdbusabstractinterface_p.h
index 1ce457d94b..1d9290b746 100644
--- a/src/dbus/qdbusabstractinterface_p.h
+++ b/src/dbus/qdbusabstractinterface_p.h
@@ -91,6 +91,8 @@ public:
     { return QDBusConnectionPrivate::d(connection); }
 
     void _q_serviceOwnerChanged(const QString &name, const QString &oldOwner, const QString &newOwner);
+
+    static void finishDisconnectNotify(QDBusAbstractInterface *iface, int signalId);
 };
 
 QT_END_NAMESPACE
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 7203f05a9b..c465706913 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -2278,7 +2278,7 @@ void QDBusConnectionPrivate::disconnectRelay(const QString &service,
     sig.append(signal.methodSignature());
     if (!prepareHook(hook, key, service, path, interface, QString(), QStringList(), receiver, sig,
                      QDBusAbstractInterface::staticMetaObject.methodCount(), true))
-        return;                 // don't connect
+        return;                 // don't disconnect
 
     Q_ASSERT(thread() != QThread::currentThread());
     emit signalNeedsDisconnecting(key, hook);