Fix race condition in QQmlData::createPropertyCache

As noted in QJSEnginePrivate::cache, there can be a race  between
calling addRef on the QQmlPropertyCache and another thread derefing and
consequently deleting it. To avoid this, we introduce a doRef flag in
QQmlMetaTypeData::propertyCache, which tells it to ref the the cache.
This fixes the issue, as the QQmlMetaTypeDataPtr in propertyCache() acts
as a mutex.

Fixes: QTBUG-84692
Pick-to: 5.15
Change-Id: I962d28cfd22696aad89a660e41c55f63a8791b44
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

6 files changed, 18 insertions, 16 deletions

diff --git a/src/qml/jsapi/qjsengine_p.h b/src/qml/jsapi/qjsengine_p.h
index 0aff6595b6..fb88781852 100644
--- a/src/qml/jsapi/qjsengine_p.h
+++ b/src/qml/jsapi/qjsengine_p.h
@@ -111,8 +111,8 @@ public:
     QProperty<QString> uiLanguage;
 
     // These methods may be called from the QML loader thread
-    inline QQmlPropertyCache *cache(QObject *obj, QTypeRevision version = QTypeRevision());
-    inline QQmlPropertyCache *cache(const QMetaObject *obj, QTypeRevision version = QTypeRevision());
+    inline QQmlPropertyCache *cache(QObject *obj, QTypeRevision version = QTypeRevision(), bool doRef = false);
+    inline QQmlPropertyCache *cache(const QMetaObject *obj, QTypeRevision version = QTypeRevision(), bool doRef = false);
 };
 
 QJSEnginePrivate::Locker::Locker(const QJSEngine *e)
@@ -162,14 +162,14 @@ and deleted before the loader thread has a chance to use or reference it.  This
 can't currently happen as the cache holds a reference to the
 QQmlPropertyCache until the QQmlEngine is destroyed.
 */
-QQmlPropertyCache *QJSEnginePrivate::cache(QObject *obj, QTypeRevision version)
+QQmlPropertyCache *QJSEnginePrivate::cache(QObject *obj, QTypeRevision version, bool doRef)
 {
     if (!obj || QObjectPrivate::get(obj)->metaObject || QObjectPrivate::get(obj)->wasDeleted)
         return nullptr;
 
     Locker locker(this);
     const QMetaObject *mo = obj->metaObject();
-    return QQmlMetaType::propertyCache(mo, version);
+    return QQmlMetaType::propertyCache(mo, version, doRef);
 }
 
 /*!
@@ -181,12 +181,12 @@ exist for the lifetime of the QQmlEngine.
 
 The returned cache is not referenced, so if it is to be stored, call addref().
 */
-QQmlPropertyCache *QJSEnginePrivate::cache(const QMetaObject *metaObject, QTypeRevision version)
+QQmlPropertyCache *QJSEnginePrivate::cache(const QMetaObject *metaObject, QTypeRevision version, bool doRef)
 {
     Q_ASSERT(metaObject);
 
     Locker locker(this);
-    return QQmlMetaType::propertyCache(metaObject, version);
+    return QQmlMetaType::propertyCache(metaObject, version, doRef);
 }
 
 
diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp
index 0692124dfe..1f203fae88 100644
--- a/src/qml/qml/qqmlengine.cpp
+++ b/src/qml/qml/qqmlengine.cpp
@@ -1947,9 +1947,7 @@ QQmlData *QQmlData::createQQmlData(QObjectPrivate *priv)
 QQmlPropertyCache *QQmlData::createPropertyCache(QJSEngine *engine, QObject *object)
 {
     QQmlData *ddata = QQmlData::get(object, /*create*/true);
-    ddata->propertyCache = QJSEnginePrivate::get(engine)->cache(object);
-    if (ddata->propertyCache)
-        ddata->propertyCache->addref();
+    ddata->propertyCache = QJSEnginePrivate::get(engine)->cache(object, QTypeRevision {}, true);
     return ddata->propertyCache;
 }
 
diff --git a/src/qml/qml/qqmlmetatype.cpp b/src/qml/qml/qqmlmetatype.cpp
index 761016669c..61e8e323c6 100644
--- a/src/qml/qml/qqmlmetatype.cpp
+++ b/src/qml/qml/qqmlmetatype.cpp
@@ -1239,10 +1239,14 @@ QQmlType QQmlMetaType::qmlType(const QUrl &unNormalizedUrl, bool includeNonFileI
         return QQmlType();
 }
 
-QQmlPropertyCache *QQmlMetaType::propertyCache(const QMetaObject *metaObject, QTypeRevision version)
+QQmlPropertyCache *QQmlMetaType::propertyCache(const QMetaObject *metaObject, QTypeRevision version, bool doRef)
 {
     QQmlMetaTypeDataPtr data; // not const: the cache is created on demand
-    return data->propertyCache(metaObject, version);
+    auto ret =  data->propertyCache(metaObject, version);
+    if (doRef)
+        return ret.take();
+    else
+        return ret.data();
 }
 
 QQmlPropertyCache *QQmlMetaType::propertyCache(const QQmlType &type, QTypeRevision version)
diff --git a/src/qml/qml/qqmlmetatype_p.h b/src/qml/qml/qqmlmetatype_p.h
index 0ac86e7d20..08d79d2e5b 100644
--- a/src/qml/qml/qqmlmetatype_p.h
+++ b/src/qml/qml/qqmlmetatype_p.h
@@ -114,7 +114,7 @@ public:
     static QQmlType qmlType(const QUrl &unNormalizedUrl, bool includeNonFileImports = false);
 
     static QQmlPropertyCache *propertyCache(const QMetaObject *metaObject,
-                                            QTypeRevision version = QTypeRevision());
+                                            QTypeRevision version = QTypeRevision(), bool doRef = false);
     static QQmlPropertyCache *propertyCache(const QQmlType &type, QTypeRevision version);
 
     static void freeUnusedTypesAndCaches();
diff --git a/src/qml/qml/qqmlmetatypedata.cpp b/src/qml/qml/qqmlmetatypedata.cpp
index a34a0c1ae4..0d1d06abdd 100644
--- a/src/qml/qml/qqmlmetatypedata.cpp
+++ b/src/qml/qml/qqmlmetatypedata.cpp
@@ -109,7 +109,7 @@ void QQmlMetaTypeData::clearPropertyCachesForVersion(int index)
         typePropertyCaches[index].clear();
 }
 
-QQmlPropertyCache *QQmlMetaTypeData::propertyCache(const QMetaObject *metaObject, QTypeRevision version)
+QQmlRefPointer<QQmlPropertyCache> QQmlMetaTypeData::propertyCache(const QMetaObject *metaObject, QTypeRevision version)
 {
     if (QQmlPropertyCache *rv = propertyCaches.value(metaObject))
         return rv;
@@ -119,7 +119,7 @@ QQmlPropertyCache *QQmlMetaTypeData::propertyCache(const QMetaObject *metaObject
         propertyCaches.insert(metaObject, rv);
         return rv;
     }
-    QQmlPropertyCache *super = propertyCache(metaObject->superClass(), version);
+    auto super = propertyCache(metaObject->superClass(), version);
     QQmlPropertyCache *rv = super->copyAndAppend(metaObject, version);
     propertyCaches.insert(metaObject, rv);
     return rv;
@@ -164,7 +164,7 @@ QQmlPropertyCache *QQmlMetaTypeData::propertyCache(const QQmlType &type, QTypeRe
         return pc;
     }
 
-    QQmlPropertyCache *raw = propertyCache(type.metaObject(), combinedVersion);
+    QQmlPropertyCache *raw = propertyCache(type.metaObject(), combinedVersion).data();
 
     bool hasCopied = false;
 
diff --git a/src/qml/qml/qqmlmetatypedata_p.h b/src/qml/qml/qqmlmetatypedata_p.h
index 727e57a6de..e7e086fb4c 100644
--- a/src/qml/qml/qqmlmetatypedata_p.h
+++ b/src/qml/qml/qqmlmetatypedata_p.h
@@ -116,7 +116,7 @@ struct QQmlMetaTypeData
     void setPropertyCacheForVersion(int index, QTypeRevision version, QQmlPropertyCache *cache);
     void clearPropertyCachesForVersion(int index);
 
-    QQmlPropertyCache *propertyCache(const QMetaObject *metaObject, QTypeRevision version);
+    QQmlRefPointer<QQmlPropertyCache> propertyCache(const QMetaObject *metaObject, QTypeRevision version);
     QQmlPropertyCache *propertyCache(const QQmlType &type, QTypeRevision version);
 
     void setTypeRegistrationFailures(QStringList *failures)