diff options
-rw-r--r-- | src/corelib/kernel/qobject.cpp | 47 | ||||
-rw-r--r-- | src/corelib/kernel/qobject_p.h | 16 | ||||
-rw-r--r-- | tests/auto/other/qobjectrace/tst_qobjectrace.cpp | 49 |
3 files changed, 91 insertions, 21 deletions
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 33afcaaa7e..326fba5be2 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -390,8 +390,14 @@ void QObjectPrivate::ConnectionData::removeConnection(QObjectPrivate::Connection Q_ASSERT(c != orphaned.loadRelaxed()); // add c to orphanedConnections - c->nextInOrphanList = orphaned.loadRelaxed(); - orphaned.storeRelaxed(c); + Connection *o = nullptr; + /* No ABA issue here: When adding a node, we only care about the list head, it doesn't + * matter if the tail changes. + */ + do { + o = orphaned.loadRelaxed(); + c->nextInOrphanList = o; + } while (!orphaned.testAndSetRelease(o, c)); #ifndef QT_NO_DEBUG found = false; @@ -420,8 +426,7 @@ void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sende // Since ref == 1, no activate() is in process since we locked the mutex. That implies, // that nothing can reference the orphaned connection objects anymore and they can // be safely deleted - c = orphaned.loadRelaxed(); - orphaned.storeRelaxed(nullptr); + c = orphaned.fetchAndStoreRelaxed(nullptr); } if (c) { // Deleting c might run arbitrary user code, so we must not hold the lock @@ -1027,7 +1032,7 @@ QObject::~QObject() QBasicMutex *m = signalSlotLock(c->receiver.loadRelaxed()); bool needToUnlock = QOrderedMutexLocker::relock(signalSlotMutex, m); - if (c->receiver.loadAcquire()) { + if (c == connectionList.first.loadAcquire() && c->receiver.loadAcquire()) { cd->removeConnection(c); Q_ASSERT(connectionList.first.loadRelaxed() != c); } @@ -1068,11 +1073,11 @@ QObject::~QObject() if (needToUnlock) m->unlock(); - if (slotObj) { - locker.unlock(); + locker.unlock(); + if (slotObj) slotObj->destroyIfLastRef(); - locker.relock(); - } + senderData->cleanOrphanedConnections(sender); + locker.relock(); } // invalidate all connections on the object and make sure @@ -4978,11 +4983,11 @@ QMetaObject::Connection QObjectPrivate::connectImpl(const QObject *sender, int s bool QObject::disconnect(const QMetaObject::Connection &connection) { QObjectPrivate::Connection *c = static_cast<QObjectPrivate::Connection *>(connection.d_ptr); + if (!c) + return false; const bool disconnected = QObjectPrivate::disconnect(c); - if (disconnected) { - const_cast<QMetaObject::Connection &>(connection).d_ptr = nullptr; - c->deref(); // has been removed from the QMetaObject::Connection object - } + const_cast<QMetaObject::Connection &>(connection).d_ptr = nullptr; + c->deref(); // has been removed from the QMetaObject::Connection object return disconnected; } @@ -5176,12 +5181,18 @@ bool QObjectPrivate::disconnect(QObjectPrivate::Connection *c) connections = QObjectPrivate::get(c->sender)->connections.loadRelaxed(); Q_ASSERT(connections); connections->removeConnection(c); - } - - connections->cleanOrphanedConnections(c->sender); - c->sender->disconnectNotify(QMetaObjectPrivate::signal(c->sender->metaObject(), - c->signal_index)); + 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 + if (receiverMutex != senderMutex) { + receiverMutex->unlock(); + } + connections->cleanOrphanedConnections(c->sender, ConnectionData::AlreadyLockedAndTemporarilyReleasingLock); + senderMutex->unlock(); // now both sender and receiver mutex have been manually unlocked + locker.dismiss(); // so we dismiss the QOrderedMutexLocker + } return true; } diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h index 8dd29e05ce..4d5543d34a 100644 --- a/src/corelib/kernel/qobject_p.h +++ b/src/corelib/kernel/qobject_p.h @@ -257,7 +257,10 @@ public: ~ConnectionData() { - deleteOrphaned(orphaned.loadRelaxed()); + Q_ASSERT(ref.loadRelaxed() == 0); + auto *c = orphaned.fetchAndStoreRelaxed(nullptr); + if (c) + deleteOrphaned(c); SignalVector *v = signalVector.loadRelaxed(); if (v) free(v); @@ -304,8 +307,15 @@ public: signalVector.storeRelaxed(newVector); if (vector) { - vector->nextInOrphanList = orphaned.loadRelaxed(); - orphaned.storeRelaxed(ConnectionOrSignalVector::fromSignalVector(vector)); + Connection *o = nullptr; + /* No ABA issue here: When adding a node, we only care about the list head, it doesn't + * matter if the tail changes. + */ + do { + o = orphaned.loadRelaxed(); + vector->nextInOrphanList = o; + } while (!orphaned.testAndSetRelease(o, ConnectionOrSignalVector::fromSignalVector(vector))); + } } int signalVectorCount() const diff --git a/tests/auto/other/qobjectrace/tst_qobjectrace.cpp b/tests/auto/other/qobjectrace/tst_qobjectrace.cpp index ac33fa3ec3..093b4d2476 100644 --- a/tests/auto/other/qobjectrace/tst_qobjectrace.cpp +++ b/tests/auto/other/qobjectrace/tst_qobjectrace.cpp @@ -49,6 +49,7 @@ private slots: void destroyRace(); void blockingQueuedDestroyRace(); void disconnectRace(); + void disconnectRace2(); }; class RaceObject : public QObject @@ -562,5 +563,53 @@ void tst_QObjectRace::disconnectRace() QCOMPARE(countedStructObjectsCount.loadRelaxed(), 0u); } +void tst_QObjectRace::disconnectRace2() +{ + enum { IterationCount = 100, ConnectionCount = 100, YieldCount = 100 }; + + QAtomicPointer<MyObject> ptr; + QSemaphore createSemaphore(0); + QSemaphore proceedSemaphore(0); + + std::unique_ptr<QThread> t1(QThread::create([&]() { + for (int i = 0; i < IterationCount; ++i) { + MyObject sender; + ptr.storeRelease(&sender); + createSemaphore.release(); + proceedSemaphore.acquire(); + ptr.storeRelaxed(nullptr); + for (int i = 0; i < YieldCount; ++i) + QThread::yieldCurrentThread(); + } + })); + t1->start(); + + + std::unique_ptr<QThread> t2(QThread::create([&]() { + auto connections = std::make_unique<QMetaObject::Connection[]>(ConnectionCount); + for (int i = 0; i < IterationCount; ++i) { + MyObject receiver; + MyObject *sender = nullptr; + + createSemaphore.acquire(); + + while (!(sender = ptr.loadAcquire())) + ; + + for (int i = 0; i < ConnectionCount; ++i) + connections[i] = QObject::connect(sender, &MyObject::signal1, &receiver, &MyObject::slot1); + + proceedSemaphore.release(); + + for (int i = 0; i < ConnectionCount; ++i) + QObject::disconnect(connections[i]); + } + })); + t2->start(); + + t1->wait(); + t2->wait(); +} + QTEST_MAIN(tst_QObjectRace) #include "tst_qobjectrace.moc" |