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>

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;

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) {

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

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

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

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;

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

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) :
+static void bindingFunction(
-      QObject(parent) {}
+        const QQmlPrivate::AOTCompiledContext *context, void *resultPtr, void **)
-
-void PropertyUpdater::doUpdate()
 {
-    auto sender = QObject::sender();
+    // metaCall expects initialized memory, the AOT function passes uninitialized memory.
-    auto mo = sender->metaObject();
+    QObject *scopeObject = context->qmlScopeObject;
-    auto signalIndex = QObject::senderSignalIndex();
+    const int propertyIndex = context->extraData;
-    ++updateCount;
+    const QMetaObject *metaObject = scopeObject->metaObject();
-    auto property = mo->property(changeSignalIndexToPropertyIndex[signalIndex]);
+    const QMetaProperty property = metaObject->property(propertyIndex);
-    // we synchronize between required properties and model rolenames by name
+    property.metaType().construct(resultPtr);
-    // 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()));
-}
 
-void PropertyUpdater::breakBinding()
+    context->qmlEngine()->captureProperty(scopeObject, property);
-{
+
-    auto it = senderToConnection.find(QObject::senderSignalIndex());
+    int status = -1;
-    if (it == senderToConnection.end())
+    int flags = 0;
-        return;
+    void *argv[] = { resultPtr, nullptr, &status, &flags };
-    if (updateCount == 0) {
+    metaObject->metacall(scopeObject, QMetaObject::ReadProperty, propertyIndex, argv);
-        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;
-    }
 }
 
 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);
+                            engine, &wasInRequired);
-                // only write to property if it was actually requested by the component
+                if (wasInRequired) {
-                if (wasInRequired && prop.hasNotifySignal()) {
+                    QV4::SyntheticAotFunction *function = new QV4::SyntheticAotFunction(
-                    QMetaMethod changeSignal = prop.notifySignal();
+                            engine->handle(), QQmlPrivate::AOTCompiledFunction {
-                    static QMetaMethod updateSlot = PropertyUpdater::staticMetaObject.method(
+                                i, prop.metaType(), {}, bindingFunction
-                                PropertyUpdater::staticMetaObject.indexOfSlot("doUpdate()"));
+                            });
 
-                    QMetaObject::Connection conn = QObject::connect(itemOrProxy, changeSignal,
+                    if (!qmlContext) {
-                                                                    updater, updateSlot);
+                        qmlContext = QV4::QmlContext::create(
-                    updater->changeSignalIndexToPropertyIndex[changeSignal.methodIndex()] = i;
+                                v4->rootContext(), contextData, itemOrProxy);
-                    auto propIdx = object->metaObject()->indexOfProperty(propName.toUtf8());
+                    }
-                    QMetaMethod writeToPropSignal
+
-                            = object->metaObject()->property(propIdx).notifySignal();
+                    QQmlAnyBinding binding = QQmlAnyBinding::createFromFunction(
-                    updater->senderToConnection[writeToPropSignal.methodIndex()] = conn;
+                                targetProp, function, itemOrProxy, contextData, qmlContext);
-                    static QMetaMethod breakBinding = PropertyUpdater::staticMetaObject.method(
+                    binding.installOn(targetProp);
-                                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);
                 }
-                else if (wasInRequired) // we still have to write, even if there is no change signal
-                    componentProp.write(prop.read(itemOrProxy));
             }
         }
     } else {

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

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()

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

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() }
+}
+
+

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"