summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/corelib/kernel/qobject.cpp47
-rw-r--r--src/corelib/kernel/qobject_p.h16
-rw-r--r--tests/auto/other/qobjectrace/tst_qobjectrace.cpp49
3 files changed, 91 insertions, 21 deletions
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 4c2a787173..b38f39e2c8 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -389,8 +389,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;
@@ -419,8 +425,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
@@ -1035,7 +1040,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);
}
@@ -1076,11 +1081,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
@@ -5024,11 +5029,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;
}
@@ -5222,12 +5227,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 f945b79ca1..539818ac1d 100644
--- a/src/corelib/kernel/qobject_p.h
+++ b/src/corelib/kernel/qobject_p.h
@@ -271,7 +271,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);
@@ -318,8 +321,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"
generated by cgit v1.2.3 (git 2.25.1) at 2024-04-19 14:40:40 +0000