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
Change-Id: I962d28cfd22696aad89a660e41c55f63a8791b44
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit 90d24b807373f7b4c10d1a88ffdb5d4ebed08de8)

7 files changed, 41 insertions, 16 deletions

diff --git a/src/qml/jsapi/qjsengine_p.h b/src/qml/jsapi/qjsengine_p.h
index 7866a5bdda..2605d3b23c 100644
--- a/src/qml/jsapi/qjsengine_p.h
+++ b/src/qml/jsapi/qjsengine_p.h
@@ -110,8 +110,8 @@ public:
     QString uiLanguage;
 
     // These methods may be called from the QML loader thread
-    inline QQmlPropertyCache *cache(QObject *obj, int minorVersion = -1);
-    inline QQmlPropertyCache *cache(const QMetaObject *, int minorVersion = -1);
+    inline QQmlPropertyCache *cache(QObject *obj, int minorVersion = -1, bool doRef = false);
+    inline QQmlPropertyCache *cache(const QMetaObject *, int minorVersion = -1, bool doRef = false);
 };
 
 QJSEnginePrivate::Locker::Locker(const QJSEngine *e)
@@ -161,14 +161,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, int minorVersion)
+QQmlPropertyCache *QJSEnginePrivate::cache(QObject *obj, int minorVersion, 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, minorVersion);
+    return QQmlMetaType::propertyCache(mo, minorVersion, doRef);
 }
 
 /*!
@@ -180,12 +180,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, int minorVersion)
+QQmlPropertyCache *QJSEnginePrivate::cache(const QMetaObject *metaObject, int minorVersion, bool doRef)
 {
     Q_ASSERT(metaObject);
 
     Locker locker(this);
-    return QQmlMetaType::propertyCache(metaObject, minorVersion);
+    return QQmlMetaType::propertyCache(metaObject, minorVersion, doRef);
 }
 
 
diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp
index 872a448bfd..ab26b83bf0 100644
--- a/src/qml/qml/qqmlengine.cpp
+++ b/src/qml/qml/qqmlengine.cpp
@@ -2016,9 +2016,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, -1, true);
     return ddata->propertyCache;
 }
 
diff --git a/src/qml/qml/qqmlmetatype.cpp b/src/qml/qml/qqmlmetatype.cpp
index b2a769cab4..be4a79297c 100644
--- a/src/qml/qml/qqmlmetatype.cpp
+++ b/src/qml/qml/qqmlmetatype.cpp
@@ -1226,10 +1226,14 @@ QQmlType QQmlMetaType::qmlType(const QUrl &unNormalizedUrl, bool includeNonFileI
         return QQmlType();
 }
 
-QQmlPropertyCache *QQmlMetaType::propertyCache(const QMetaObject *metaObject, int minorVersion)
+QQmlPropertyCache *QQmlMetaType::propertyCache(const QMetaObject *metaObject, int minorVersion, bool doRef)
 {
     QQmlMetaTypeDataPtr data; // not const: the cache is created on demand
-    return data->propertyCache(metaObject, minorVersion);
+    auto ret =  data->propertyCache(metaObject, minorVersion);
+    if (doRef)
+        return ret.take();
+    else
+        return ret.data();
 }
 
 QQmlPropertyCache *QQmlMetaType::propertyCache(const QQmlType &type, int minorVersion)
diff --git a/src/qml/qml/qqmlmetatype_p.h b/src/qml/qml/qqmlmetatype_p.h
index 0c5bc043c4..3856e6f121 100644
--- a/src/qml/qml/qqmlmetatype_p.h
+++ b/src/qml/qml/qqmlmetatype_p.h
@@ -115,7 +115,7 @@ public:
     static QQmlType qmlType(int typeId, TypeIdCategory category = TypeIdCategory::MetaType);
     static QQmlType qmlType(const QUrl &unNormalizedUrl, bool includeNonFileImports = false);
 
-    static QQmlPropertyCache *propertyCache(const QMetaObject *metaObject, int minorVersion = -1);
+    static QQmlPropertyCache *propertyCache(const QMetaObject *metaObject, int minorVersion = -1, bool doRef = false);
     static QQmlPropertyCache *propertyCache(const QQmlType &type, int minorVersion);
 
     static void freeUnusedTypesAndCaches();
diff --git a/src/qml/qml/qqmlmetatypedata.cpp b/src/qml/qml/qqmlmetatypedata.cpp
index ed885eaa97..9556dee334 100644
--- a/src/qml/qml/qqmlmetatypedata.cpp
+++ b/src/qml/qml/qqmlmetatypedata.cpp
@@ -109,7 +109,7 @@ void QQmlMetaTypeData::clearPropertyCachesForMinorVersion(int index)
         typePropertyCaches[index].clear();
 }
 
-QQmlPropertyCache *QQmlMetaTypeData::propertyCache(const QMetaObject *metaObject, int minorVersion)
+QQmlRefPointer<QQmlPropertyCache> QQmlMetaTypeData::propertyCache(const QMetaObject *metaObject, int minorVersion)
 {
     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(), minorVersion);
+    auto super = propertyCache(metaObject->superClass(), minorVersion);
     QQmlPropertyCache *rv = super->copyAndAppend(metaObject, minorVersion);
     propertyCaches.insert(metaObject, rv);
     return rv;
@@ -155,7 +155,7 @@ QQmlPropertyCache *QQmlMetaTypeData::propertyCache(const QQmlType &type, int min
         return pc;
     }
 
-    QQmlPropertyCache *raw = propertyCache(type.metaObject(), minorVersion);
+    QQmlPropertyCache *raw = propertyCache(type.metaObject(), minorVersion).data();
 
     bool hasCopied = false;
 
diff --git a/src/qml/qml/qqmlmetatypedata_p.h b/src/qml/qml/qqmlmetatypedata_p.h
index e51d4ca1a4..4c2b2edf67 100644
--- a/src/qml/qml/qqmlmetatypedata_p.h
+++ b/src/qml/qml/qqmlmetatypedata_p.h
@@ -118,7 +118,7 @@ struct QQmlMetaTypeData
     void setPropertyCacheForMinorVersion(int index, int minorVersion, QQmlPropertyCache *cache);
     void clearPropertyCachesForMinorVersion(int index);
 
-    QQmlPropertyCache *propertyCache(const QMetaObject *metaObject, int minorVersion);
+    QQmlRefPointer<QQmlPropertyCache> propertyCache(const QMetaObject *metaObject, int minorVersion);
     QQmlPropertyCache *propertyCache(const QQmlType &type, int minorVersion);
 
     void setTypeRegistrationFailures(QStringList *failures)
diff --git a/tests/auto/qml/qjsengine/tst_qjsengine.cpp b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
index 26737e79c4..3b7d74df63 100644
--- a/tests/auto/qml/qjsengine/tst_qjsengine.cpp
+++ b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
@@ -78,6 +78,7 @@ private slots:
     void newDate();
     void jsParseDate();
     void newQObject();
+    void newQObjectRace();
     void newQObject_ownership();
     void newQObject_deletedEngine();
     void newQObjectPropertyCache();
@@ -784,6 +785,28 @@ void tst_QJSEngine::newQObject()
     }
 }
 
+void tst_QJSEngine::newQObjectRace()
+{
+    class Thread : public QThread
+    {
+        void run() override
+        {
+            for (int i=0;i<100;++i)
+            {
+                QJSEngine e;
+                auto obj = e.newQObject(new QObject);
+            }
+        }
+    };
+
+
+    Thread threads[8];
+    for (auto& t : threads)
+        t.start(); // should not crash
+    for (auto& t : threads)
+        t.wait();
+}
+
 void tst_QJSEngine::newQObject_ownership()
 {
     QJSEngine eng;