From 7d96f726337d62f5cd37dd5c31ebcf8accd56bcd Mon Sep 17 00:00:00 2001 From: Luca Di Sera Date: Fri, 21 Feb 2025 14:12:04 +0100 Subject: [PATCH] Avoid a memory leak in ReferenceObject::init `ReferenceObject::init` currently allocates connections to certain signals to enable a reduction of the amount of reads the `ReferenceObject` needs to perform, by using the signal to identify when the data that is being represented was invalidated. In particular, when the `ReferenceObject` is part of a chain that traces back to a `Q_PROPERTY`, and that property either has a `NOTIFY` signal or is a `BINDABLE`, a connection to the signal or a subscription to the `BINDABLE` is activated. When one of those is done, we further construct a connection to the `destroyed` signal of the object that holds the property, to ensure that a read is performed and our data is invalidated if that object is destroyed. The code that performs this process, in `ReferenceObject::init`, was written with the incorrect assumption that a property either has a `NOTIFY` signal or is `BINDABLE`, but not both. In truth, a property might both have a `NOTIFY` signal and be a `BINDABLE`. When this is the case, the current code would allocate a connection to the `destroyed` signal on the same memory block twice, once when setting up a connection to the `NOTIFY` signal and once when subscribing to the `BINDABLE`, without ensuring that the previously allocated connection was disposed of. To avoid this issue, the code that takes care of setting up the connections is now exclusive between the two connections path, with a priority on the `BINDABLE` subscription, as this mirrors the already existing preference we have when dealing with bindings and is expected to be slightly more performant. The documentation for this connection process was modified to add a small mention of this priority of execution. Some defensive asserts were added to the relevant connection code, to ensure that we can catch the construction of multiple connections at once, which is to be considered a bug. The code that takes care of disposing of the `destroyed` signal connection was modified to ensure that we only take into account our allocation strategy and not our actual connection status, which, while they shouldn't generally be in discord, might incorrectly avoid a necessary disposal if they would. A comment that related to the condition for the disposal was modified to be more precise with regards to the new condition. Some test cases were added to `tst_qqmllanguage` to check the leak and `BINDABLE` preference behavior. Change-Id: Ibdc657fd857a8838797e47ff235f67cfaeec20de Reviewed-by: Fabian Kosmale --- src/qml/jsruntime/qv4referenceobject.cpp | 4 +++ src/qml/jsruntime/qv4referenceobject_p.h | 22 +++++++++------- ...nToTheDestroyedSignalOnANotifyBindable.qml | 8 ++++++ ...rsBindableConnectionToNotifyConnection.qml | 8 ++++++ tests/auto/qml/qqmllanguage/testtypes.h | 22 ++++++++++++++++ .../qml/qqmllanguage/tst_qqmllanguage.cpp | 26 +++++++++++++++++++ 6 files changed, 81 insertions(+), 9 deletions(-) create mode 100644 tests/auto/qml/qqmllanguage/data/referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable.qml create mode 100644 tests/auto/qml/qqmllanguage/data/referenceObjectPrefersBindableConnectionToNotifyConnection.qml diff --git a/src/qml/jsruntime/qv4referenceobject.cpp b/src/qml/jsruntime/qv4referenceobject.cpp index c054976868..fcad9b92dc 100644 --- a/src/qml/jsruntime/qv4referenceobject.cpp +++ b/src/qml/jsruntime/qv4referenceobject.cpp @@ -293,6 +293,10 @@ DEFINE_OBJECT_VTABLE(QV4::ReferenceObject); A ReferenceObject can take advantage of this to reduce the number of reads that are required when dealing with a \c{QObject}'s property provening data. + When a property has a \tt{NOTIFY} signal and is a \tt{BINDABLE} at + the same time, we only need to use one such connection. + Currently, the \tt{BINDABLE} subscription will take predecedence. + ReferenceObjects that are part of a \l{Reference object chains}{chain}, will traverse the chain up until a QOjbect holding root is found, and connect based on that object. diff --git a/src/qml/jsruntime/qv4referenceobject_p.h b/src/qml/jsruntime/qv4referenceobject_p.h index 34bedb690f..35690a7228 100644 --- a/src/qml/jsruntime/qv4referenceobject_p.h +++ b/src/qml/jsruntime/qv4referenceobject_p.h @@ -56,6 +56,9 @@ DECLARE_HEAP_OBJECT(ReferenceObject, Object) { Q_ASSERT(obj); Q_ASSERT(engine); + Q_ASSERT(!referenceEndpoint); + Q_ASSERT(!bindableNotifier); + referenceEndpoint = new ReferenceObjectEndpoint(this); referenceEndpoint->connect( obj, @@ -106,6 +109,9 @@ DECLARE_HEAP_OBJECT(ReferenceObject, Object) { Q_ASSERT(obj); Q_ASSERT(engine); + Q_ASSERT(!referenceEndpoint); + Q_ASSERT(!bindableNotifier); + bindableNotifier = new QPropertyNotifier(obj->metaObject()->property(property).bindable(obj).addNotifier([this](){ setDirty(true); })); new(onDelete) QMetaObject::Connection(QObject::connect(obj, &QObject::destroyed, [this](){ setDirty(true); })); }; @@ -135,11 +141,10 @@ DECLARE_HEAP_OBJECT(ReferenceObject, Object) { auto wrapper = static_cast(object); QObject* obj = wrapper->object(); - if (obj->metaObject()->property(property).hasNotifySignal() && internalClass->engine->qmlEngine()) - connectToNotifySignal(obj, property, internalClass->engine->qmlEngine()); - if (obj->metaObject()->property(property).isBindable() && internalClass->engine->qmlEngine()) connectToBindable(obj, property, internalClass->engine->qmlEngine()); + else if (obj->metaObject()->property(property).hasNotifySignal() && internalClass->engine->qmlEngine()) + connectToNotifySignal(obj, property, internalClass->engine->qmlEngine()); } if (object && object->internalClass->vtable->type == Managed::Type_QMLTypeWrapper) { @@ -149,11 +154,10 @@ DECLARE_HEAP_OBJECT(ReferenceObject, Object) { Scoped scopedWrapper(scope, wrapper); QObject* obj = scopedWrapper->object(); - if (obj->metaObject()->property(property).hasNotifySignal() && internalClass->engine->qmlEngine()) - connectToNotifySignal(obj, property, internalClass->engine->qmlEngine()); - if (obj->metaObject()->property(property).isBindable() && internalClass->engine->qmlEngine()) connectToBindable(obj, property, internalClass->engine->qmlEngine()); + else if (obj->metaObject()->property(property).hasNotifySignal() && internalClass->engine->qmlEngine()) + connectToNotifySignal(obj, property, internalClass->engine->qmlEngine()); } // If we could not connect to anything we don't have a way to @@ -208,9 +212,9 @@ DECLARE_HEAP_OBJECT(ReferenceObject, Object) { } void destroy() { - // If we are connected we must have connected to the destroyed - // signal too, and we should clean it up. - if (isConnected()) { + // If we allocated any connection then we must have connected + // to the destroyed signal too, and we should clean it up. + if (referenceEndpoint || bindableNotifier) { QObject::disconnect(*reinterpret_cast(&onDelete)); std::destroy_at(reinterpret_cast(&onDelete)); } diff --git a/tests/auto/qml/qqmllanguage/data/referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable.qml b/tests/auto/qml/qqmllanguage/data/referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable.qml new file mode 100644 index 0000000000..469d409979 --- /dev/null +++ b/tests/auto/qml/qqmllanguage/data/referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable.qml @@ -0,0 +1,8 @@ +import Test +import QtQml + +ReadCounter { + Component.onCompleted: { + notifyBindable + } +} diff --git a/tests/auto/qml/qqmllanguage/data/referenceObjectPrefersBindableConnectionToNotifyConnection.qml b/tests/auto/qml/qqmllanguage/data/referenceObjectPrefersBindableConnectionToNotifyConnection.qml new file mode 100644 index 0000000000..469d409979 --- /dev/null +++ b/tests/auto/qml/qqmllanguage/data/referenceObjectPrefersBindableConnectionToNotifyConnection.qml @@ -0,0 +1,8 @@ +import Test +import QtQml + +ReadCounter { + Component.onCompleted: { + notifyBindable + } +} diff --git a/tests/auto/qml/qqmllanguage/testtypes.h b/tests/auto/qml/qqmllanguage/testtypes.h index 046bd81b3d..82d0864a55 100644 --- a/tests/auto/qml/qqmllanguage/testtypes.h +++ b/tests/auto/qml/qqmllanguage/testtypes.h @@ -3188,6 +3188,7 @@ class ReadCounter : public QObject { Q_PROPERTY(ValueTypeWithEnum1 valueType READ getValueType WRITE setValueType NOTIFY valueTypeChanged) Q_PROPERTY(QStringList bindable READ getBindable WRITE default BINDABLE bindableProperty FINAL) Q_PROPERTY(ReadCounterInner inner READ getInner WRITE setInner NOTIFY innerChanged) + Q_PROPERTY(QStringList notifyBindable READ default WRITE default BINDABLE notifyBindableProperty NOTIFY notifyBindableChanged) public: ReadCounter(QObject* parent = nullptr) @@ -3226,11 +3227,31 @@ public: } void setInner(const ReadCounterInner& l) { m_inner = l; emit innerChanged(); } + QBindable notifyBindableProperty() { return QBindable(&m_notifyBindable); } + + std::size_t destroyedConnections = 0; + std::size_t notifyBindableSignalConnections = 0; + + void connectNotify(const QMetaMethod& signal) override { + if (signal == QMetaMethod::fromSignal(&ReadCounter::destroyed)) + ++destroyedConnections; + if (signal == QMetaMethod::fromSignal(&ReadCounter::notifyBindableChanged)) + ++notifyBindableSignalConnections; + } + + void disconnectNotify(const QMetaMethod& signal) override { + if (signal == QMetaMethod::fromSignal(&ReadCounter::destroyed)) + --destroyedConnections; + if (signal == QMetaMethod::fromSignal(&ReadCounter::notifyBindableChanged)) + --notifyBindableSignalConnections; + } + signals: void stringListChanged(); void dateTimeChanged(); void valueTypeChanged(); void innerChanged(); + void notifyBindableChanged(); private: QStringList m_stringList; @@ -3238,6 +3259,7 @@ private: ValueTypeWithEnum1 m_valueType; QProperty m_bindable; ReadCounterInner m_inner{}; + QProperty m_notifyBindable; }; #endif // TESTTYPES_H diff --git a/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp b/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp index 2a0c0b5fae..799e2f87c6 100644 --- a/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp +++ b/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp @@ -507,6 +507,8 @@ private slots: void referenceToSingletonReadsBackOnlyWhenRequired(); void referenceToBindableReadsBackOnlyWhenRequired(); void referenceObjectChainReadsBackAsRequiredBasedOnParentSignals(); + void referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable(); + void referenceObjectPrefersBindableConnectionToNotifyConnection(); private: QQmlEngine engine; @@ -9561,6 +9563,30 @@ void tst_qqmllanguage::referenceObjectChainReadsBackAsRequiredBasedOnParentSigna QCOMPARE(inner.timesRead, 4); } +void tst_qqmllanguage::referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable() { + QQmlEngine engine; + QQmlComponent c(&engine, testFileUrl("referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable.qml")); + QVERIFY2(c.isReady(), qPrintable(c.errorString())); + QScopedPointer o(c.create()); + + auto *readCounter = qobject_cast(o.data()); + QVERIFY(readCounter); + + QCOMPARE(readCounter->destroyedConnections, 1); +} + +void tst_qqmllanguage::referenceObjectPrefersBindableConnectionToNotifyConnection() { + QQmlEngine engine; + QQmlComponent c(&engine, testFileUrl("referenceObjectPrefersBindableConnectionToNotifyConnection.qml")); + QVERIFY2(c.isReady(), qPrintable(c.errorString())); + QScopedPointer o(c.create()); + + auto *readCounter = qobject_cast(o.data()); + QVERIFY(readCounter); + + QCOMPARE(readCounter->notifyBindableSignalConnections, 0); +} + QTEST_MAIN(tst_qqmllanguage) #include "tst_qqmllanguage.moc"