qiurui
/
qtdeclarative
mirror of https://github.com/qt/qtdeclarative.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 <fabian.kosmale@qt.io>
This commit is contained in:
Luca Di Sera 2025-02-21 14:12:04 +01:00
parent 7044837294
commit 7d96f72633
6 changed files with 81 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/qml/jsruntime/qv4referenceobject.cpp
View File

@ -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.

22
src/qml/jsruntime/qv4referenceobject_p.h
View File

@ -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<QV4::Heap::QObjectWrapper*>(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<QV4::QQmlTypeWrapper> 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<QMetaObject::Connection*>(&onDelete));
std::destroy_at(reinterpret_cast<QMetaObject::Connection*>(&onDelete));
}

8
tests/auto/qml/qqmllanguage/data/referenceObjectDoesNotLeakAConnectionToTheDestroyedSignalOnANotifyBindable.qml Normal file
View File

@ -0,0 +1,8 @@
import Test
import QtQml
ReadCounter {
Component.onCompleted: {
notifyBindable
}
}

8
tests/auto/qml/qqmllanguage/data/referenceObjectPrefersBindableConnectionToNotifyConnection.qml Normal file
View File

@ -0,0 +1,8 @@
import Test
import QtQml
ReadCounter {
Component.onCompleted: {
notifyBindable
}
}

22
tests/auto/qml/qqmllanguage/testtypes.h
View File

@ -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<QStringList> notifyBindableProperty() { return QBindable<QStringList>(&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<QStringList> m_bindable;
ReadCounterInner m_inner{};
QProperty<QStringList> m_notifyBindable;
};
#endif // TESTTYPES_H

26
tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp
View File

@ -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<QObject> o(c.create());
auto *readCounter = qobject_cast<ReadCounter *>(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<QObject> o(c.create());
auto *readCounter = qobject_cast<ReadCounter *>(o.data());
QVERIFY(readCounter);
QCOMPARE(readCounter->notifyBindableSignalConnections, 0);
}
QTEST_MAIN(tst_qqmllanguage)
#include "tst_qqmllanguage.moc"