From b1f53bf6eaabc9caa4f1eb523ad605c6719c752c Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Tue, 24 Mar 2020 19:31:55 +0100 Subject: QML: Avoid cyclic references between ResolvedTypeRefence and CU The compilation unit keeps a list of references to used types. Usually those are references to types in other compilation units. However, in the case of inline components they can be references to the same compilation unit. In that case we should not addref() the compilation unit, as that forms a cyclic reference. Fixes: QTBUG-82768 Change-Id: I29d28f3a4780aad4fabd4aa2ed02ca0d0108987e Reviewed-by: Simon Hausmann --- src/qml/jsruntime/qv4executablecompilationunit.cpp | 46 +++++++-------- src/qml/jsruntime/qv4executablecompilationunit_p.h | 66 ++++++++++++++++++---- src/qml/qml/qqmlobjectcreator.cpp | 13 +++-- src/qml/qml/qqmlpropertycachecreator_p.h | 2 +- src/qml/qml/qqmltypecompiler.cpp | 4 +- src/qml/qml/qqmltypedata.cpp | 15 +++-- src/qmltest/quicktest.cpp | 10 ++-- 7 files changed, 105 insertions(+), 51 deletions(-) diff --git a/src/qml/jsruntime/qv4executablecompilationunit.cpp b/src/qml/jsruntime/qv4executablecompilationunit.cpp index fa4a1f1ce4..29e1c6dbf7 100644 --- a/src/qml/jsruntime/qv4executablecompilationunit.cpp +++ b/src/qml/jsruntime/qv4executablecompilationunit.cpp @@ -406,9 +406,9 @@ void ExecutableCompilationUnit::finalizeCompositeType(QQmlEnginePrivate *qmlEngi const QV4::CompiledData::Object *obj = objectAt(/*root object*/0); auto *typeRef = resolvedTypes.value(obj->inheritedTypeNameIndex); Q_ASSERT(typeRef); - if (typeRef->compilationUnit) { - metaTypeId = typeRef->compilationUnit->metaTypeId; - listMetaTypeId = typeRef->compilationUnit->listMetaTypeId; + if (const auto compilationUnit = typeRef->compilationUnit()) { + metaTypeId = compilationUnit->metaTypeId; + listMetaTypeId = compilationUnit->listMetaTypeId; } else { metaTypeId = typeRef->type.typeId(); listMetaTypeId = typeRef->type.qListTypeId(); @@ -453,17 +453,17 @@ void ExecutableCompilationUnit::finalizeCompositeType(QQmlEnginePrivate *qmlEngi ++inlineComponentData[lastICRoot].totalParserStatusCount; ++inlineComponentData[lastICRoot].totalObjectCount; - if (typeRef->compilationUnit) { + if (const auto compilationUnit = typeRef->compilationUnit()) { // if the type is an inline component type, we have to extract the information from it // This requires that inline components are visited in the correct order - auto icRoot = typeRef->compilationUnit->icRoot; + auto icRoot = compilationUnit->icRoot; if (typeRef->type.isInlineComponentType()) { icRoot = typeRef->type.inlineComponendId(); } - QScopedValueRollback rollback {typeRef->compilationUnit->icRoot, icRoot}; - inlineComponentData[lastICRoot].totalBindingCount += typeRef->compilationUnit->totalBindingsCount(); - inlineComponentData[lastICRoot].totalParserStatusCount += typeRef->compilationUnit->totalParserStatusCount(); - inlineComponentData[lastICRoot].totalObjectCount += typeRef->compilationUnit->totalObjectCount(); + QScopedValueRollback rollback {compilationUnit->icRoot, icRoot}; + inlineComponentData[lastICRoot].totalBindingCount += compilationUnit->totalBindingsCount(); + inlineComponentData[lastICRoot].totalParserStatusCount += compilationUnit->totalParserStatusCount(); + inlineComponentData[lastICRoot].totalObjectCount += compilationUnit->totalObjectCount(); } } } @@ -481,15 +481,15 @@ void ExecutableCompilationUnit::finalizeCompositeType(QQmlEnginePrivate *qmlEngi if (typeRef->type.isValid() && typeRef->type.parserStatusCast() != -1) ++parserStatusCount; ++objectCount; - if (typeRef->compilationUnit) { - auto icRoot = typeRef->compilationUnit->icRoot; + if (const auto compilationUnit = typeRef->compilationUnit()) { + auto icRoot = compilationUnit->icRoot; if (typeRef->type.isInlineComponentType()) { icRoot = typeRef->type.inlineComponendId(); } - QScopedValueRollback rollback {typeRef->compilationUnit->icRoot, icRoot}; - bindingCount += typeRef->compilationUnit->totalBindingsCount(); - parserStatusCount += typeRef->compilationUnit->totalParserStatusCount(); - objectCount += typeRef->compilationUnit->totalObjectCount(); + QScopedValueRollback rollback {compilationUnit->icRoot, icRoot}; + bindingCount += compilationUnit->totalBindingsCount(); + parserStatusCount += compilationUnit->totalParserStatusCount(); + objectCount += compilationUnit->totalObjectCount(); } } } @@ -806,7 +806,7 @@ QQmlRefPointer ResolvedTypeReference::propertyCache() const if (type.isValid()) return typePropertyCache; else - return compilationUnit->rootPropertyCache(); + return m_compilationUnit->rootPropertyCache(); } /*! @@ -820,8 +820,8 @@ QQmlRefPointer ResolvedTypeReference::createPropertyCache(QQm typePropertyCache = QQmlEnginePrivate::get(engine)->cache(type.metaObject(), minorVersion); return typePropertyCache; } else { - Q_ASSERT(compilationUnit); - return compilationUnit->rootPropertyCache(); + Q_ASSERT(m_compilationUnit); + return m_compilationUnit->rootPropertyCache(); } } @@ -832,10 +832,10 @@ bool ResolvedTypeReference::addToHash(QCryptographicHash *hash, QQmlEngine *engi hash->addData(createPropertyCache(engine)->checksum(&ok)); return ok; } - if (!compilationUnit) + if (!m_compilationUnit) return false; - hash->addData(compilationUnit->data->md5Checksum, - sizeof(compilationUnit->data->md5Checksum)); + hash->addData(m_compilationUnit->data->md5Checksum, + sizeof(m_compilationUnit->data->md5Checksum)); return true; } @@ -856,8 +856,8 @@ void ResolvedTypeReference::doDynamicTypeCheck() mo = typePropertyCache->firstCppMetaObject(); else if (type.isValid()) mo = type.metaObject(); - else if (compilationUnit) - mo = compilationUnit->rootPropertyCache()->firstCppMetaObject(); + else if (m_compilationUnit) + mo = m_compilationUnit->rootPropertyCache()->firstCppMetaObject(); isFullyDynamicType = qtTypeInherits(mo); } diff --git a/src/qml/jsruntime/qv4executablecompilationunit_p.h b/src/qml/jsruntime/qv4executablecompilationunit_p.h index 8cad18a3dc..7bfb68da9d 100644 --- a/src/qml/jsruntime/qv4executablecompilationunit_p.h +++ b/src/qml/jsruntime/qv4executablecompilationunit_p.h @@ -325,27 +325,73 @@ private: struct ResolvedTypeReference { +public: ResolvedTypeReference() - : majorVersion(0) - , minorVersion(0) - , isFullyDynamicType(false) + : m_compilationUnit(nullptr) + , m_stronglyReferencesCompilationUnit(true) + , majorVersion(0) + , minorVersion(0) + , isFullyDynamicType(false) {} + ~ResolvedTypeReference() + { + if (m_stronglyReferencesCompilationUnit && m_compilationUnit) + m_compilationUnit->release(); + } + + QQmlRefPointer compilationUnit() { return m_compilationUnit; } + void setCompilationUnit(QQmlRefPointer unit) + { + if (m_compilationUnit == unit.data()) + return; + if (m_stronglyReferencesCompilationUnit) { + if (m_compilationUnit) + m_compilationUnit->release(); + m_compilationUnit = unit.take(); + } else { + m_compilationUnit = unit.data(); + } + } + + bool referencesCompilationUnit() const { return m_stronglyReferencesCompilationUnit; } + void setReferencesCompilationUnit(bool doReference) + { + if (doReference == m_stronglyReferencesCompilationUnit) + return; + m_stronglyReferencesCompilationUnit = doReference; + if (!m_compilationUnit) + return; + if (doReference) { + m_compilationUnit->addref(); + } else if (m_compilationUnit->count() == 1) { + m_compilationUnit->release(); + m_compilationUnit = nullptr; + } else { + m_compilationUnit->release(); + } + } + + QQmlRefPointer propertyCache() const; + QQmlRefPointer createPropertyCache(QQmlEngine *); + bool addToHash(QCryptographicHash *hash, QQmlEngine *engine); + + void doDynamicTypeCheck(); + QQmlType type; QQmlRefPointer typePropertyCache; - QQmlRefPointer compilationUnit; +private: + Q_DISABLE_COPY_MOVE(ResolvedTypeReference) + + QV4::ExecutableCompilationUnit *m_compilationUnit; + bool m_stronglyReferencesCompilationUnit; +public: int majorVersion; int minorVersion; // Types such as QQmlPropertyMap can add properties dynamically at run-time and // therefore cannot have a property cache installed when instantiated. bool isFullyDynamicType; - - QQmlRefPointer propertyCache() const; - QQmlRefPointer createPropertyCache(QQmlEngine *); - bool addToHash(QCryptographicHash *hash, QQmlEngine *engine); - - void doDynamicTypeCheck(); }; IdentifierHash ExecutableCompilationUnit::namedObjectsPerComponent(int componentObjectIndex) diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp index 76f387bbdc..3ca89924f9 100644 --- a/src/qml/qml/qqmlobjectcreator.cpp +++ b/src/qml/qml/qqmlobjectcreator.cpp @@ -1214,11 +1214,12 @@ QObject *QQmlObjectCreator::createInstance(int index, QObject *parent, bool isCo sharedState->allCreatedObjects.push(instance); } else { - Q_ASSERT(typeRef->compilationUnit); - typeName = typeRef->compilationUnit->fileName(); + const auto compilationUnit = typeRef->compilationUnit(); + Q_ASSERT(compilationUnit); + typeName = compilationUnit->fileName(); // compilation unit is shared between root type and its inline component types // so isSingleton errorneously returns true for inline components - if (typeRef->compilationUnit->unitData()->isSingleton() && !type.isInlineComponentType()) + if (compilationUnit->unitData()->isSingleton() && !type.isInlineComponentType()) { recordError(obj->location, tr("Composite Singleton Type %1 is not creatable").arg(stringAt(obj->inheritedTypeNameIndex))); return nullptr; @@ -1226,7 +1227,7 @@ QObject *QQmlObjectCreator::createInstance(int index, QObject *parent, bool isCo if (!type.isInlineComponentType()) { - QQmlObjectCreator subCreator(context, typeRef->compilationUnit, sharedState.data()); + QQmlObjectCreator subCreator(context, compilationUnit, sharedState.data()); instance = subCreator.create(); if (!instance) { errors += subCreator.errors; @@ -1234,8 +1235,8 @@ QObject *QQmlObjectCreator::createInstance(int index, QObject *parent, bool isCo } } else { int subObjectId = type.inlineComponendId(); - QScopedValueRollback rollback {typeRef->compilationUnit->icRoot, subObjectId}; - QQmlObjectCreator subCreator(context, typeRef->compilationUnit, sharedState.data()); + QScopedValueRollback rollback {compilationUnit->icRoot, subObjectId}; + QQmlObjectCreator subCreator(context, compilationUnit, sharedState.data()); instance = subCreator.create(subObjectId, nullptr, nullptr, CreationFlags::InlineComponent); if (!instance) { errors += subCreator.errors; diff --git a/src/qml/qml/qqmlpropertycachecreator_p.h b/src/qml/qml/qqmlpropertycachecreator_p.h index 5669f9e13d..6b02d6fb98 100644 --- a/src/qml/qml/qqmlpropertycachecreator_p.h +++ b/src/qml/qml/qqmlpropertycachecreator_p.h @@ -840,7 +840,7 @@ inline QQmlError QQmlPropertyCacheAliasCreator::propertyDataFor if (typeRef->type.isValid()) *type = typeRef->type.typeId(); else - *type = typeRef->compilationUnit->metaTypeId; + *type = typeRef->compilationUnit()->metaTypeId; *minorVersion = typeRef->minorVersion; diff --git a/src/qml/qml/qqmltypecompiler.cpp b/src/qml/qml/qqmltypecompiler.cpp index f4737ba99d..058bf8848c 100644 --- a/src/qml/qml/qqmltypecompiler.cpp +++ b/src/qml/qml/qqmltypecompiler.cpp @@ -813,8 +813,8 @@ void QQmlComponentAndAliasResolver::findAndRegisterImplicitComponents(const QmlI const QMetaObject *firstMetaObject = nullptr; if (tr->type.isValid()) firstMetaObject = tr->type.metaObject(); - else if (tr->compilationUnit) - firstMetaObject = tr->compilationUnit->rootPropertyCache()->firstCppMetaObject(); + else if (const auto compilationUnit = tr->compilationUnit()) + firstMetaObject = compilationUnit->rootPropertyCache()->firstCppMetaObject(); if (isUsableComponent(firstMetaObject)) continue; // if here, not a QQmlComponent, so needs wrapping diff --git a/src/qml/qml/qqmltypedata.cpp b/src/qml/qml/qqmltypedata.cpp index cd4e0acf39..fc1d0cfbcf 100644 --- a/src/qml/qml/qqmltypedata.cpp +++ b/src/qml/qml/qqmltypedata.cpp @@ -300,7 +300,14 @@ void QQmlTypeData::setCompileUnit(const Container &container) auto const root = container->objectAt(i); for (auto it = root->inlineComponentsBegin(); it != root->inlineComponentsEnd(); ++it) { auto *typeRef = m_compiledData->resolvedType(it->nameIndex); - typeRef->compilationUnit = m_compiledData; // share compilation unit + + // We don't want the type reference to keep a strong reference to the compilation unit + // here. The compilation unit owns the type reference, and having a strong reference + // would prevent the compilation unit from ever getting deleted. We can still be sure + // that the compilation unit outlives the type reference, due to ownership. + typeRef->setReferencesCompilationUnit(false); + + typeRef->setCompilationUnit(m_compiledData); // share compilation unit } } } @@ -919,7 +926,7 @@ QQmlError QQmlTypeData::buildTypeResolutionCaches( if (resolvedType->needsCreation && qmlType.isCompositeSingleton()) { return qQmlCompileError(resolvedType->location, tr("Composite Singleton Type %1 is not creatable.").arg(qmlType.qmlTypeName())); } - ref->compilationUnit = resolvedType->typeData->compilationUnit(); + ref->setCompilationUnit(resolvedType->typeData->compilationUnit()); if (resolvedType->type.isInlineComponentType()) { // Inline component which is part of an already resolved type int objectId = -1; @@ -943,12 +950,12 @@ QQmlError QQmlTypeData::buildTypeResolutionCaches( auto typeID = qmlType.lookupInlineComponentById(qmlType.inlineComponendId()).typeId(); auto exUnit = engine->obtainExecutableCompilationUnit(typeID); if (exUnit) { - ref->compilationUnit = exUnit; + ref->setCompilationUnit(exUnit); ref->typePropertyCache = engine->propertyCacheForType(typeID); } } } else { - ref->compilationUnit = m_inlineComponentToCompiledData[resolvedType.key()]; + ref->setCompilationUnit(m_inlineComponentToCompiledData[resolvedType.key()]); ref->typePropertyCache = m_inlineComponentToCompiledData[resolvedType.key()]->rootPropertyCache(); } diff --git a/src/qmltest/quicktest.cpp b/src/qmltest/quicktest.cpp index 470b3c0f7a..443496c9bc 100644 --- a/src/qmltest/quicktest.cpp +++ b/src/qmltest/quicktest.cpp @@ -332,8 +332,9 @@ private: } }; - TestCaseEnumerationResult enumerateTestCases(QV4::ExecutableCompilationUnit *compilationUnit, - const Object *object = nullptr) + TestCaseEnumerationResult enumerateTestCases( + const QQmlRefPointer &compilationUnit, + const Object *object = nullptr) { QQmlType testCaseType; for (quint32 i = 0, count = compilationUnit->importCount(); i < count; ++i) { @@ -356,9 +357,8 @@ private: if (!object) // Start at root of compilation unit if not enumerating a specific child object = compilationUnit->objectAt(0); - if (QV4::ExecutableCompilationUnit *superTypeUnit - = compilationUnit->resolvedTypes.value(object->inheritedTypeNameIndex) - ->compilationUnit.data()) { + if (const auto superTypeUnit = compilationUnit->resolvedTypes.value( + object->inheritedTypeNameIndex)->compilationUnit()) { // We have a non-C++ super type, which could indicate we're a subtype of a TestCase if (testCaseType.isValid() && superTypeUnit->url() == testCaseType.sourceUrl()) result.isTestCase = true; -- cgit v1.2.3