From a952fd7d5b03ce1968ec58871fbb8b3600895900 Mon Sep 17 00:00:00 2001 From: Kari Oikarinen Date: Wed, 26 Sep 2018 10:29:14 +0300 Subject: [PATCH] QObject: Fix isSignalConnected() when signals have been disconnected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bitmap cache for the first 64 signals being connected was only set when the connection is added. It was never unset when the connection was removed. Internal use of the connectedSignals bitmap is not hurt by it occasionally saying a signal is connected even though it is not, since the purpose of those checks is avoiding expensive operations that are not necessary if nothing is connected to the signal. However, the public API using this cache meant that it also never spotted signals being disconnected. This was not documented. Fix the behavior by only using the cache if it is up to date. If it is not, use a slower path that gives the correct answer. To avoid making disconnections and QObject destructions slower, the cache is only updated to unset disconnected signals when new signal connections are added. No extra work is done in the common case where signals are only removed in the end of the QObject's lifetime. Fixes: QTBUG-32340 Change-Id: Ieb6e498060157153cec60d9c8f1c33056993fda1 Reviewed-by: Ville Voutilainen Reviewed-by: Thiago Macieira Reviewed-by: Olivier Goffart (Woboq GmbH) Reviewed-by: Jędrzej Nowacki --- src/corelib/kernel/qobject.cpp | 34 ++++++++---- .../corelib/kernel/qobject/tst_qobject.cpp | 53 +++++++++++++++++++ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 07208b7f40..deab51cfd0 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -421,6 +421,7 @@ void QObjectPrivate::cleanConnectionLists() { if (connectionLists->dirty && !connectionLists->inUse) { // remove broken connections + bool allConnected = false; for (int signal = -1; signal < connectionLists->count(); ++signal) { QObjectPrivate::ConnectionList &connectionList = (*connectionLists)[signal]; @@ -432,11 +433,13 @@ void QObjectPrivate::cleanConnectionLists() QObjectPrivate::Connection **prev = &connectionList.first; QObjectPrivate::Connection *c = *prev; + bool connected = false; // whether the signal is still connected somewhere while (c) { if (c->receiver) { last = c; prev = &c->nextConnectionList; c = *prev; + connected = true; } else { QObjectPrivate::Connection *next = c->nextConnectionList; *prev = next; @@ -448,6 +451,14 @@ void QObjectPrivate::cleanConnectionLists() // Correct the connection list's last pointer. // As conectionList.last could equal last, this could be a noop connectionList.last = last; + + if (!allConnected && !connected && signal >= 0 + && size_t(signal) < sizeof(connectedSignals) * 8) { + // This signal is no longer connected + connectedSignals[signal >> 5] &= ~(1 << (signal & 0x1f)); + } else if (signal == -1) { + allConnected = connected; + } } connectionLists->dirty = false; } @@ -2501,19 +2512,20 @@ bool QObject::isSignalConnected(const QMetaMethod &signal) const signalIndex += QMetaObjectPrivate::signalOffset(signal.mobj); - if (signalIndex < sizeof(d->connectedSignals) * 8) + QMutexLocker locker(signalSlotLock(this)); + if (!d->connectionLists) + return false; + + if (signalIndex < sizeof(d->connectedSignals) * 8 && !d->connectionLists->dirty) return d->isSignalConnected(signalIndex); - QMutexLocker locker(signalSlotLock(this)); - if (d->connectionLists) { - if (signalIndex < uint(d->connectionLists->count())) { - const QObjectPrivate::Connection *c = - d->connectionLists->at(signalIndex).first; - while (c) { - if (c->receiver) - return true; - c = c->nextConnectionList; - } + if (signalIndex < uint(d->connectionLists->count())) { + const QObjectPrivate::Connection *c = + d->connectionLists->at(signalIndex).first; + while (c) { + if (c->receiver) + return true; + c = c->nextConnectionList; } } return false; diff --git a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp index 68c6ece583..effc82261b 100644 --- a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp +++ b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp @@ -103,6 +103,7 @@ private slots: void deleteQObjectWhenDeletingEvent(); void overloads(); void isSignalConnected(); + void isSignalConnectedAfterDisconnection(); void qMetaObjectConnect(); void qMetaObjectDisconnectOne(); void sameName(); @@ -3835,6 +3836,58 @@ void tst_QObject::isSignalConnected() QVERIFY(!o.isSignalConnected(QMetaMethod())); } +void tst_QObject::isSignalConnectedAfterDisconnection() +{ + ManySignals o; + const QMetaObject *meta = o.metaObject(); + + const QMetaMethod sig00 = meta->method(meta->indexOfSignal("sig00()")); + QVERIFY(!o.isSignalConnected(sig00)); + QObject::connect(&o, &ManySignals::sig00, qt_noop); + QVERIFY(o.isSignalConnected(sig00)); + QVERIFY(QObject::disconnect(&o, &ManySignals::sig00, 0, 0)); + QVERIFY(!o.isSignalConnected(sig00)); + + const QMetaMethod sig69 = meta->method(meta->indexOfSignal("sig69()")); + QVERIFY(!o.isSignalConnected(sig69)); + QObject::connect(&o, &ManySignals::sig69, qt_noop); + QVERIFY(o.isSignalConnected(sig69)); + QVERIFY(QObject::disconnect(&o, &ManySignals::sig69, 0, 0)); + QVERIFY(!o.isSignalConnected(sig69)); + + { + ManySignals o2; + QObject::connect(&o, &ManySignals::sig00, &o2, &ManySignals::sig00); + QVERIFY(o.isSignalConnected(sig00)); + // o2 is destructed + } + QVERIFY(!o.isSignalConnected(sig00)); + + const QMetaMethod sig01 = meta->method(meta->indexOfSignal("sig01()")); + QObject::connect(&o, &ManySignals::sig00, qt_noop); + QObject::connect(&o, &ManySignals::sig01, qt_noop); + QObject::connect(&o, &ManySignals::sig69, qt_noop); + QVERIFY(o.isSignalConnected(sig00)); + QVERIFY(o.isSignalConnected(sig01)); + QVERIFY(o.isSignalConnected(sig69)); + QVERIFY(QObject::disconnect(&o, &ManySignals::sig69, 0, 0)); + QVERIFY(o.isSignalConnected(sig00)); + QVERIFY(o.isSignalConnected(sig01)); + QVERIFY(!o.isSignalConnected(sig69)); + QVERIFY(QObject::disconnect(&o, &ManySignals::sig00, 0, 0)); + QVERIFY(!o.isSignalConnected(sig00)); + QVERIFY(o.isSignalConnected(sig01)); + QVERIFY(!o.isSignalConnected(sig69)); + QObject::connect(&o, &ManySignals::sig69, qt_noop); + QVERIFY(!o.isSignalConnected(sig00)); + QVERIFY(o.isSignalConnected(sig01)); + QVERIFY(o.isSignalConnected(sig69)); + QVERIFY(QObject::disconnect(&o, &ManySignals::sig01, 0, 0)); + QVERIFY(!o.isSignalConnected(sig00)); + QVERIFY(!o.isSignalConnected(sig01)); + QVERIFY(o.isSignalConnected(sig69)); +} + void tst_QObject::qMetaObjectConnect() { SenderObject *s = new SenderObject;