diff options
author | Fabian Kosmale <fabian.kosmale@qt.io> | 2020-06-04 10:33:47 +0200 |
---|---|---|
committer | Fabian Kosmale <fabian.kosmale@qt.io> | 2020-06-05 13:02:29 +0200 |
commit | 90d24b807373f7b4c10d1a88ffdb5d4ebed08de8 (patch) | |
tree | 8487cc2f11489a421ae26c8c632d4b6e20dcf5f1 | |
parent | 3d1b34e5bfd56d8035fa53ffb14726e6120f3ff0 (diff) |
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>
-rw-r--r-- | src/qml/jsapi/qjsengine_p.h | 12 | ||||
-rw-r--r-- | src/qml/qml/qqmlengine.cpp | 4 | ||||
-rw-r--r-- | src/qml/qml/qqmlmetatype.cpp | 8 | ||||
-rw-r--r-- | src/qml/qml/qqmlmetatype_p.h | 2 | ||||
-rw-r--r-- | src/qml/qml/qqmlmetatypedata.cpp | 6 | ||||
-rw-r--r-- | src/qml/qml/qqmlmetatypedata_p.h | 2 | ||||
-rw-r--r-- | tests/auto/qml/qjsengine/tst_qjsengine.cpp | 23 |
7 files changed, 41 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) diff --git a/tests/auto/qml/qjsengine/tst_qjsengine.cpp b/tests/auto/qml/qjsengine/tst_qjsengine.cpp index 4414592a6a..95747ef3ab 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(); @@ -777,6 +778,28 @@ void tst_QJSEngine::newQObject() } } +void tst_QJSEngine::newQObjectRace() +{ + class Thread : public QThread + { + void run() override + { + for (int i=0;i<1000;++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; |