Resolve data race in QQmlPropertyData in a minimally invasive way

We keep all the idiosyncrasies of when what flag or member is set. We
only make sure not to call resolve() concurrently by moving the propType
into an atomic int which also replaces the "fully resolved" flag.

We need to move another 16bit member in there as only 32bit atomics are
guaranteed to work everywhere and we want to keep the fixed size of the
class. So we choose the relative property index and only set it when we
are sure there isn't a data race. This is fine because for missing
relative property indices there is a fallback.

Task-number: QTBUG-93973
Change-Id: Ia357444cad85fe2df234e76226dd8adc10fe8549
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

4 files changed, 138 insertions, 85 deletions

diff --git a/src/qml/qml/qqmlbinding.cpp b/src/qml/qml/qqmlbinding.cpp
index a4c0717dfc..3b5b0de91b 100644
--- a/src/qml/qml/qqmlbinding.cpp
+++ b/src/qml/qml/qqmlbinding.cpp
@@ -645,7 +645,10 @@ void QQmlBinding::getPropertyData(QQmlPropertyData **propertyData, QQmlPropertyD
         Q_ASSERT(valueTypeMetaObject);
         QMetaProperty vtProp = valueTypeMetaObject->property(m_targetIndex.valueTypeIndex());
         valueTypeData->setFlags(QQmlPropertyData::flagsForProperty(vtProp));
+
+        // valueTypeData is expected to be local here. It must not be shared with other threads.
         valueTypeData->setPropType(vtProp.userType());
+
         valueTypeData->setCoreIndex(m_targetIndex.valueTypeIndex());
     }
 }
@@ -748,7 +751,11 @@ QQmlBinding *QQmlBinding::newBinding(QQmlEnginePrivate *engine, const QQmlProper
     if (property && property->isQObject())
         return new QObjectPointerBinding(engine, property->propType());
 
-    const int type = (property && property->isFullyResolved()) ? property->propType() : QMetaType::UnknownType;
+    // If the property is not resolved at this point, you get a binding of unknown type.
+    // This has been the case for a long time and we keep it like this in Qt5 to be bug-compatible.
+    const int type = (property && property->isResolved())
+            ? property->propType()
+            : QMetaType::UnknownType;
 
     if (type == qMetaTypeId<QQmlBinding *>()) {
         return new QQmlBindingBinding;
diff --git a/src/qml/qml/qqmlpropertycache.cpp b/src/qml/qml/qqmlpropertycache.cpp
index 3cc2b1e391..c5e34e1d15 100644
--- a/src/qml/qml/qqmlpropertycache.cpp
+++ b/src/qml/qml/qqmlpropertycache.cpp
@@ -138,17 +138,17 @@ void QQmlPropertyData::lazyLoad(const QMetaProperty &p)
 {
     populate(this, p);
     int type = static_cast<int>(p.userType());
-    if (type == QMetaType::QObjectStar) {
-        setPropType(type);
+
+    if (type >= QMetaType::User || type == 0)
+        return; // Resolve later
+
+    if (type == QMetaType::QObjectStar)
         m_flags.type = Flags::QObjectDerivedType;
-    } else if (type == QMetaType::QVariant) {
-        setPropType(type);
+    else if (type == QMetaType::QVariant)
         m_flags.type = Flags::QVariantType;
-    } else if (type >= QMetaType::User || type == 0) {
-        m_flags.notFullyResolved = true;
-    } else {
-        setPropType(type);
-    }
+
+    // This is OK because lazyLoad is done before exposing the property data.
+    setPropType(type);
 }
 
 void QQmlPropertyData::load(const QMetaProperty &p)
@@ -158,45 +158,49 @@ void QQmlPropertyData::load(const QMetaProperty &p)
     flagsForPropertyType(propType(), m_flags);
 }
 
-void QQmlPropertyData::load(const QMetaMethod &m)
+static void populate(QQmlPropertyData *data, const QMetaMethod &m)
 {
-    setCoreIndex(m.methodIndex());
-    setArguments(nullptr);
+    data->setCoreIndex(m.methodIndex());
+    data->setArguments(nullptr);
 
-    setPropType(m.returnType());
-
-    m_flags.type = Flags::FunctionType;
-    if (m.methodType() == QMetaMethod::Signal) {
-        m_flags.setIsSignal(true);
-    } else if (m.methodType() == QMetaMethod::Constructor) {
-        m_flags.setIsConstructor(true);
-        setPropType(QMetaType::QObjectStar);
-    }
+    QQmlPropertyData::Flags flags = data->flags();
+    flags.type = QQmlPropertyData::Flags::FunctionType;
+    if (m.methodType() == QMetaMethod::Signal)
+        flags.setIsSignal(true);
+    else if (m.methodType() == QMetaMethod::Constructor)
+        flags.setIsConstructor(true);
 
     const int paramCount = m.parameterCount();
     if (paramCount) {
-        m_flags.setHasArguments(true);
+        flags.setHasArguments(true);
         if ((paramCount == 1) && (m.parameterTypes().constFirst() == "QQmlV4Function*"))
-            m_flags.setIsV4Function(true);
+            flags.setIsV4Function(true);
     }
 
     if (m.attributes() & QMetaMethod::Cloned)
-        m_flags.setIsCloned(true);
+        flags.setIsCloned(true);
+
+    data->setFlags(flags);
 
     Q_ASSERT(m.revision() <= Q_INT16_MAX);
-    setRevision(m.revision());
+    data->setRevision(m.revision());
 }
 
-void QQmlPropertyData::lazyLoad(const QMetaMethod &m)
+void QQmlPropertyData::load(const QMetaMethod &m)
 {
-    load(m);
+    populate(this, m);
+    setPropType(m.methodType() == QMetaMethod::Constructor
+                    ? QMetaType::QObjectStar
+                    : m.returnType());
+}
 
+void QQmlPropertyData::lazyLoad(const QMetaMethod &m)
+{
     const char *returnType = m.typeName();
-    if (!returnType)
-        returnType = "\0";
-    if ((*returnType != 'v') || (qstrcmp(returnType+1, "oid") != 0)) {
-        m_flags.notFullyResolved = true;
-    }
+    if (!returnType || *returnType != 'v' || qstrcmp(returnType + 1, "oid") != 0)
+        populate(this, m);
+    else
+        load(m); // If it's void, resolve it right away
 }
 
 /*!
@@ -650,25 +654,23 @@ void QQmlPropertyCache::append(const QMetaObject *metaObject,
     }
 }
 
-void QQmlPropertyCache::resolve(QQmlPropertyData *data) const
+int QQmlPropertyCache::findPropType(const QQmlPropertyData *data) const
 {
-    Q_ASSERT(data->notFullyResolved());
-    data->m_flags.notFullyResolved = false;
-
+    int type = QMetaType::UnknownType;
     const QMetaObject *mo = firstCppMetaObject();
     if (data->isFunction()) {
         auto metaMethod = mo->method(data->coreIndex());
         const char *retTy = metaMethod.typeName();
         if (!retTy)
             retTy = "\0";
-        data->setPropType(QMetaType::type(retTy));
+        type = QMetaType::type(retTy);
     } else {
         auto metaProperty = mo->property(data->coreIndex());
-        data->setPropType(QMetaType::type(metaProperty.typeName()));
+        type = QMetaType::type(metaProperty.typeName());
     }
 
     if (!data->isFunction()) {
-        if (data->propType() == QMetaType::UnknownType) {
+        if (type == QMetaType::UnknownType) {
             QQmlPropertyCache *p = _parent;
             while (p && (!mo || _ownMetaObject)) {
                 mo = p->_metaObject;
@@ -684,11 +686,33 @@ void QQmlPropertyCache::resolve(QQmlPropertyData *data) const
 
                 int registerResult = -1;
                 void *argv[] = { &registerResult };
-                mo->static_metacall(QMetaObject::RegisterPropertyMetaType, data->coreIndex() - propOffset, argv);
-                data->setPropType(registerResult == -1 ? QMetaType::UnknownType : registerResult);
+                mo->static_metacall(QMetaObject::RegisterPropertyMetaType,
+                                    data->coreIndex() - propOffset, argv);
+                type = registerResult == -1 ? QMetaType::UnknownType : registerResult;
             }
         }
-        flagsForPropertyType(data->propType(), data->m_flags);
+    }
+
+    return type;
+}
+
+void QQmlPropertyCache::resolve(QQmlPropertyData *data) const
+{
+    const int type = findPropType(data);
+
+    // Setting the flags unsynchronized is somewhat dirty but unlikely to cause trouble
+    // in practice. We have to do this before setting the property type because otherwise
+    // a consumer of the flags might see outdated flags even after the property type has
+    // become valid. The flags should only depend on the property type and the property
+    // type should be the same across different invocations. So, setting this concurrently
+    // should be a noop.
+    if (!data->isFunction())
+        flagsForPropertyType(type, data->m_flags);
+
+    // This is the one place where we can update the property type after exposing the data.
+    if (!data->m_propTypeAndRelativePropIndex.testAndSetOrdered(
+                0, type > 0 ? quint32(type) : quint32(QQmlPropertyData::PropTypeUnknown))) {
+        return; // Someone else is resolving it already
     }
 }
 
diff --git a/src/qml/qml/qqmlpropertycache_p.h b/src/qml/qml/qqmlpropertycache_p.h
index e6700e2e15..088c382d7d 100644
--- a/src/qml/qml/qqmlpropertycache_p.h
+++ b/src/qml/qml/qqmlpropertycache_p.h
@@ -220,6 +220,8 @@ private:
         _hasPropertyOverrides |= isOverride;
     }
 
+    int findPropType(const QQmlPropertyData *data) const;
+
 private:
     QQmlPropertyCache *_parent;
     int propertyIndexCacheStart;
@@ -246,7 +248,11 @@ private:
 
 inline QQmlPropertyData *QQmlPropertyCache::ensureResolved(QQmlPropertyData *p) const
 {
-    if (p && Q_UNLIKELY(p->notFullyResolved()))
+    // Avoid resolve() in the common case where it's already initialized and we don't
+    // run into a data race. resolve() checks again, with an atomic operation.
+    // If there is no coreIndex, there is no point in trying to resolve anything. In that
+    // case it's a default-constructed instance that never got load()'ed or lazyLoad()'ed.
+    if (p && p->coreIndex() != -1 && Q_UNLIKELY(p->m_propTypeAndRelativePropIndex == 0))
         resolve(p);
 
     return p;
diff --git a/src/qml/qml/qqmlpropertydata_p.h b/src/qml/qml/qqmlpropertydata_p.h
index f4415aa6a9..345342af16 100644
--- a/src/qml/qml/qqmlpropertydata_p.h
+++ b/src/qml/qml/qqmlpropertydata_p.h
@@ -84,14 +84,6 @@ public:
             QVariantType         = 9  // Property is a QVariant
         };
 
-        // The _otherBits (which "pad" the Flags struct to align it nicely) are used
-        // to store the relative property index. It will only get used when said index fits. See
-        // trySetStaticMetaCallFunction for details.
-        // (Note: this padding is done here, because certain compilers have surprising behavior
-        // when an enum is declared in-between two bit fields.)
-        enum { BitsLeftInFlags = 15 };
-        unsigned otherBits       : BitsLeftInFlags; // align to 32 bits
-
         // Members of the form aORb can only be a when type is not FunctionType, and only be
         // b when type equals FunctionType. For that reason, the semantic meaning of the bit is
         // overloaded, and the accessor functions are used to get the correct value
@@ -102,25 +94,24 @@ public:
         //
         // Lastly, isDirect and isOverridden apply to both functions and non-functions
     private:
-        unsigned isConstantORisVMEFunction     : 1; // Has CONST flag OR Function was added by QML
-        unsigned isWritableORhasArguments      : 1; // Has WRITE function OR Function takes arguments
-        unsigned isResettableORisSignal        : 1; // Has RESET function OR Function is a signal
-        unsigned isAliasORisVMESignal          : 1; // Is a QML alias to another property OR Signal was added by QML
-        unsigned isFinalORisV4Function         : 1; // Has FINAL flag OR Function takes QQmlV4Function* args
-        unsigned isSignalHandler               : 1; // Function is a signal handler
-        unsigned isOverload                    : 1; // Function is an overload of another function
-        unsigned isRequiredORisCloned          : 1; // Has REQUIRED flag OR The function was marked as cloned
-        unsigned isConstructor                 : 1; // The function was marked is a constructor
-        unsigned isDirect                      : 1; // Exists on a C++ QMetaObject
-        unsigned isOverridden                  : 1; // Is overridden by a extension property
+        quint16 isConstantORisVMEFunction   : 1; // Has CONST flag OR Function was added by QML
+        quint16 isWritableORhasArguments    : 1; // Has WRITE function OR Function takes arguments
+        quint16 isResettableORisSignal      : 1; // Has RESET function OR Function is a signal
+        quint16 isAliasORisVMESignal        : 1; // Is a QML alias to another property OR Signal was added by QML
+        quint16 isFinalORisV4Function       : 1; // Has FINAL flag OR Function takes QQmlV4Function* args
+        quint16 isSignalHandler             : 1; // Function is a signal handler
+        quint16 isOverload                  : 1; // Function is an overload of another function
+        quint16 isRequiredORisCloned        : 1; // Has REQUIRED flag OR The function was marked as cloned
+        quint16 isConstructor               : 1; // The function was marked is a constructor
+        quint16 isDirect                    : 1; // Exists on a C++ QMetaObject
+        quint16 isOverridden                : 1; // Is overridden by a extension property
     public:
-        unsigned type             : 4; // stores an entry of Types
+        quint16 type                        : 4; // stores an entry of Types
 
         // Apply only to IsFunctions
 
         // Internal QQmlPropertyCache flags
-        unsigned notFullyResolved : 1; // True if the type data is to be lazily resolved
-        unsigned overrideIndexIsProperty: 1;
+        quint16 overrideIndexIsProperty: 1;
 
         inline Flags();
         inline bool operator==(const Flags &other) const;
@@ -208,16 +199,12 @@ public:
 
     };
 
+    Q_STATIC_ASSERT(sizeof(Flags) == sizeof(quint16));
 
     inline bool operator==(const QQmlPropertyData &) const;
 
     Flags flags() const { return m_flags; }
-    void setFlags(Flags f)
-    {
-        unsigned otherBits = m_flags.otherBits;
-        m_flags = f;
-        m_flags.otherBits = otherBits;
-    }
+    void setFlags(Flags f) { m_flags = f; }
 
     bool isValid() const { return coreIndex() != -1; }
 
@@ -253,14 +240,26 @@ public:
     bool hasOverride() const { return overrideIndex() >= 0; }
     bool hasRevision() const { return revision() != 0; }
 
-    bool isFullyResolved() const { return !m_flags.notFullyResolved; }
+    // This is unsafe in the general case. The property might be in the process of getting
+    // resolved. Only use it if this case has been taken into account.
+    bool isResolved() const { return m_propTypeAndRelativePropIndex != 0; }
+
+    int propType() const
+    {
+        const quint32 type = m_propTypeAndRelativePropIndex & PropTypeMask;
+        Q_ASSERT(type > 0); // Property has to be fully resolved.
+        return type == PropTypeUnknown ? 0 : type;
+    }
 
-    int propType() const { Q_ASSERT(isFullyResolved()); return m_propType; }
     void setPropType(int pt)
     {
+        // You can only directly set the property type if you own the QQmlPropertyData.
+        // It must not be exposed to other threads before setting the type!
         Q_ASSERT(pt >= 0);
-        Q_ASSERT(pt <= std::numeric_limits<qint16>::max());
-        m_propType = quint16(pt);
+        Q_ASSERT(uint(pt) < PropTypeUnknown);
+        m_propTypeAndRelativePropIndex
+                = (m_propTypeAndRelativePropIndex & RelativePropIndexMask)
+                    | (pt == 0 ? PropTypeUnknown : quint32(pt));
     }
 
     int notifyIndex() const { return m_notifyIndex; }
@@ -336,12 +335,26 @@ public:
     StaticMetaCallFunction staticMetaCallFunction() const { return m_staticMetaCallFunction; }
     void trySetStaticMetaCallFunction(StaticMetaCallFunction f, unsigned relativePropertyIndex)
     {
-        if (relativePropertyIndex < (1 << Flags::BitsLeftInFlags) - 1) {
-            m_flags.otherBits = relativePropertyIndex;
+        if (relativePropertyIndex > std::numeric_limits<quint16>::max())
+            return;
+
+        const quint16 propType = m_propTypeAndRelativePropIndex & PropTypeMask;
+        if (propType > 0) {
+            // We can do this because we know that resolve() has run at this point
+            // and we don't need to synchronize anymore. If we get a 0, that means it hasn't
+            // run or is currently in progress. We don't want to interfer and just go through
+            // the meta object.
+            m_propTypeAndRelativePropIndex
+                    = propType | (relativePropertyIndex << RelativePropIndexShift);
             m_staticMetaCallFunction = f;
         }
     }
-    quint16 relativePropertyIndex() const { Q_ASSERT(hasStaticMetaCallFunction()); return m_flags.otherBits; }
+
+    quint16 relativePropertyIndex() const
+    {
+        Q_ASSERT(hasStaticMetaCallFunction());
+        return m_propTypeAndRelativePropIndex >> 16;
+    }
 
     static Flags flagsForProperty(const QMetaProperty &);
     void load(const QMetaProperty &);
@@ -401,11 +414,17 @@ private:
     friend class QQmlPropertyCache;
     void lazyLoad(const QMetaProperty &);
     void lazyLoad(const QMetaMethod &);
-    bool notFullyResolved() const { return m_flags.notFullyResolved; }
+
+    enum {
+        PropTypeMask           = 0x0000ffff,
+        RelativePropIndexMask  = 0xffff0000,
+        RelativePropIndexShift = 16,
+        PropTypeUnknown        = std::numeric_limits<quint16>::max(),
+    };
+    QAtomicInteger<quint32> m_propTypeAndRelativePropIndex;
 
     Flags m_flags;
     qint16 m_coreIndex = -1;
-    quint16 m_propType = 0;
 
     // The notify index is in the range returned by QObjectPrivate::signalIndex().
     // This is different from QMetaMethod::methodIndex().
@@ -436,8 +455,7 @@ bool QQmlPropertyData::operator==(const QQmlPropertyData &other) const
 }
 
 QQmlPropertyData::Flags::Flags()
-    : otherBits(0)
-    , isConstantORisVMEFunction(false)
+    : isConstantORisVMEFunction(false)
     , isWritableORhasArguments(false)
     , isResettableORisSignal(false)
     , isAliasORisVMESignal(false)
@@ -449,7 +467,6 @@ QQmlPropertyData::Flags::Flags()
     , isDirect(false)
     , isOverridden(false)
     , type(OtherType)
-    , notFullyResolved(false)
     , overrideIndexIsProperty(false)
 {}
 
@@ -465,7 +482,6 @@ bool QQmlPropertyData::Flags::operator==(const QQmlPropertyData::Flags &other) c
             isRequiredORisCloned == other.isRequiredORisCloned &&
             type == other.type &&
             isConstructor == other.isConstructor &&
-            notFullyResolved == other.notFullyResolved &&
             overrideIndexIsProperty == other.overrideIndexIsProperty;
 }