QtQml: Restore immutability of QQmlPropertyCache

We need to make sure the operations on the metaobject pointer are
atomic. Otherwise we can mess up the refcounting or create the shared
metaobjects multiple times. We have an atomic refcounting mechanism, so
let's use that. At the same time, realize that we don't have to
double-indirect access to the static metaobjects because we don't have
to refcount those. Therefore, replace the RefCountedMetaObject with a
class that makes both the pointer operations and the refcounting atomic,
and stores static metaobject pointers directly.

Pick-to: 6.3
Fixes: QTBUG-73271
Change-Id: Icd63413a3dbbb43ebb266ed6b4f9e6444ecbf908
Reviewed-by: Maximilian Goldstein <max.goldstein@qt.io>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

1 files changed, 65 insertions, 68 deletions

diff --git a/src/qml/qml/qqmlpropertycache_p.h b/src/qml/qml/qqmlpropertycache_p.h
index 4c10de7823..9762cdef5a 100644
--- a/src/qml/qml/qqmlpropertycache_p.h
+++ b/src/qml/qml/qqmlpropertycache_p.h
@@ -72,85 +72,76 @@ class QQmlContextData;
 class QQmlPropertyCacheMethodArguments;
 class QQmlVMEMetaObject;
 
-class RefCountedMetaObject {
+class QQmlMetaObjectPointer
+{
 public:
-    enum OwnershipMode {
-        StaticMetaObject,
-        SharedMetaObject
-    };
+    QQmlMetaObjectPointer() = default;
 
-    struct Data {
-        ~Data() {
-            if (mode == SharedMetaObject)
-                ::free(sharedMetaObject);
-        }
-        union {
-            QMetaObject *sharedMetaObject = nullptr;
-            const QMetaObject *staticMetaObject;
-        };
-        int ref = 1;
-        OwnershipMode mode;
-    } *d;
-
-    RefCountedMetaObject()
-        : d(nullptr)
-    {}
-
-    static RefCountedMetaObject createShared(QMetaObject *mo)
+    QQmlMetaObjectPointer(const QMetaObject *staticMetaObject)
+        : d(quintptr(staticMetaObject))
     {
-        RefCountedMetaObject result;
-        result.d = new Data();
-        result.d->sharedMetaObject = mo;
-        result.d->mode = SharedMetaObject;
-        return result;
+        Q_ASSERT((d & Shared) == 0);
     }
 
-    static RefCountedMetaObject createStatic(const QMetaObject *mo)
+    ~QQmlMetaObjectPointer()
     {
-        RefCountedMetaObject result;
-        result.d = new Data();
-        result.d->staticMetaObject = mo;
-        result.d->mode = StaticMetaObject;
-        return result;
+        if (d & Shared)
+            reinterpret_cast<SharedHolder *>(d ^ Shared)->release();
     }
 
-    ~RefCountedMetaObject() {
-        if (d && !--d->ref)
-            delete d;
+    QQmlMetaObjectPointer(const QQmlMetaObjectPointer &other)
+        : d(other.d.loadRelaxed())
+    {
+        // other has to survive until this ctor is done. So d cannot disappear before.
+        if (d & Shared)
+            reinterpret_cast<SharedHolder *>(d ^ Shared)->addref();
     }
-    RefCountedMetaObject(const RefCountedMetaObject &other)
-        : d(other.d)
+
+    QQmlMetaObjectPointer(QQmlMetaObjectPointer &&other) = delete;
+    QQmlMetaObjectPointer &operator=(QQmlMetaObjectPointer &&other) = delete;
+    QQmlMetaObjectPointer &operator=(const QQmlMetaObjectPointer &other) = delete;
+
+    void setSharedOnce(QMetaObject *shared) const
     {
-        if (d && d->ref > 0)
-            ++d->ref;
+        SharedHolder *holder = new SharedHolder(shared);
+        if (!d.testAndSetRelaxed(0, quintptr(holder) | Shared))
+            holder->release();
     }
-    RefCountedMetaObject &operator =(const RefCountedMetaObject &other)
+
+    const QMetaObject *metaObject() const
     {
-        if (d == other.d)
-            return *this;
-        if (d && !--d->ref)
-            delete d;
-        d = other.d;
-        if (d && d->ref > 0)
-            ++d->ref;
-        return *this;
+        if (d & Shared)
+            return reinterpret_cast<SharedHolder *>(d ^ Shared)->metaObject;
+        return reinterpret_cast<const QMetaObject *>(d.loadRelaxed());
     }
 
-    const QMetaObject *constMetaObject() const
+    bool isShared() const
     {
-        if (!d)
-            return nullptr;
-        return isShared() ? d->sharedMetaObject : d->staticMetaObject;
+        // This works because static metaobjects need to be set in the ctor and once a shared
+        // metaobject has been set, it cannot be removed anymore.
+        return !d || (d & Shared);
     }
 
-    QMetaObject *sharedMetaObject() const
+    bool isNull() const
     {
-        return isShared() ? d->sharedMetaObject : nullptr;
+        return d == 0;
     }
 
-    operator const QMetaObject *() const { return constMetaObject(); }
-    const QMetaObject * operator ->() const { return constMetaObject(); }
-    bool isShared() const { return d && d->mode == SharedMetaObject; }
+private:
+    enum Tag {
+        Static = 0,
+        Shared = 1
+    };
+
+    struct SharedHolder : public QQmlRefCount
+    {
+        Q_DISABLE_COPY_MOVE(SharedHolder)
+        SharedHolder(QMetaObject *shared) : metaObject(shared) {}
+        ~SharedHolder() { free(metaObject); }
+        QMetaObject *metaObject;
+    };
+
+    mutable QBasicAtomicInteger<quintptr> d = 0;
 };
 
 class Q_QML_PRIVATE_EXPORT QQmlPropertyCache : public QQmlRefCount
@@ -184,7 +175,7 @@ public:
     void appendEnum(const QString &, const QVector<QQmlEnumValue> &);
 
     const QMetaObject *metaObject() const;
-    const QMetaObject *createMetaObject();
+    const QMetaObject *createMetaObject() const;
     const QMetaObject *firstCppMetaObject() const;
 
     template<typename K>
@@ -240,7 +231,7 @@ public:
     inline int signalOffset() const;
     inline int qmlEnumCount() const;
 
-    void toMetaObjectBuilder(QMetaObjectBuilder &);
+    void toMetaObjectBuilder(QMetaObjectBuilder &) const;
 
     inline bool callJSFactoryMethod(QObject *object, void **args) const;
 
@@ -260,7 +251,9 @@ private:
     friend class QQmlComponentAndAliasResolver;
     friend class QQmlMetaObject;
 
-    inline QQmlRefPointer<QQmlPropertyCache> copy(int reserve);
+    QQmlPropertyCache(const QQmlMetaObjectPointer &metaObject) : _metaObject(metaObject) {}
+
+    inline QQmlRefPointer<QQmlPropertyCache> copy(const QQmlMetaObjectPointer &mo, int reserve);
 
     void append(const QMetaObject *, QTypeRevision typeVersion,
                 QQmlPropertyData::Flags propertyFlags = QQmlPropertyData::Flags(),
@@ -327,7 +320,7 @@ private:
     AllowedRevisionCache allowedRevisionCache;
     QVector<QQmlEnumData> enumCache;
 
-    RefCountedMetaObject _metaObject;
+    QQmlMetaObjectPointer _metaObject;
     QByteArray _dynamicClassName;
     QByteArray _dynamicStringData;
     QByteArray _listPropertyAssignBehavior;
@@ -342,7 +335,7 @@ private:
 // Returns this property cache's metaObject.  May be null if it hasn't been created yet.
 inline const QMetaObject *QQmlPropertyCache::metaObject() const
 {
-    return _metaObject;
+    return _metaObject.metaObject();
 }
 
 // Returns the first C++ type's QMetaObject - that is, the first QMetaObject not created by
@@ -350,9 +343,9 @@ inline const QMetaObject *QQmlPropertyCache::metaObject() const
 inline const QMetaObject *QQmlPropertyCache::firstCppMetaObject() const
 {
     const QQmlPropertyCache *p = this;
-    while (!p->_metaObject || p->_metaObject.isShared())
+    while (p->_metaObject.isShared())
         p = p->parent().data();
-    return p->_metaObject;
+    return p->_metaObject.metaObject();
 }
 
 inline QQmlPropertyData *QQmlPropertyCache::property(int index) const
@@ -495,8 +488,12 @@ int QQmlPropertyCache::qmlEnumCount() const
 bool QQmlPropertyCache::callJSFactoryMethod(QObject *object, void **args) const
 {
     if (_jsFactoryMethodIndex != -1) {
-        _metaObject->d.static_metacall(object, QMetaObject::InvokeMetaMethod, _jsFactoryMethodIndex, args);
-        return true;
+        if (const QMetaObject *mo = _metaObject.metaObject()) {
+            mo->d.static_metacall(object, QMetaObject::InvokeMetaMethod,
+                                  _jsFactoryMethodIndex, args);
+            return true;
+        }
+        return false;
     }
     if (_parent)
         return _parent->callJSFactoryMethod(object, args);