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.

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>
(cherry picked from commit fe37c2667efb189a0e43083f626da317cfd2fb60)

2 files changed, 86 insertions, 89 deletions

diff --git a/src/qml/qml/qqmlpropertycache.cpp b/src/qml/qml/qqmlpropertycache.cpp
index 191944b2d5..bb305549e0 100644
--- a/src/qml/qml/qqmlpropertycache.cpp
+++ b/src/qml/qml/qqmlpropertycache.cpp
@@ -152,6 +152,7 @@ void QQmlPropertyData::load(const QMetaMethod &m)
 Creates a new QQmlPropertyCache of \a metaObject.
 */
 QQmlPropertyCache::QQmlPropertyCache(const QMetaObject *metaObject, QTypeRevision metaObjectRevision)
+    : _metaObject(metaObject)
 {
     Q_ASSERT(metaObject);
 
@@ -183,17 +184,17 @@ QQmlPropertyCache::~QQmlPropertyCache()
     stringCache.clear();
 }
 
-QQmlRefPointer<QQmlPropertyCache> QQmlPropertyCache::copy(int reserve)
+QQmlRefPointer<QQmlPropertyCache> QQmlPropertyCache::copy(
+        const QQmlMetaObjectPointer &mo, int reserve)
 {
     QQmlRefPointer<QQmlPropertyCache> cache = QQmlRefPointer<QQmlPropertyCache>(
-            new QQmlPropertyCache(), QQmlRefPointer<QQmlPropertyCache>::Adopt);
+            new QQmlPropertyCache(mo), QQmlRefPointer<QQmlPropertyCache>::Adopt);
     cache->_parent = this;
     cache->propertyIndexCacheStart = propertyIndexCache.count() + propertyIndexCacheStart;
     cache->methodIndexCacheStart = methodIndexCache.count() + methodIndexCacheStart;
     cache->signalHandlerIndexCacheStart = signalHandlerIndexCache.count() + signalHandlerIndexCacheStart;
     cache->stringCache.linkAndReserve(stringCache, reserve);
     cache->allowedRevisionCache = allowedRevisionCache;
-    cache->_metaObject = _metaObject;
     cache->_defaultPropertyName = _defaultPropertyName;
     cache->_listPropertyAssignBehavior = _listPropertyAssignBehavior;
 
@@ -202,19 +203,18 @@ QQmlRefPointer<QQmlPropertyCache> QQmlPropertyCache::copy(int reserve)
 
 QQmlRefPointer<QQmlPropertyCache> QQmlPropertyCache::copy()
 {
-    return copy(0);
+    return copy(_metaObject, 0);
 }
 
 QQmlRefPointer<QQmlPropertyCache> QQmlPropertyCache::copyAndReserve(
         int propertyCount, int methodCount, int signalCount, int enumCount)
 {
-    QQmlRefPointer<QQmlPropertyCache> rv = copy(propertyCount + methodCount + signalCount);
+    QQmlRefPointer<QQmlPropertyCache> rv = copy(
+                QQmlMetaObjectPointer(), propertyCount + methodCount + signalCount);
     rv->propertyIndexCache.reserve(propertyCount);
     rv->methodIndexCache.reserve(methodCount);
     rv->signalHandlerIndexCache.reserve(signalCount);
     rv->enumCache.reserve(enumCount);
-    rv->_metaObject = RefCountedMetaObject();
-
     return rv;
 }
 
@@ -323,16 +323,16 @@ void QQmlPropertyCache::appendEnum(const QString &name, const QVector<QQmlEnumVa
 }
 
 // Returns this property cache's metaObject, creating it if necessary.
-const QMetaObject *QQmlPropertyCache::createMetaObject()
+const QMetaObject *QQmlPropertyCache::createMetaObject() const
 {
-    if (!_metaObject) {
+    if (_metaObject.isNull()) {
         QMetaObjectBuilder builder;
         toMetaObjectBuilder(builder);
         builder.setSuperClass(_parent->createMetaObject());
-        _metaObject = RefCountedMetaObject::createShared(builder.toMetaObject());
+        _metaObject.setSharedOnce(builder.toMetaObject());
     }
 
-    return _metaObject;
+    return _metaObject.metaObject();
 }
 
 QQmlPropertyData *QQmlPropertyCache::maybeUnresolvedProperty(int index) const
@@ -372,9 +372,10 @@ QQmlPropertyCache::copyAndAppend(const QMetaObject *metaObject,
     // signal handlers and all the properties.  This assumes no name clashes, but this is the
     // common case.
     QQmlRefPointer<QQmlPropertyCache> rv = copy(
-            QMetaObjectPrivate::get(metaObject)->methodCount
-            + QMetaObjectPrivate::get(metaObject)->signalCount
-            + QMetaObjectPrivate::get(metaObject)->propertyCount);
+                metaObject,
+                QMetaObjectPrivate::get(metaObject)->methodCount
+                + QMetaObjectPrivate::get(metaObject)->signalCount
+                + QMetaObjectPrivate::get(metaObject)->propertyCount);
 
     rv->append(metaObject, typeVersion, propertyFlags, methodFlags, signalFlags);
 
@@ -387,7 +388,6 @@ void QQmlPropertyCache::append(const QMetaObject *metaObject,
                                QQmlPropertyData::Flags methodFlags,
                                QQmlPropertyData::Flags signalFlags)
 {
-    _metaObject = RefCountedMetaObject::createStatic(metaObject);
     allowedRevisionCache.append(QTypeRevision::zero());
 
     int methodCount = metaObject->methodCount();
@@ -959,20 +959,20 @@ static inline const QByteArray stringData(const QMetaObject *mo, int index)
 
 const char *QQmlPropertyCache::className() const
 {
-    if (_metaObject)
-        return _metaObject->className();
+    if (const QMetaObject *mo = _metaObject.metaObject())
+        return mo->className();
     else
         return _dynamicClassName.constData();
 }
 
-void QQmlPropertyCache::toMetaObjectBuilder(QMetaObjectBuilder &builder)
+void QQmlPropertyCache::toMetaObjectBuilder(QMetaObjectBuilder &builder) const
 {
     struct Sort { static bool lt(const QPair<QString, QQmlPropertyData *> &lhs,
                                  const QPair<QString, QQmlPropertyData *> &rhs) {
         return lhs.second->coreIndex() < rhs.second->coreIndex();
     } };
 
-    struct Insert { static void in(QQmlPropertyCache *This,
+    struct Insert { static void in(const QQmlPropertyCache *This,
                                    QList<QPair<QString, QQmlPropertyData *> > &properties,
                                    QList<QPair<QString, QQmlPropertyData *> > &methods,
                                    StringCache::ConstIterator iter, QQmlPropertyData *data) {
@@ -1268,7 +1268,7 @@ QByteArray QQmlPropertyCache::checksum(QHash<quintptr, QByteArray> *checksums, b
     }
 
     // Generate a checksum on the meta-object data only on C++ types.
-    if (!_metaObject || _metaObject.isShared()) {
+    if (_metaObject.isShared()) {
         *ok = false;
         return QByteArray();
     }
@@ -1281,7 +1281,7 @@ QByteArray QQmlPropertyCache::checksum(QHash<quintptr, QByteArray> *checksums, b
             return QByteArray();
     }
 
-    if (!addToHash(hash, *_metaObject)) {
+    if (!addToHash(hash, *_metaObject.metaObject())) {
         *ok = false;
         return QByteArray();
     }
diff --git a/src/qml/qml/qqmlpropertycache_p.h b/src/qml/qml/qqmlpropertycache_p.h
index 2e3368ebaa..36dc6f782f 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>
@@ -237,7 +228,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;
 
@@ -257,7 +248,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(),
@@ -324,7 +317,7 @@ private:
     AllowedRevisionCache allowedRevisionCache;
     QVector<QQmlEnumData> enumCache;
 
-    RefCountedMetaObject _metaObject;
+    QQmlMetaObjectPointer _metaObject;
     QByteArray _dynamicClassName;
     QByteArray _dynamicStringData;
     QByteArray _listPropertyAssignBehavior;
@@ -339,7 +332,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
@@ -347,9 +340,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
@@ -492,8 +485,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);