QObject: cleanup the orphaned connection lists on destruction

When a signal/slot connection is broken, it gets added to the
sender's list of "orphaned connections", to clean up later.
This cleanup happens when the sender gets destroyed or as soon as
it emits any signal.

This may cause soft memory leaks in case receivers get destroyed,
and the sender is a long living object and doesn't emit signals
for a while (e.g. QThread).

For some reason, an explicit disconnection cleans up the list
(either by using the QMetaObject::Connection object, or in case
of string-based connect, using a string-based disconnect). This
raises lots of doubts about why having this list in the first
place.

Fix the soft-leak by cleaning up the orphaned connection list when
destroying a receiver.

Note: I still believe that we shouldn't have any "orphaned"
connection list, and rather cleanup on disconnect/deletion
(otherwise, emitting a signal may cause a CPU spike because it
triggers a cleanup).  If we allow for any "impredictability" during
signal activation we're just admitting that signals/slots aren't
suitable for e.g. low-latency codepaths. That's why I'm not marking
the problem as fixed.

Original-patch-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
Task-number: QTBUG-88248
Task-number: QTBUG-87774
Pick-to: 6.2 6.1 5.15
Change-Id: Id25f67a45dff49f740132a44d36e88740eb12070
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
This commit is contained in:
Giuseppe D'Angelo 2020-11-07 01:15:34 +01:00 committed by Fabian Kosmale
parent 71b4d4f150
commit c2839843f2

View File

@ -1078,13 +1078,28 @@ QObject::~QObject()
} }
senderData->removeConnection(node); senderData->removeConnection(node);
/*
When we unlock, another thread has the chance to delete/modify sender data.
Thus we need to call cleanOrphanedConnections before unlocking. We use the
variant of the function which assumes that the lock is already held to avoid
a deadlock.
We need to hold m, the sender lock. Considering that we might execute arbitrary user
code, we should already release the signalSlotMutex here unless they are the same.
*/
const bool locksAreTheSame = signalSlotMutex == m;
if (!locksAreTheSame)
locker.unlock();
senderData->cleanOrphanedConnections(
sender,
QObjectPrivate::ConnectionData::AlreadyLockedAndTemporarilyReleasingLock
);
if (needToUnlock) if (needToUnlock)
m->unlock(); m->unlock();
if (locksAreTheSame) // otherwise already unlocked
locker.unlock(); locker.unlock();
if (slotObj) if (slotObj)
slotObj->destroyIfLastRef(); slotObj->destroyIfLastRef();
senderData->cleanOrphanedConnections(sender);
locker.relock(); locker.relock();
} }