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 <[email protected]>

1 files changed, 13 insertions, 9 deletions

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<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));
         }