Qml: Allow const and non-const QObjectWrappers to coexist

We can access the same QObject in const and non-const contexts. Both
should be possible. Store the const objectwrapper in
m_multiplyWrappedObjects. That's somewhat slow, but const QObjects are
rather rare.

Pick-to: 6.4
Fixes: QTBUG-98479
Change-Id: I047afc121f5c29b955cd833e0a2c8299fc52b021
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/qml/jsruntime/qv4engine.cpp

@@ -1855,16 +1855,10 @@ QV4::ReturnedValue ExecutionEngine::fromData(
             a->setArrayLengthUnchecked(list.count());
             return a.asReturnedValue();
         } else if (auto flags = metaType.flags(); flags & QMetaType::PointerToQObject) {
-            QV4::ReturnedValue ret = QV4::QObjectWrapper::wrap(this, *reinterpret_cast<QObject* const *>(ptr));
-            if (!flags.testFlag(QMetaType::IsConst))
-                return ret;
-            QV4::ScopedValue v(scope, ret);
-            if (auto obj = v->as<Object>()) {
-                obj->setInternalClass(obj->internalClass()->cryopreserved());
-                return obj->asReturnedValue();
-            } else {
-                return ret;
-            }
+            if (flags.testFlag(QMetaType::IsConst))
+                return QV4::QObjectWrapper::wrapConst(this, *reinterpret_cast<QObject* const *>(ptr));
+            else
+                return QV4::QObjectWrapper::wrap(this, *reinterpret_cast<QObject* const *>(ptr));
         }
 
         bool succeeded = false;

src/qml/jsruntime/qv4qobjectwrapper.cpp

@@ -100,15 +100,10 @@ static ReturnedValue loadProperty(ExecutionEngine *v4, QObject *object,
     if (property.isQObject()) {
         QObject *rv = nullptr;
         property.readProperty(object, &rv);
-        ReturnedValue ret = QObjectWrapper::wrap(v4, rv);
-        if (propMetaType.flags().testFlag(QMetaType::IsConst)) {
-            ScopedValue v(scope, ret);
-            if (auto obj = v->as<Object>()) {
-                obj->setInternalClass(obj->internalClass()->cryopreserved());
-                return obj->asReturnedValue();
-            }
-        }
-        return ret;
+        if (propMetaType.flags().testFlag(QMetaType::IsConst))
+            return QObjectWrapper::wrapConst(v4, rv);
+        else
+            return QObjectWrapper::wrap(v4, rv);
     }
 
     if (property.isQList() && propMetaType.flags().testFlag(QMetaType::IsQmlList))
@@ -670,6 +665,29 @@ ReturnedValue QObjectWrapper::wrap_slowPath(ExecutionEngine *engine, QObject *ob
     }
 }
 
+ReturnedValue QObjectWrapper::wrapConst_slowPath(ExecutionEngine *engine, QObject *object)
+{
+    const QObject *constObject = object;
+
+    QQmlData *ddata = QQmlData::get(object, true);
+
+    Scope scope(engine);
+    ScopedObject constWrapper(scope);
+    if (engine->m_multiplyWrappedQObjects && ddata->hasConstWrapper)
+        constWrapper = engine->m_multiplyWrappedQObjects->value(constObject);
+
+    if (!constWrapper) {
+        constWrapper = create(engine, object);
+        constWrapper->setInternalClass(constWrapper->internalClass()->cryopreserved());
+        if (!engine->m_multiplyWrappedQObjects)
+            engine->m_multiplyWrappedQObjects = new MultiplyWrappedQObjectMap;
+        engine->m_multiplyWrappedQObjects->insert(constObject, constWrapper->d());
+        ddata->hasConstWrapper = true;
+    }
+
+    return constWrapper.asReturnedValue();
+}
+
 void QObjectWrapper::markWrapper(QObject *object, MarkStack *markStack)
 {
     if (QQmlData::wasDeleted(object))
@@ -684,6 +702,8 @@ void QObjectWrapper::markWrapper(QObject *object, MarkStack *markStack)
         ddata->jsWrapper.markOnce(markStack);
     else if (engine->m_multiplyWrappedQObjects && ddata->hasTaintedV4Object)
         engine->m_multiplyWrappedQObjects->mark(object, markStack);
+    if (ddata->hasConstWrapper)
+        engine->m_multiplyWrappedQObjects->mark(static_cast<const QObject *>(object), markStack);
 }
 
 void QObjectWrapper::setProperty(ExecutionEngine *engine, int propertyIndex, const Value &value)
@@ -711,14 +731,13 @@ void QObjectWrapper::setProperty(ExecutionEngine *engine, QObject *object, int p
 bool QObjectWrapper::virtualIsEqualTo(Managed *a, Managed *b)
 {
     Q_ASSERT(a->as<QObjectWrapper>());
-    QObjectWrapper *qobjectWrapper = static_cast<QObjectWrapper *>(a);
-    Object *o = b->as<Object>();
-    if (o) {
-        if (QQmlTypeWrapper *qmlTypeWrapper = o->as<QQmlTypeWrapper>())
-            return qmlTypeWrapper->toVariant().value<QObject*>() == qobjectWrapper->object();
-    }
+    const QObjectWrapper *aobjectWrapper = static_cast<QObjectWrapper *>(a);
+    if (const QQmlTypeWrapper *qmlTypeWrapper = b->as<QQmlTypeWrapper>())
+        return qmlTypeWrapper->object() == aobjectWrapper->object();
 
-    return false;
+    // We can have a const and a non-const wrapper for the same object.
+    const QObjectWrapper *bobjectWrapper = b->as<QObjectWrapper>();
+    return bobjectWrapper && aobjectWrapper->object() == bobjectWrapper->object();
 }
 
 ReturnedValue QObjectWrapper::create(ExecutionEngine *engine, QObject *object)
@@ -2463,39 +2482,20 @@ void QmlSignalHandler::initProto(ExecutionEngine *engine)
     engine->jsObjects[ExecutionEngine::SignalHandlerProto] = o->d();
 }
 
-void MultiplyWrappedQObjectMap::insert(QObject *key, Heap::Object *value)
+
+MultiplyWrappedQObjectMap::Iterator MultiplyWrappedQObjectMap::erase(
+        MultiplyWrappedQObjectMap::Iterator it)
 {
-    QHash<QObject*, WeakValue>::operator[](key).set(value->internalClass->engine, value);
-    connect(key, SIGNAL(destroyed(QObject*)), this, SLOT(removeDestroyedObject(QObject*)));
-}
-
-
-
-MultiplyWrappedQObjectMap::Iterator MultiplyWrappedQObjectMap::erase(MultiplyWrappedQObjectMap::Iterator it)
-{
-    disconnect(it.key(), SIGNAL(destroyed(QObject*)), this, SLOT(removeDestroyedObject(QObject*)));
-    return QHash<QObject*, WeakValue>::erase(it);
-}
-
-void MultiplyWrappedQObjectMap::remove(QObject *key)
-{
-    Iterator it = find(key);
-    if (it == end())
-        return;
-    erase(it);
-}
-
-void MultiplyWrappedQObjectMap::mark(QObject *key, MarkStack *markStack)
-{
-    Iterator it = find(key);
-    if (it == end())
-        return;
-    it->markOnce(markStack);
+    const QObjectBiPointer key = it.key();
+    const QObject *obj = key.isT1() ? key.asT1() : key.asT2();
+    disconnect(obj, &QObject::destroyed, this, &MultiplyWrappedQObjectMap::removeDestroyedObject);
+    return QHash<QObjectBiPointer, WeakValue>::erase(it);
 }
 
 void MultiplyWrappedQObjectMap::removeDestroyedObject(QObject *object)
 {
-    QHash<QObject*, WeakValue>::remove(object);
+    QHash<QObjectBiPointer, WeakValue>::remove(object);
+    QHash<QObjectBiPointer, WeakValue>::remove(static_cast<const QObject *>(object));
 }
 
 } // namespace QV4

src/qml/jsruntime/qv4qobjectwrapper_p.h

@@ -140,6 +140,7 @@ struct Q_QML_EXPORT QObjectWrapper : public Object
             QObject *object, String *name, RevisionMode revisionMode, const Value &value);
 
     static ReturnedValue wrap(ExecutionEngine *engine, QObject *object);
+    static ReturnedValue wrapConst(ExecutionEngine *engine, QObject *object);
     static void markWrapper(QObject *object, MarkStack *markStack);
 
     using Object::get;
@@ -181,6 +182,7 @@ protected:
 
 private:
     Q_NEVER_INLINE static ReturnedValue wrap_slowPath(ExecutionEngine *engine, QObject *object);
+    Q_NEVER_INLINE static ReturnedValue wrapConst_slowPath(ExecutionEngine *engine, QObject *object);
 };
 
 inline ReturnedValue QObjectWrapper::wrap(ExecutionEngine *engine, QObject *object)
@@ -197,6 +199,15 @@ inline ReturnedValue QObjectWrapper::wrap(ExecutionEngine *engine, QObject *obje
     return wrap_slowPath(engine, object);
 }
 
+// Unfortunately we still need a non-const QObject* here because QQmlData needs to register itself in QObjectPrivate.
+inline ReturnedValue QObjectWrapper::wrapConst(ExecutionEngine *engine, QObject *object)
+{
+    if (Q_UNLIKELY(QQmlData::wasDeleted(object)))
+        return QV4::Encode::null();
+
+    return wrapConst_slowPath(engine, object);
+}
+
 template <typename ReversalFunctor>
 inline ReturnedValue QObjectWrapper::lookupGetterImpl(Lookup *lookup, ExecutionEngine *engine, const Value &object, bool useOriginalProperty, ReversalFunctor revertLookup)
 {
@@ -292,23 +303,32 @@ struct Q_QML_EXPORT QmlSignalHandler : public QV4::Object
     static void initProto(ExecutionEngine *v4);
 };
 
+using QObjectBiPointer = QBiPointer<QObject, const QObject>;
+
 class MultiplyWrappedQObjectMap : public QObject,
-                                  private QHash<QObject*, QV4::WeakValue>
+                                  private QHash<QObjectBiPointer, QV4::WeakValue>
 {
     Q_OBJECT
 public:
-    typedef QHash<QObject*, QV4::WeakValue>::ConstIterator ConstIterator;
-    typedef QHash<QObject*, QV4::WeakValue>::Iterator Iterator;
+    typedef QHash<QObjectBiPointer, QV4::WeakValue>::ConstIterator ConstIterator;
+    typedef QHash<QObjectBiPointer, QV4::WeakValue>::Iterator Iterator;
 
-    using value_type = QHash<QObject*, QV4::WeakValue>::value_type;
+    using value_type = QHash<QObjectBiPointer, QV4::WeakValue>::value_type;
 
-    ConstIterator begin() const { return QHash<QObject*, QV4::WeakValue>::constBegin(); }
-    Iterator begin() { return QHash<QObject*, QV4::WeakValue>::begin(); }
-    ConstIterator end() const { return QHash<QObject*, QV4::WeakValue>::constEnd(); }
-    Iterator end() { return QHash<QObject*, QV4::WeakValue>::end(); }
+    ConstIterator begin() const { return QHash<QObjectBiPointer, QV4::WeakValue>::constBegin(); }
+    Iterator begin() { return QHash<QObjectBiPointer, QV4::WeakValue>::begin(); }
+    ConstIterator end() const { return QHash<QObjectBiPointer, QV4::WeakValue>::constEnd(); }
+    Iterator end() { return QHash<QObjectBiPointer, QV4::WeakValue>::end(); }
 
-    void insert(QObject *key, Heap::Object *value);
-    ReturnedValue value(QObject *key) const
+    template<typename Pointer>
+    void insert(Pointer key, Heap::Object *value)
+    {
+        QHash<QObjectBiPointer, WeakValue>::operator[](key).set(value->internalClass->engine, value);
+        connect(key, SIGNAL(destroyed(QObject*)), this, SLOT(removeDestroyedObject(QObject*)));
+    }
+
+    template<typename Pointer>
+    ReturnedValue value(Pointer key) const
     {
         ConstIterator it = find(key);
         return it == end()
@@ -317,8 +337,24 @@ public:
     }
 
     Iterator erase(Iterator it);
-    void remove(QObject *key);
-    void mark(QObject *key, MarkStack *markStack);
+
+    template<typename Pointer>
+    void remove(Pointer key)
+    {
+        Iterator it = find(key);
+        if (it == end())
+            return;
+        erase(it);
+    }
+
+    template<typename Pointer>
+    void mark(Pointer key, MarkStack *markStack)
+    {
+        Iterator it = find(key);
+        if (it == end())
+            return;
+        it->markOnce(markStack);
+    }
 
 private Q_SLOTS:
     void removeDestroyedObject(QObject*);

src/qml/qml/ftw/qbipointer_p.h

@@ -87,6 +87,11 @@ public:
     inline T *asT1() const;
     inline T2 *asT2() const;
 
+    friend size_t qHash(const QBiPointer<T, T2> &ptr, size_t seed = 0)
+    {
+        return qHash(ptr.isNull() ? quintptr(0) : ptr.ptr_value, seed);
+    }
+
 private:
     quintptr ptr_value = 0;
 

src/qml/qml/qqmldata_p.h

@@ -103,7 +103,9 @@ public:
     // set when at least one of the object's properties is intercepted
     quint32 hasInterceptorMetaObject:1;
     quint32 hasVMEMetaObject:1;
-    quint32 dummy:8;
+    // If we have another wrapper for a const QObject * in the multiply wrapped QObjects.
+    quint32 hasConstWrapper: 1;
+    quint32 dummy:7;
 
     // When bindingBitsSize < sizeof(ptr), we store the binding bit flags inside
     // bindingBitsValue. When we need more than sizeof(ptr) bits, we allocated

src/qml/qml/qqmlengine.cpp

@@ -245,7 +245,7 @@ void QQmlPrivate::qdeclarativeelement_destructor(QObject *o)
 QQmlData::QQmlData()
     : ownMemory(true), indestructible(true), explicitIndestructibleSet(false),
       hasTaintedV4Object(false), isQueuedForDeletion(false), rootObjectInCreation(false),
-      hasInterceptorMetaObject(false), hasVMEMetaObject(false),
+      hasInterceptorMetaObject(false), hasVMEMetaObject(false), hasConstWrapper(false),
       bindingBitsArraySize(InlineBindingArraySize), notifyList(nullptr),
       bindings(nullptr), signalHandlers(nullptr), nextContextObject(nullptr), prevContextObject(nullptr),
       lineNumber(0), columnNumber(0), jsEngineId(0),

tests/auto/qml/qqmlecmascript/data/frozenQObject2.qml

@@ -0,0 +1,19 @@
+import QtQml
+import test
+
+FrozenObjects {
+    id: app;
+
+    property bool caughtSignal: false
+    onFooMember2Emitted: caughtSignal = true
+
+    Component.onCompleted: {
+        app.fooMember.name = "John";
+        app.fooMemberConst;
+        app.fooMember.name = "Jane";
+
+        app.fooMember2.name = "John";
+        app.triggerSignal();
+        app.fooMember2.name = "Jane";
+    }
+}

tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp

@@ -9945,14 +9945,65 @@ void tst_qqmlecmascript::cmpInThrows()
     QCOMPARE(stacktrace.at(0), QStringLiteral("%entry:14:-1:file:foo.js"));
 }
 
+class FrozenFoo : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(QString name MEMBER m_name NOTIFY nameChanged)
+
+public:
+    FrozenFoo(QObject *parent = nullptr) : QObject(parent) {}
+    QString name() const { return m_name; }
+
+signals:
+    void nameChanged();
+
+private:
+    QString m_name{ "Foo" };
+};
+
+class FrozenObjects : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(FrozenFoo *fooMember READ fooMember CONSTANT);
+    Q_PROPERTY(const FrozenFoo *fooMemberConst READ fooMemberConst CONSTANT);
+    Q_PROPERTY(FrozenFoo *fooMember2 READ fooMember2 CONSTANT);
+
+public:
+    FrozenObjects(QObject *parent = nullptr) : QObject(parent) {}
+
+    Q_INVOKABLE void triggerSignal() { emit fooMember2Emitted(&m_fooMember2); }
+
+    FrozenFoo *fooMember() { return &m_fooMember; }
+    FrozenFoo *fooMember2() { return &m_fooMember2; }
+
+signals:
+    void fooMember2Emitted(const FrozenFoo *fooMember2);
+
+private:
+    const FrozenFoo *fooMemberConst() const { return &m_fooMember; }
+
+    FrozenFoo m_fooMember;
+    FrozenFoo m_fooMember2;
+};
+
 void tst_qqmlecmascript::frozenQObject()
 {
+    qmlRegisterType<FrozenObjects>("test", 1, 0, "FrozenObjects");
+
     QQmlEngine engine;
-    QQmlComponent component(&engine, testFileUrl("frozenQObject.qml"));
-    QScopedPointer<QObject> root(component.create());
-    QVERIFY2(root, qPrintable(component.errorString()));
-    QVERIFY(root->property("caughtException").toBool());
-    QVERIFY(root->property("nameCorrect").toBool());
+    QQmlComponent component1(&engine, testFileUrl("frozenQObject.qml"));
+    QScopedPointer<QObject> root1(component1.create());
+    QVERIFY2(root1, qPrintable(component1.errorString()));
+    QVERIFY(root1->property("caughtException").toBool());
+    QVERIFY(root1->property("nameCorrect").toBool());
+
+    QQmlComponent component2(&engine, testFileUrl("frozenQObject2.qml"));
+    QScopedPointer<QObject> root2(component2.create());
+    FrozenObjects *frozenObjects = qobject_cast<FrozenObjects *>(root2.data());
+    QVERIFY2(frozenObjects, qPrintable(component2.errorString()));
+    QVERIFY(frozenObjects->property("caughtSignal").toBool());
+    QCOMPARE(frozenObjects->fooMember()->name(), QStringLiteral("Jane"));
+    QCOMPARE(frozenObjects->fooMember2()->name(), QStringLiteral("Jane"));
 }
 
 struct ConstPointer : QObject