DelegateModel: Use actual bindings for required properties

Tracking the change signals is brittle and error prone. We have bindings
for this case. Let's use them. We can construct a synthetic
QV4::Function that contains its own QQmlJSAotFunction. In order to pass
the property index to the function we generalize the "index" property of
QQmlJSAotFunction to contain any extra data the function may want to
use. If there is no compilation unit, we pass that instead.

Fixes: QTBUG-91649
Change-Id: I0758bcc4964a48c6818d18bfb0972e67dbc16a1f
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

13 files changed, 215 insertions, 84 deletions

diff --git a/src/qml/jsruntime/qv4executablecompilationunit.cpp b/src/qml/jsruntime/qv4executablecompilationunit.cpp
index 0a3ceee6c0..1abb24c34e 100644
--- a/src/qml/jsruntime/qv4executablecompilationunit.cpp
+++ b/src/qml/jsruntime/qv4executablecompilationunit.cpp
@@ -220,7 +220,7 @@ QV4::Function *ExecutableCompilationUnit::linkToEngine(ExecutionEngine *engine)
     auto advanceAotFunction = [&](int i) -> const QQmlPrivate::AOTCompiledFunction * {
         if (aotFunction) {
             if (aotFunction->functionPtr) {
-                if (aotFunction->index == i)
+                if (aotFunction->extraData == i)
                     return aotFunction++;
             } else {
                 aotFunction = nullptr;
diff --git a/src/qml/jsruntime/qv4function.cpp b/src/qml/jsruntime/qv4function.cpp
index 95a7506c80..a34f93be31 100644
--- a/src/qml/jsruntime/qv4function.cpp
+++ b/src/qml/jsruntime/qv4function.cpp
@@ -138,6 +138,18 @@ Function::Function(ExecutionEngine *engine, ExecutableCompilationUnit *unit,
     nFormals = compiledFunction->nFormals;
 }
 
+Function::Function(ExecutionEngine *engine, const QQmlPrivate::AOTCompiledFunction *aotFunction)
+    : FunctionData(nullptr)
+    , compiledFunction(nullptr)
+    , codeData(nullptr)
+    , jittedCode(nullptr)
+    , codeRef(nullptr)
+    , aotFunction(aotFunction)
+{
+    internalClass = engine->internalClasses(EngineBase::Class_CallContext);
+    nFormals = aotFunction->argumentTypes.length();
+}
+
 Function::~Function()
 {
     if (codeRef) {
diff --git a/src/qml/jsruntime/qv4function_p.h b/src/qml/jsruntime/qv4function_p.h
index 2c5de203b6..8f2a38416d 100644
--- a/src/qml/jsruntime/qv4function_p.h
+++ b/src/qml/jsruntime/qv4function_p.h
@@ -81,9 +81,10 @@ struct Q_QML_EXPORT FunctionData {
 Q_STATIC_ASSERT(std::is_standard_layout< FunctionData >::value);
 
 struct Q_QML_EXPORT Function : public FunctionData {
-private:
+protected:
     Function(ExecutionEngine *engine, ExecutableCompilationUnit *unit,
              const CompiledData::Function *function, const QQmlPrivate::AOTCompiledFunction *aotFunction);
+    Function(ExecutionEngine *engine, const QQmlPrivate::AOTCompiledFunction *aotFunction);
     ~Function();
 
 public:
@@ -122,8 +123,12 @@ public:
     static Function *create(ExecutionEngine *engine, ExecutableCompilationUnit *unit,
                             const CompiledData::Function *function,
                             const QQmlPrivate::AOTCompiledFunction *aotFunction);
+    static Function *create(ExecutionEngine *engine,
+                            const QQmlPrivate::AOTCompiledFunction *aotFunction);
     void destroy();
 
+    bool isSyntheticAotFunction() const { return codeData == nullptr && aotFunction != nullptr; }
+
     // used when dynamically assigning signal handlers (QQmlConnection)
     void updateInternalClass(ExecutionEngine *engine, const QList<QByteArray> &parameters);
 
@@ -151,6 +156,18 @@ public:
     }
 };
 
+struct SyntheticAotFunction : public Function
+{
+    SyntheticAotFunction(ExecutionEngine *engine, QQmlPrivate::AOTCompiledFunction aotFunction)
+        : Function(engine, &m_aotFunction)
+        , m_aotFunction(std::move(aotFunction))
+    {
+    }
+
+private:
+    QQmlPrivate::AOTCompiledFunction m_aotFunction;
+};
+
 }
 
 QT_END_NAMESPACE
diff --git a/src/qml/jsruntime/qv4vme_moth.cpp b/src/qml/jsruntime/qv4vme_moth.cpp
index c21f1b4e32..5f41b4b601 100644
--- a/src/qml/jsruntime/qv4vme_moth.cpp
+++ b/src/qml/jsruntime/qv4vme_moth.cpp
@@ -494,7 +494,11 @@ void VME::exec(MetaTypesStackFrame *frame, ExecutionEngine *engine)
     }
 
     aotContext.engine = engine->jsEngine();
-    aotContext.compilationUnit = function->executableCompilationUnit();
+    if (function->isSyntheticAotFunction())
+        aotContext.extraData = function->aotFunction->extraData;
+    else
+        aotContext.compilationUnit = function->executableCompilationUnit();
+
     function->aotFunction->functionPtr(
                 &aotContext, transformedResult ? transformedResult : frame->returnValue(),
                 transformedArguments ? transformedArguments : frame->argv());
diff --git a/src/qml/qml/qqmljavascriptexpression.cpp b/src/qml/qml/qqmljavascriptexpression.cpp
index 547894b897..f2058d7925 100644
--- a/src/qml/qml/qqmljavascriptexpression.cpp
+++ b/src/qml/qml/qqmljavascriptexpression.cpp
@@ -123,6 +123,9 @@ QQmlJavaScriptExpression::~QQmlJavaScriptExpression()
     clearError();
     if (m_scopeObject.isT2()) // notify DeleteWatcher of our deletion.
         m_scopeObject.asT2()->_s = nullptr;
+
+    if (m_v4Function.tag() == OwnsSyntheticAotFunction)
+        delete static_cast<QV4::SyntheticAotFunction *>(m_v4Function.data());
 }
 
 QString QQmlJavaScriptExpression::expressionIdentifier() const
@@ -536,6 +539,12 @@ void QQmlJavaScriptExpression::setupFunction(QV4::ExecutionContext *qmlContext,
         return;
     m_qmlScope.set(qmlContext->engine(), *qmlContext);
     m_v4Function = f;
+
+    // Synthetic AOT functions are owned by the QQmlJavaScriptExpressions they are assigned to.
+    // We need to check this here, because non-synthetic functions may be removed before the
+    // QQmlJavaScriptExpressions that use them.
+    m_v4Function.setTag(f->isSyntheticAotFunction() ? OwnsSyntheticAotFunction : DoesNotOwn);
+
     m_compilationUnit.reset(m_v4Function->executableCompilationUnit());
 }
 
diff --git a/src/qml/qml/qqmljavascriptexpression_p.h b/src/qml/qml/qqmljavascriptexpression_p.h
index ddf7aa7761..b57b3982ec 100644
--- a/src/qml/qml/qqmljavascriptexpression_p.h
+++ b/src/qml/qml/qqmljavascriptexpression_p.h
@@ -100,6 +100,7 @@ private:
 
 class Q_QML_PRIVATE_EXPORT QQmlJavaScriptExpression
 {
+    Q_DISABLE_COPY_MOVE(QQmlJavaScriptExpression)
 public:
     QQmlJavaScriptExpression();
     virtual ~QQmlJavaScriptExpression();
@@ -137,7 +138,7 @@ public:
         *listHead = this;
     }
 
-    QV4::Function *function() const { return m_v4Function; }
+    QV4::Function *function() const { return m_v4Function.data(); }
 
     virtual void refresh();
 
@@ -210,7 +211,9 @@ private:
 
     QV4::PersistentValue m_qmlScope;
     QQmlRefPointer<QV4::ExecutableCompilationUnit> m_compilationUnit;
-    QV4::Function *m_v4Function;
+
+    enum Ownership { DoesNotOwn, OwnsSyntheticAotFunction };
+    QTaggedPointer<QV4::Function, Ownership> m_v4Function;
 
 protected:
     TriggerList *qpropertyChangeTriggers = nullptr;
diff --git a/src/qml/qml/qqmlprivate.h b/src/qml/qml/qqmlprivate.h
index e4cb412c67..817f25c112 100644
--- a/src/qml/qml/qqmlprivate.h
+++ b/src/qml/qml/qqmlprivate.h
@@ -619,7 +619,10 @@ namespace QQmlPrivate
         QQmlContextData *qmlContext;
         QObject *qmlScopeObject;
         QJSEngine *engine;
-        QV4::ExecutableCompilationUnit *compilationUnit;
+        union {
+            QV4::ExecutableCompilationUnit *compilationUnit;
+            qintptr extraData;
+        };
 
         QQmlEngine *qmlEngine() const;
 
@@ -700,7 +703,7 @@ namespace QQmlPrivate
     };
 
     struct AOTCompiledFunction {
-        int index;
+        qintptr extraData;
         QMetaType returnType;
         QList<QMetaType> argumentTypes;
         void (*functionPtr)(const AOTCompiledContext *context, void *resultPtr, void **arguments);
diff --git a/src/qmlmodels/qqmldelegatemodel.cpp b/src/qmlmodels/qqmldelegatemodel.cpp
index 142d180c08..9ef78a80bd 100644
--- a/src/qmlmodels/qqmldelegatemodel.cpp
+++ b/src/qmlmodels/qqmldelegatemodel.cpp
@@ -45,6 +45,7 @@
 #include <private/qquickpackage_p.h>
 #include <private/qmetaobjectbuilder_p.h>
 #include <private/qqmladaptormodel_p.h>
+#include <private/qqmlanybinding_p.h>
 #include <private/qqmlchangeset_p.h>
 #include <private/qqmlengine_p.h>
 #include <private/qqmlcomponent_p.h>
@@ -937,44 +938,22 @@ static bool isDoneIncubating(QQmlIncubator::Status status)
      return status == QQmlIncubator::Ready || status == QQmlIncubator::Error;
 }
 
-PropertyUpdater::PropertyUpdater(QObject *parent) :
-      QObject(parent) {}
-
-void PropertyUpdater::doUpdate()
+static void bindingFunction(
+        const QQmlPrivate::AOTCompiledContext *context, void *resultPtr, void **)
 {
-    auto sender = QObject::sender();
-    auto mo = sender->metaObject();
-    auto signalIndex = QObject::senderSignalIndex();
-    ++updateCount;
-    auto property = mo->property(changeSignalIndexToPropertyIndex[signalIndex]);
-    // we synchronize between required properties and model rolenames by name
-    // that's why the QQmlProperty and the metaobject property must have the same name
-    QQmlProperty qmlProp(parent(), QString::fromLatin1(property.name()));
-    qmlProp.write(property.read(QObject::sender()));
-}
+    // metaCall expects initialized memory, the AOT function passes uninitialized memory.
+    QObject *scopeObject = context->qmlScopeObject;
+    const int propertyIndex = context->extraData;
+    const QMetaObject *metaObject = scopeObject->metaObject();
+    const QMetaProperty property = metaObject->property(propertyIndex);
+    property.metaType().construct(resultPtr);
 
-void PropertyUpdater::breakBinding()
-{
-    auto it = senderToConnection.find(QObject::senderSignalIndex());
-    if (it == senderToConnection.end())
-        return;
-    if (updateCount == 0) {
-        QObject::disconnect(*it);
-        senderToConnection.erase(it);
-        QQmlError warning;
-        if (auto context = qmlContext(QObject::sender()))
-            warning.setUrl(context->baseUrl());
-        else
-            return;
-        auto signalName = QString::fromLatin1(QObject::sender()->metaObject()->method(QObject::senderSignalIndex()).name());
-        signalName.chop(sizeof("changed")-1);
-        QString propName = signalName;
-        propName[0] = propName[0].toLower();
-        warning.setDescription(QString::fromUtf8("Writing to \"%1\" broke the binding to the underlying model").arg(propName));
-        qmlWarning(this, warning);
-    } else {
-        --updateCount;
-    }
+    context->qmlEngine()->captureProperty(scopeObject, property);
+
+    int status = -1;
+    int flags = 0;
+    void *argv[] = { resultPtr, nullptr, &status, &flags };
+    metaObject->metacall(scopeObject, QMetaObject::ReadProperty, propertyIndex, argv);
 }
 
 void QQDMIncubationTask::initializeRequiredProperties(QQmlDelegateModelItem *modelItemToIncubate, QObject *object)
@@ -1022,41 +1001,39 @@ void QQDMIncubationTask::initializeRequiredProperties(QQmlDelegateModelItem *mod
         if (proxiedObject)
             mos.push_back(qMakePair(proxiedObject->metaObject(), proxiedObject));
 
-        auto updater = new PropertyUpdater(object);
+        QQmlEngine *engine = QQmlEnginePrivate::get(incubatorPriv->enginePriv);
+        QV4::ExecutionEngine *v4 = engine->handle();
+        QV4::Scope scope(v4);
+
         for (const auto &metaObjectAndObject : mos) {
             const QMetaObject *mo = metaObjectAndObject.first;
             QObject *itemOrProxy = metaObjectAndObject.second;
+            QV4::Scoped<QV4::QmlContext> qmlContext(scope);
+
             for (int i = mo->propertyOffset(); i < mo->propertyCount() + mo->propertyOffset(); ++i) {
                 auto prop = mo->property(i);
                 if (!prop.name())
                     continue;
-                auto propName = QString::fromUtf8(prop.name());
+                const QString propName = QString::fromUtf8(prop.name());
                 bool wasInRequired = false;
-                QQmlProperty componentProp = QQmlComponentPrivate::removePropertyFromRequired(
+                QQmlProperty targetProp = QQmlComponentPrivate::removePropertyFromRequired(
                             object, propName, requiredProperties,
-                            QQmlEnginePrivate::get(incubatorPriv->enginePriv), &wasInRequired);
-                // only write to property if it was actually requested by the component
-                if (wasInRequired && prop.hasNotifySignal()) {
-                    QMetaMethod changeSignal = prop.notifySignal();
-                    static QMetaMethod updateSlot = PropertyUpdater::staticMetaObject.method(
-                                PropertyUpdater::staticMetaObject.indexOfSlot("doUpdate()"));
-
-                    QMetaObject::Connection conn = QObject::connect(itemOrProxy, changeSignal,
-                                                                    updater, updateSlot);
-                    updater->changeSignalIndexToPropertyIndex[changeSignal.methodIndex()] = i;
-                    auto propIdx = object->metaObject()->indexOfProperty(propName.toUtf8());
-                    QMetaMethod writeToPropSignal
-                            = object->metaObject()->property(propIdx).notifySignal();
-                    updater->senderToConnection[writeToPropSignal.methodIndex()] = conn;
-                    static QMetaMethod breakBinding = PropertyUpdater::staticMetaObject.method(
-                                PropertyUpdater::staticMetaObject.indexOfSlot("breakBinding()"));
-                    componentProp.write(prop.read(itemOrProxy));
-                    // the connection needs to established after the write,
-                    // else the signal gets triggered by it and breakBinding will remove the connection
-                    QObject::connect(object, writeToPropSignal, updater, breakBinding);
+                            engine, &wasInRequired);
+                if (wasInRequired) {
+                    QV4::SyntheticAotFunction *function = new QV4::SyntheticAotFunction(
+                            engine->handle(), QQmlPrivate::AOTCompiledFunction {
+                                i, prop.metaType(), {}, bindingFunction
+                            });
+
+                    if (!qmlContext) {
+                        qmlContext = QV4::QmlContext::create(
+                                v4->rootContext(), contextData, itemOrProxy);
+                    }
+
+                    QQmlAnyBinding binding = QQmlAnyBinding::createFromFunction(
+                                targetProp, function, itemOrProxy, contextData, qmlContext);
+                    binding.installOn(targetProp);
                 }
-                else if (wasInRequired) // we still have to write, even if there is no change signal
-                    componentProp.write(prop.read(itemOrProxy));
             }
         }
     } else {
diff --git a/src/qmlmodels/qqmldelegatemodel_p_p.h b/src/qmlmodels/qqmldelegatemodel_p_p.h
index a1c9e7f583..e99a514c15 100644
--- a/src/qmlmodels/qqmldelegatemodel_p_p.h
+++ b/src/qmlmodels/qqmldelegatemodel_p_p.h
@@ -468,20 +468,6 @@ private:
     const int indexPropertyOffset;
 };
 
-class PropertyUpdater : public QObject
-{
-    Q_OBJECT
-
-public:
-    PropertyUpdater(QObject *parent);
-    QHash<int, QMetaObject::Connection> senderToConnection;
-    QHash<int, int> changeSignalIndexToPropertyIndex;
-    int updateCount = 0;
-public Q_SLOTS:
-    void doUpdate();
-    void breakBinding();
-};
-
 QT_END_NAMESPACE
 
 #endif
diff --git a/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp b/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp
index 0a2cbe1478..d9b51fb67c 100644
--- a/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp
+++ b/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp
@@ -793,7 +793,7 @@ void tst_QmlCppCodegen::failures()
             = QmlCacheGeneratedCode::_0x5f_TestTypes_failures_qml::aotBuiltFunctions[0];
     QVERIFY(aotFailure.argumentTypes.isEmpty());
     QVERIFY(!aotFailure.functionPtr);
-    QCOMPARE(aotFailure.index, 0);
+    QCOMPARE(aotFailure.extraData, 0);
 }
 
 void tst_QmlCppCodegen::enumScope()
diff --git a/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp b/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp
index 2a0e1e5b6d..93899d3cfd 100644
--- a/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp
+++ b/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp
@@ -2784,7 +2784,6 @@ void tst_QQuickPathView::requiredPropertiesInDelegate()
     }
     {
         QScopedPointer<QQuickView> window(createView());
-        QTest::ignoreMessage(QtMsgType::QtWarningMsg, QRegularExpression("Writing to \"name\" broke the binding to the underlying model"));
         window->setSource(testFileUrl("delegateWithRequiredProperties.3.qml"));
         window->show();
         QTRY_VERIFY(window->rootObject()->property("working").toBool());
diff --git a/tests/auto/quick/qquickvisualdatamodel/data/objectDeletion.qml b/tests/auto/quick/qquickvisualdatamodel/data/objectDeletion.qml
new file mode 100644
index 0000000000..4e01b0a1ba
--- /dev/null
+++ b/tests/auto/quick/qquickvisualdatamodel/data/objectDeletion.qml
@@ -0,0 +1,20 @@
+import QtQuick
+import TestTypes
+
+ListView {
+    id: listView
+    anchors.fill: parent
+    model: InventoryModel {}
+    delegate: Text {
+        width: listView.width
+        text: itemName
+
+        required property int index
+        required property string itemName
+        required property ComponentEntity entity
+    }
+
+    function removeLast() { model.removeLast() }
+}
+
+
diff --git a/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp b/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp
index e9f867da8d..0c0bb00106 100644
--- a/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp
+++ b/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp
@@ -437,6 +437,7 @@ private slots:
     void delegateModelChangeDelegate();
     void checkFilterGroupForDelegate();
     void readFromProxyObject();
+    void noWarningOnObjectDeletion();
 
 private:
     template <int N> void groups_verify(
@@ -4391,6 +4392,106 @@ void tst_qquickvisualdatamodel::readFromProxyObject()
     QTRY_VERIFY(window->property("name").toString() != QLatin1String("wrong"));
 }
 
+
+class ComponentEntity : public QObject
+{
+    Q_OBJECT
+
+public:
+    ComponentEntity(QObject *parent = nullptr) : QObject(parent)
+    {
+        QQmlEngine::setObjectOwnership(this, QQmlEngine::CppOwnership);
+    }
+};
+
+class InventoryModel : public QAbstractListModel
+{
+    Q_OBJECT
+
+public:
+    InventoryModel() {
+        for (int i = 0; i < 10; ++i) {
+            QSharedPointer<ComponentEntity> entity(new ComponentEntity());
+            entity->setObjectName(QString::fromLatin1("Item %1").arg(i));
+            mContents.append(entity);
+        }
+    }
+
+    int rowCount(const QModelIndex &) const override { return mContents.size(); }
+
+    QVariant data(const QModelIndex &index, int role) const override
+    {
+        if (!checkIndex(index, CheckIndexOption::IndexIsValid))
+            return {};
+
+        auto entity = mContents.at(index.row()).data();
+        switch (role) {
+        case ItemNameRole: return entity->objectName();
+        case EntityRole: return QVariant::fromValue(entity);
+        }
+
+        return {};
+    }
+
+    Q_INVOKABLE void removeLast() {
+        const int index = rowCount(QModelIndex()) - 1;
+        if (index < 0)
+            return;
+
+        const auto item = mContents.at(index);
+        beginRemoveRows(QModelIndex(), index, index);
+        mContents.takeLast();
+        endRemoveRows();
+    }
+
+    enum InventoryModelRoles {
+        ItemNameRole = Qt::UserRole,
+        EntityRole
+    };
+
+    virtual QHash<int, QByteArray> roleNames() const override {
+        QHash<int, QByteArray> names;
+        names.insert(ItemNameRole, "itemName");
+        names.insert(EntityRole, "entity");
+        return names;
+    }
+
+private:
+    QVector<QSharedPointer<ComponentEntity>> mContents;
+};
+
+
+static QString lastWarning;
+static QtMessageHandler oldHandler;
+static void warningsHandler(QtMsgType type, const QMessageLogContext &ctxt, const QString &msg)
+{
+    if (type == QtWarningMsg)
+        lastWarning = msg;
+    else
+        oldHandler(type, ctxt, msg);
+}
+
+void tst_qquickvisualdatamodel::noWarningOnObjectDeletion()
+{
+    qmlRegisterType<InventoryModel>("TestTypes", 1, 0, "InventoryModel");
+    qmlRegisterUncreatableType<ComponentEntity>("TestTypes", 1, 0, "ComponentEntity", "no");
+
+    oldHandler = qInstallMessageHandler(warningsHandler);
+    const auto guard = qScopeGuard([&]() { qInstallMessageHandler(oldHandler); });
+
+    {
+        QQmlEngine engine;
+        QQmlComponent component(&engine, testFileUrl("objectDeletion.qml"));
+        QVERIFY2(component.isReady(), qPrintable(component.errorString()));
+        QScopedPointer<QObject> o(component.create());
+        QVERIFY(!o.isNull());
+        for (int i = 0; i < 5; ++i)
+            o->metaObject()->invokeMethod(o.data(), "removeLast");
+    }
+
+    QVERIFY2(lastWarning.isEmpty(), qPrintable(lastWarning));
+}
+
 QTEST_MAIN(tst_qquickvisualdatamodel)
 
 #include "tst_qquickvisualdatamodel.moc"