Get rid of global gadgetPtr in QQmlValueType

We should not keep user-created objects in global data structures. This
is inherently thread-unsafe and crashes when the user passes static data
and later unloads the same.

Instead we keep the cached gadgetPtr wrapper objects in the engine now.

Fixes: QTBUG-79553
Change-Id: I24ac3e84b572831d1d70b61b8a6001338579e284
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

7 files changed, 150 insertions, 69 deletions

diff --git a/src/qml/qml/qqmlengine_p.h b/src/qml/qml/qqmlengine_p.h
index 385ae02ce5..502e2ed3d9 100644
--- a/src/qml/qml/qqmlengine_p.h
+++ b/src/qml/qml/qqmlengine_p.h
@@ -263,8 +263,24 @@ public:
 
     mutable QMutex networkAccessManagerMutex;
 
+    QQmlGadgetPtrWrapper *valueTypeInstance(int typeIndex)
+    {
+        auto it = cachedValueTypeInstances.find(typeIndex);
+        if (it != cachedValueTypeInstances.end())
+            return *it;
+
+        if (QQmlValueType *valueType = QQmlValueTypeFactory::valueType(typeIndex)) {
+            QQmlGadgetPtrWrapper *instance = new QQmlGadgetPtrWrapper(valueType, q_func());
+            cachedValueTypeInstances.insert(typeIndex, instance);
+            return instance;
+        }
+
+        return nullptr;
+    }
+
 private:
     QHash<QQmlType, QJSValue> singletonInstances;
+    QHash<int, QQmlGadgetPtrWrapper *> cachedValueTypeInstances;
 
     // These members must be protected by a QQmlEnginePrivate::Locker as they are required by
     // the threaded loader.  Only access them through their respective accessor methods.
diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index a4270628e8..836d27f245 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -871,12 +871,12 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *bindingProper
         if (stringAt(obj->inheritedTypeNameIndex).isEmpty()) {
 
             QObject *groupObject = nullptr;
-            QQmlValueType *valueType = nullptr;
+            QQmlGadgetPtrWrapper *valueType = nullptr;
             const QQmlPropertyData *valueTypeProperty = nullptr;
             QObject *bindingTarget = _bindingTarget;
 
             if (QQmlValueTypeFactory::isValueType(bindingProperty->propType())) {
-                valueType = QQmlValueTypeFactory::valueType(bindingProperty->propType());
+                valueType = QQmlGadgetPtrWrapper::instance(engine, bindingProperty->propType());
                 if (!valueType) {
                     recordError(binding->location, tr("Cannot set properties on %1 as it is null").arg(stringAt(binding->propertyNameIndex)));
                     return false;
diff --git a/src/qml/qml/qqmlproperty.cpp b/src/qml/qml/qqmlproperty.cpp
index f5aae8f462..d191d07258 100644
--- a/src/qml/qml/qqmlproperty.cpp
+++ b/src/qml/qml/qqmlproperty.cpp
@@ -1044,13 +1044,19 @@ QVariant QQmlProperty::read(const QObject *object, const QString &name, QQmlEngi
 
 QVariant QQmlPropertyPrivate::readValueProperty()
 {
-    if (isValueType()) {
-
-        QQmlValueType *valueType = QQmlValueTypeFactory::valueType(core.propType());
-        Q_ASSERT(valueType);
-        valueType->read(object, core.coreIndex());
-        return valueType->metaObject()->property(valueTypeData.coreIndex()).read(valueType);
+    auto doRead = [&](QQmlGadgetPtrWrapper *wrapper) {
+        wrapper->read(object, core.coreIndex());
+        return wrapper->property(valueTypeData.coreIndex()).read(wrapper);
+    };
 
+    if (isValueType()) {
+        if (QQmlGadgetPtrWrapper *wrapper = QQmlGadgetPtrWrapper::instance(engine, core.propType()))
+            return doRead(wrapper);
+        if (QQmlValueType *valueType = QQmlValueTypeFactory::valueType(core.propType())) {
+            QQmlGadgetPtrWrapper wrapper(valueType, nullptr);
+            return doRead(&wrapper);
+        }
+        return QVariant();
     } else if (core.isQList()) {
 
         QQmlListProperty<QObject> prop;
@@ -1183,10 +1189,22 @@ QQmlPropertyPrivate::writeValueProperty(QObject *object,
 
     bool rv = false;
     if (valueTypeData.isValid()) {
-        QQmlValueType *writeBack = QQmlValueTypeFactory::valueType(core.propType());
-        writeBack->read(object, core.coreIndex());
-        rv = write(writeBack, valueTypeData, value, context, flags);
-        writeBack->write(object, core.coreIndex(), flags);
+        auto doWrite = [&](QQmlGadgetPtrWrapper *wrapper) {
+            wrapper->read(object, core.coreIndex());
+            rv = write(wrapper, valueTypeData, value, context, flags);
+            wrapper->write(object, core.coreIndex(), flags);
+        };
+
+        QQmlGadgetPtrWrapper *wrapper = context
+                ? QQmlGadgetPtrWrapper::instance(context->engine, core.propType())
+                : nullptr;
+        if (wrapper) {
+            doWrite(wrapper);
+        } else if (QQmlValueType *valueType = QQmlValueTypeFactory::valueType(core.propType())) {
+            QQmlGadgetPtrWrapper wrapper(valueType, nullptr);
+            doWrite(&wrapper);
+        }
+
     } else {
         rv = write(object, core, value, context, flags);
     }
diff --git a/src/qml/qml/qqmlvaluetype.cpp b/src/qml/qml/qqmlvaluetype.cpp
index a4737b3da9..a8fe4ec2c6 100644
--- a/src/qml/qml/qqmlvaluetype.cpp
+++ b/src/qml/qml/qqmlvaluetype.cpp
@@ -42,11 +42,11 @@
 #include <QtCore/qmutex.h>
 #include <private/qqmlglobal_p.h>
 #include <QtCore/qdebug.h>
+#include <private/qqmlengine_p.h>
 #include <private/qmetaobjectbuilder_p.h>
 #if QT_CONFIG(qml_itemmodel)
 #include <private/qqmlmodelindexvaluetype_p.h>
 #endif
-#include <private/qmetatype_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -214,61 +214,82 @@ void QQmlValueTypeFactory::registerValueTypes(const char *uri, int versionMajor,
     qmlRegisterValueTypeEnums<QQmlEasingValueType>(uri, versionMajor, versionMinor, "Easing");
 }
 
-QQmlValueType::QQmlValueType() :
-    _metaObject(nullptr),
-    gadgetPtr(nullptr),
-    metaType(QMetaType::UnknownType)
+QQmlValueType::QQmlValueType(int typeId, const QMetaObject *gadgetMetaObject)
+    : metaType(typeId)
 {
+    QMetaObjectBuilder builder(gadgetMetaObject);
+    dynamicMetaObject = builder.toMetaObject();
+    *static_cast<QMetaObject*>(this) = *dynamicMetaObject;
 }
 
-QQmlValueType::QQmlValueType(int typeId, const QMetaObject *gadgetMetaObject)
-    : gadgetPtr(QMetaType::create(typeId))
-    , metaType(typeId)
+QQmlValueType::~QQmlValueType()
 {
-    QObjectPrivate *op = QObjectPrivate::get(this);
-    Q_ASSERT(!op->metaObject);
-    op->metaObject = this;
+    ::free(dynamicMetaObject);
+}
 
-    QMetaObjectBuilder builder(gadgetMetaObject);
-    _metaObject = builder.toMetaObject();
+QQmlGadgetPtrWrapper *QQmlGadgetPtrWrapper::instance(QQmlEngine *engine, int index)
+{
+    return engine ? QQmlEnginePrivate::get(engine)->valueTypeInstance(index) : nullptr;
+}
 
-    *static_cast<QMetaObject*>(this) = *_metaObject;
+QQmlGadgetPtrWrapper::QQmlGadgetPtrWrapper(QQmlValueType *valueType, QObject *parent)
+    : QObject(parent), m_gadgetPtr(valueType->create())
+{
+    QObjectPrivate *d = QObjectPrivate::get(this);
+    Q_ASSERT(!d->metaObject);
+    d->metaObject = valueType;
 }
 
-QQmlValueType::~QQmlValueType()
+QQmlGadgetPtrWrapper::~QQmlGadgetPtrWrapper()
 {
-    QObjectPrivate *op = QObjectPrivate::get(this);
-    Q_ASSERT(op->metaObject == nullptr || op->metaObject == this);
-    op->metaObject = nullptr;
-    ::free(const_cast<QMetaObject *>(_metaObject));
-    metaType.destroy(gadgetPtr);
+    QObjectPrivate *d = QObjectPrivate::get(this);
+    static_cast<const QQmlValueType *>(d->metaObject)->destroy(m_gadgetPtr);
+    d->metaObject = nullptr;
 }
 
-void QQmlValueType::read(QObject *obj, int idx)
+void QQmlGadgetPtrWrapper::read(QObject *obj, int idx)
 {
-    void *a[] = { gadgetPtr, nullptr };
+    Q_ASSERT(m_gadgetPtr);
+    void *a[] = { m_gadgetPtr, nullptr };
     QMetaObject::metacall(obj, QMetaObject::ReadProperty, idx, a);
 }
 
-void QQmlValueType::write(QObject *obj, int idx, QQmlPropertyData::WriteFlags flags)
+void QQmlGadgetPtrWrapper::write(QObject *obj, int idx, QQmlPropertyData::WriteFlags flags)
 {
-    Q_ASSERT(gadgetPtr);
+    Q_ASSERT(m_gadgetPtr);
     int status = -1;
-    void *a[] = { gadgetPtr, nullptr, &status, &flags };
+    void *a[] = { m_gadgetPtr, nullptr, &status, &flags };
     QMetaObject::metacall(obj, QMetaObject::WriteProperty, idx, a);
 }
 
-QVariant QQmlValueType::value()
+QVariant QQmlGadgetPtrWrapper::value()
+{
+    Q_ASSERT(m_gadgetPtr);
+    return QVariant(metaTypeId(), m_gadgetPtr);
+}
+
+void QQmlGadgetPtrWrapper::setValue(const QVariant &value)
+{
+    Q_ASSERT(m_gadgetPtr);
+    Q_ASSERT(metaTypeId() == value.userType());
+    const QQmlValueType *type = valueType();
+    type->destruct(m_gadgetPtr);
+    type->construct(m_gadgetPtr, value.constData());
+}
+
+int QQmlGadgetPtrWrapper::metaCall(QMetaObject::Call type, int id, void **argv)
 {
-    Q_ASSERT(gadgetPtr);
-    return QVariant(metaType.id(), gadgetPtr);
+    Q_ASSERT(m_gadgetPtr);
+    const QMetaObject *metaObject = valueType();
+    QQmlMetaObject::resolveGadgetMethodOrPropertyIndex(type, &metaObject, &id);
+    metaObject->d.static_metacall(static_cast<QObject *>(m_gadgetPtr), type, id, argv);
+    return id;
 }
 
-void QQmlValueType::setValue(const QVariant &value)
+const QQmlValueType *QQmlGadgetPtrWrapper::valueType() const
 {
-    Q_ASSERT(metaType.id() == value.userType());
-    metaType.destruct(gadgetPtr);
-    metaType.construct(gadgetPtr, value.constData());
+    const QObjectPrivate *d = QObjectPrivate::get(this);
+    return static_cast<const QQmlValueType *>(d->metaObject);
 }
 
 QAbstractDynamicMetaObject *QQmlValueType::toDynamicMetaObject(QObject *)
@@ -280,12 +301,9 @@ void QQmlValueType::objectDestroyed(QObject *)
 {
 }
 
-int QQmlValueType::metaCall(QObject *, QMetaObject::Call type, int _id, void **argv)
+int QQmlValueType::metaCall(QObject *object, QMetaObject::Call type, int _id, void **argv)
 {
-    const QMetaObject *mo = _metaObject;
-    QQmlMetaObject::resolveGadgetMethodOrPropertyIndex(type, &mo, &_id);
-    mo->d.static_metacall(reinterpret_cast<QObject*>(gadgetPtr), type, _id, argv);
-    return _id;
+    return static_cast<QQmlGadgetPtrWrapper *>(object)->metaCall(type, _id, argv);
 }
 
 QString QQmlPointFValueType::toString() const
diff --git a/src/qml/qml/qqmlvaluetype_p.h b/src/qml/qml/qqmlvaluetype_p.h
index cf53b8cb4a..fec942fcbb 100644
--- a/src/qml/qml/qqmlvaluetype_p.h
+++ b/src/qml/qml/qqmlvaluetype_p.h
@@ -54,7 +54,9 @@
 #include "qqml.h"
 #include "qqmlproperty.h"
 #include "qqmlproperty_p.h"
+
 #include <private/qqmlnullablevalue_p.h>
+#include <private/qmetatype_p.h>
 
 #include <QtCore/qobject.h>
 #include <QtCore/qrect.h>
@@ -63,16 +65,20 @@
 
 QT_BEGIN_NAMESPACE
 
-class Q_QML_PRIVATE_EXPORT QQmlValueType : public QObject, public QAbstractDynamicMetaObject
+class Q_QML_PRIVATE_EXPORT QQmlValueType : public QAbstractDynamicMetaObject
 {
 public:
-    QQmlValueType();
+    QQmlValueType() : metaType(QMetaType::UnknownType) {}
     QQmlValueType(int userType, const QMetaObject *metaObject);
-    ~QQmlValueType() override;
-    void read(QObject *, int);
-    void write(QObject *, int, QQmlPropertyData::WriteFlags flags);
-    QVariant value();
-    void setValue(const QVariant &);
+    ~QQmlValueType();
+
+    void *create() const { return metaType.create(); }
+    void destroy(void *gadgetPtr) const { metaType.destroy(gadgetPtr); }
+
+    void construct(void *gadgetPtr, const void *copy) const { metaType.construct(gadgetPtr, copy); }
+    void destruct(void *gadgetPtr) const { metaType.destruct(gadgetPtr); }
+
+    int metaTypeId() const { return metaType.id(); }
 
     // ---- dynamic meta object data interface
     QAbstractDynamicMetaObject *toDynamicMetaObject(QObject *) override;
@@ -80,12 +86,33 @@ public:
     int metaCall(QObject *obj, QMetaObject::Call type, int _id, void **argv) override;
     // ----
 
-private:
-    const QMetaObject *_metaObject;
-    void *gadgetPtr;
-
 public:
     QMetaType metaType;
+    QMetaObject *dynamicMetaObject = nullptr;
+};
+
+class Q_QML_PRIVATE_EXPORT QQmlGadgetPtrWrapper : public QObject
+{
+    Q_OBJECT
+public:
+    static QQmlGadgetPtrWrapper *instance(QQmlEngine *engine, int index);
+
+    QQmlGadgetPtrWrapper(QQmlValueType *valueType, QObject *parent);
+    ~QQmlGadgetPtrWrapper();
+
+    void read(QObject *obj, int idx);
+    void write(QObject *obj, int idx, QQmlPropertyData::WriteFlags flags);
+    QVariant value();
+    void setValue(const QVariant &value);
+
+    int metaTypeId() const { return valueType()->metaTypeId(); }
+    int metaCall(QMetaObject::Call type, int id, void **argv);
+    QMetaProperty property(int index) { return valueType()->property(index); }
+
+private:
+    const QQmlValueType *valueType() const;
+
+    void *m_gadgetPtr = nullptr;
 };
 
 class Q_QML_PRIVATE_EXPORT QQmlValueTypeFactory
diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp
index 95e99206f0..69215b1304 100644
--- a/src/qml/qml/qqmlvmemetaobject.cpp
+++ b/src/qml/qml/qqmlvmemetaobject.cpp
@@ -241,11 +241,13 @@ bool QQmlInterceptorMetaObject::intercept(QMetaObject::Call c, int id, void **a)
                 continue;
 
             const int valueIndex = vi->m_propertyIndex.valueTypeIndex();
-            int type = QQmlData::get(object)->propertyCache->property(id)->propType();
+            const QQmlData *data = QQmlData::get(object);
+            const int type = data->propertyCache->property(id)->propType();
 
             if (type != QVariant::Invalid) {
                 if (valueIndex != -1) {
-                    QQmlValueType *valueType = QQmlValueTypeFactory::valueType(type);
+                    QQmlGadgetPtrWrapper *valueType = QQmlGadgetPtrWrapper::instance(
+                                data->context->engine, type);
                     Q_ASSERT(valueType);
 
                     //
@@ -279,7 +281,7 @@ bool QQmlInterceptorMetaObject::intercept(QMetaObject::Call c, int id, void **a)
                     //   (7) Issue the interceptor call with the new component value.
                     //
 
-                    QMetaProperty valueProp = valueType->metaObject()->property(valueIndex);
+                    QMetaProperty valueProp = valueType->property(valueIndex);
                     QVariant newValue(type, a[0]);
 
                     valueType->read(object, id);
@@ -833,9 +835,9 @@ int QQmlVMEMetaObject::metaCall(QObject *o, QMetaObject::Call c, int _id, void *
                         return -1;
                     const QQmlPropertyData *pd = targetDData->propertyCache->property(coreIndex);
                     // Value type property or deep alias
-                    QQmlValueType *valueType = QQmlValueTypeFactory::valueType(pd->propType());
+                    QQmlGadgetPtrWrapper *valueType = QQmlGadgetPtrWrapper::instance(
+                                ctxt->engine, pd->propType());
                     if (valueType) {
-
                         valueType->read(target, coreIndex);
                         int rv = QMetaObject::metacall(valueType, c, valueTypePropertyIndex, a);
 
diff --git a/src/quick/designer/qquickdesignersupportproperties.cpp b/src/quick/designer/qquickdesignersupportproperties.cpp
index c746f55daa..335795acf1 100644
--- a/src/quick/designer/qquickdesignersupportproperties.cpp
+++ b/src/quick/designer/qquickdesignersupportproperties.cpp
@@ -155,8 +155,8 @@ QQuickDesignerSupport::PropertyNameList QQuickDesignerSupportProperties::propert
                                                                                   baseName +  QQuickDesignerSupport::PropertyName(metaProperty.name())
                                                                                   + '.', inspectedObjects));
             }
-        } else if (QQmlValueTypeFactory::valueType(metaProperty.userType())) {
-            QQmlValueType *valueType = QQmlValueTypeFactory::valueType(metaProperty.userType());
+        } else if (QQmlGadgetPtrWrapper *valueType
+                   = QQmlGadgetPtrWrapper::instance(qmlEngine(object), metaProperty.userType())) {
             valueType->setValue(metaProperty.read(object));
             propertyNameList.append(propertyNameListForWritableProperties(valueType,
                                                                           baseName +  QQuickDesignerSupport::PropertyName(metaProperty.name())
@@ -223,8 +223,8 @@ QQuickDesignerSupport::PropertyNameList QQuickDesignerSupportProperties::allProp
                                                              + QQuickDesignerSupport::PropertyName(metaProperty.name())
                                                              + '.', inspectedObjects));
             }
-        } else if (QQmlValueTypeFactory::valueType(metaProperty.userType())) {
-            QQmlValueType *valueType = QQmlValueTypeFactory::valueType(metaProperty.userType());
+        } else if (QQmlGadgetPtrWrapper *valueType
+                   = QQmlGadgetPtrWrapper::instance(qmlEngine(object), metaProperty.userType())) {
             valueType->setValue(metaProperty.read(object));
             propertyNameList.append(baseName + QQuickDesignerSupport::PropertyName(metaProperty.name()));
             propertyNameList.append(allPropertyNames(valueType,