From b46e17918675f2130e6faa792ce51fee7d890ad5 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Mon, 29 Aug 2022 14:24:52 -0300 Subject: [PATCH] QObject: attempt to fix a deadlock introduced by an earlier fix Commit 71b4d4f150bc3c904a5aceec37513ddc3cd1c150 is likely the source of the issue. It fixed a race on disconnection, but kept the call to disconnectNotify() (which is user code) inside the locked section. My analysis is that by construction the sender object can't be undergoing concurrent deletion anyway at this point. All call sites (QObject::disconnect or the signal-slot activations but before the slot is activated) imply that the user code that reached here cannot itself be racing the deletion. There may be one race condition left: if the same signal was connected earlier to a slot via queued connection and that slot deletes the sender asynchronously. A synchronous deletion is handled by doActivate(), so the single-shot connection is never activated in the first place, but an asynchronous deletion could race past that check and delete the sender while QObjectPrivate::removeConnection is running. However, I'd call this a mistake in user code. [ChangeLog][QtCore][QObject] Fixed a regression from 6.3 that caused QObject::isSignalConnected() to deadlock if called from inside disconnectNotify(). Fixes: QTBUG-106025 Change-Id: Ic6547f8247454b47baa8fffd170fe0bdb62cfcaf Reviewed-by: Qt CI Bot Reviewed-by: Lars Knoll (cherry picked from commit 22d4c67234fd152296c3ec98fc57526356a9f62b) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/kernel/qobject.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index a0ee963713f..b5838d2e7b0 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -5275,11 +5275,20 @@ bool QObjectPrivate::disconnect(const QObject *sender, int signal_index, const Q /*! \internal \threadsafe + + Thread-safety warning: this function may be called from any thread and is + thread-safe, \b{so long as the sender is not being deleted}. At the time of + this writing, this function is called from QObject::disconnect() and from + the multiple places where a single-shot connection is activated; in both + cases, the construction of the user code is already such that the sender + object cannot be undergoing deletion in another thread. */ bool QObjectPrivate::disconnect(QObjectPrivate::Connection *c) { if (!c) return false; + + // double-checked locking on this pointer QObject *receiver = c->receiver.loadRelaxed(); if (!receiver) return false; @@ -5300,7 +5309,6 @@ bool QObjectPrivate::disconnect(QObjectPrivate::Connection *c) Q_ASSERT(connections); connections->removeConnection(c); - c->sender->disconnectNotify(QMetaObjectPrivate::signal(c->sender->metaObject(), c->signal_index)); // We must not hold the receiver mutex, else we risk dead-locking; we also only need the sender mutex // It is however vital to hold the senderMutex before calling cleanOrphanedConnections, as otherwise // another thread might modify/delete the connection @@ -5312,6 +5320,8 @@ bool QObjectPrivate::disconnect(QObjectPrivate::Connection *c) locker.dismiss(); // so we dismiss the QOrderedMutexLocker } + // this is safe if the condition in the documentation is correct + c->sender->disconnectNotify(QMetaObjectPrivate::signal(c->sender->metaObject(), c->signal_index)); return true; }