Clean up manual reference of QQmlTypeData and QQmlPropertyCache

We have a few places in the type loader where we do adventurous manual
reference counting, where getType() returns a raw pointer that has been
addref()'ed and then sometimes somehow we call release() later. Commit
0b394e30bba4f6bb7e6f7dbe5585a2e15aa0f21d is an example of where this can
easily go wrong. As a consequence and also in preparation for future
work on the type loader, this patch starts replacing the manual
reference counting there.

Changing the return type from QQmlTypeData *getType() to a
QQmlRefPointer<> itself is not sufficient though, as the implicit
operator T*() will still allow the caller to store the result as a raw
pointer. Therefore this patch removes the "unsafe" implicit extraction
operator.

As a result of that change, other types that are sometimes stored in
QQmlRefPointer are also affected and their usage needs to be adapted
to QQmlRefPointer usage or manual raw pointer extraction with .data().

Change-Id: I18fd40634047f13196a237f4e6766cbef3bfbea2
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

8 files changed, 28 insertions, 34 deletions

diff --git a/src/qml/compiler/qqmlirbuilder_p.h b/src/qml/compiler/qqmlirbuilder_p.h
index 14f59dd7e8..1c1ffeb8c9 100644
--- a/src/qml/compiler/qqmlirbuilder_p.h
+++ b/src/qml/compiler/qqmlirbuilder_p.h
@@ -576,7 +576,7 @@ private:
 #ifndef V4_BOOTSTRAP
 struct Q_QML_EXPORT PropertyResolver
 {
-    PropertyResolver(const QQmlPropertyCache *cache)
+    PropertyResolver(const QQmlRefPointer<QQmlPropertyCache> &cache)
         : cache(cache)
     {}
 
@@ -595,7 +595,7 @@ struct Q_QML_EXPORT PropertyResolver
     // This code must match the semantics of QQmlPropertyPrivate::findSignalByName
     QQmlPropertyData *signal(const QString &name, bool *notInRevision) const;
 
-    const QQmlPropertyCache *cache;
+    QQmlRefPointer<QQmlPropertyCache> cache;
 };
 #endif
 
diff --git a/src/qml/compiler/qqmlpropertycachecreator_p.h b/src/qml/compiler/qqmlpropertycachecreator_p.h
index 8bbc8291b4..02517ea6bb 100644
--- a/src/qml/compiler/qqmlpropertycachecreator_p.h
+++ b/src/qml/compiler/qqmlpropertycachecreator_p.h
@@ -99,8 +99,8 @@ public:
 
 protected:
     QQmlCompileError buildMetaObjectRecursively(int objectIndex, const QQmlBindingInstantiationContext &context);
-    QQmlPropertyCache *propertyCacheForObject(const CompiledObject *obj, const QQmlBindingInstantiationContext &context, QQmlCompileError *error) const;
-    QQmlCompileError createMetaObject(int objectIndex, const CompiledObject *obj, QQmlPropertyCache *baseTypeCache);
+    QQmlRefPointer<QQmlPropertyCache> propertyCacheForObject(const CompiledObject *obj, const QQmlBindingInstantiationContext &context, QQmlCompileError *error) const;
+    QQmlCompileError createMetaObject(int objectIndex, const CompiledObject *obj, const QQmlRefPointer<QQmlPropertyCache> &baseTypeCache);
 
     QString stringAt(int index) const { return objectContainer->stringAt(index); }
 
@@ -152,7 +152,7 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::buildMetaObje
                         const CompiledObject *obj = objectContainer->objectAt(context.referencingObjectIndex);
                         auto *typeRef = objectContainer->resolvedTypes.value(obj->inheritedTypeNameIndex);
                         Q_ASSERT(typeRef);
-                        QQmlPropertyCache *baseTypeCache = typeRef->createPropertyCache(QQmlEnginePrivate::get(enginePrivate));
+                        QQmlRefPointer<QQmlPropertyCache> baseTypeCache = typeRef->createPropertyCache(QQmlEnginePrivate::get(enginePrivate));
                         QQmlCompileError error = createMetaObject(context.referencingObjectIndex, obj, baseTypeCache);
                         if (error.isSet())
                             return error;
@@ -166,7 +166,7 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::buildMetaObje
         }
     }
 
-    QQmlPropertyCache *baseTypeCache;
+    QQmlRefPointer<QQmlPropertyCache> baseTypeCache;
     {
         QQmlCompileError error;
         baseTypeCache = propertyCacheForObject(obj, context, &error);
@@ -209,7 +209,7 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::buildMetaObje
 }
 
 template <typename ObjectContainer>
-inline QQmlPropertyCache *QQmlPropertyCacheCreator<ObjectContainer>::propertyCacheForObject(const CompiledObject *obj, const QQmlBindingInstantiationContext &context, QQmlCompileError *error) const
+inline QQmlRefPointer<QQmlPropertyCache> QQmlPropertyCacheCreator<ObjectContainer>::propertyCacheForObject(const CompiledObject *obj, const QQmlBindingInstantiationContext &context, QQmlCompileError *error) const
 {
     if (context.instantiatingProperty) {
         return context.instantiatingPropertyCache(enginePrivate);
@@ -241,14 +241,12 @@ inline QQmlPropertyCache *QQmlPropertyCacheCreator<ObjectContainer>::propertyCac
             QString propertyName = stringAt(context.instantiatingBinding->propertyNameIndex);
             if (imports->resolveType(propertyName, &qmltype, nullptr, nullptr, nullptr)) {
                 if (qmltype.isComposite()) {
-                    QQmlTypeData *tdata = enginePrivate->typeLoader.getType(qmltype.sourceUrl());
+                    QQmlRefPointer<QQmlTypeData> tdata = enginePrivate->typeLoader.getType(qmltype.sourceUrl());
                     Q_ASSERT(tdata);
                     Q_ASSERT(tdata->isComplete());
 
                     auto compilationUnit = tdata->compilationUnit();
                     qmltype = QQmlMetaType::qmlType(compilationUnit->metaTypeId);
-
-                    tdata->release();
                 }
             }
         }
@@ -264,7 +262,7 @@ inline QQmlPropertyCache *QQmlPropertyCacheCreator<ObjectContainer>::propertyCac
 }
 
 template <typename ObjectContainer>
-inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject(int objectIndex, const CompiledObject *obj, QQmlPropertyCache *baseTypeCache)
+inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject(int objectIndex, const CompiledObject *obj, const QQmlRefPointer<QQmlPropertyCache> &baseTypeCache)
 {
     QQmlRefPointer<QQmlPropertyCache> cache;
     cache.adopt(baseTypeCache->copyAndReserve(obj->propertyCount() + obj->aliasCount(),
@@ -314,7 +312,7 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObj
         }
     }
     if (newClassName.isEmpty()) {
-        newClassName = QQmlMetaObject(baseTypeCache).className();
+        newClassName = QQmlMetaObject(baseTypeCache.data()).className();
         newClassName.append("_QML_");
         newClassName.append(QByteArray::number(classIndexCounter.fetchAndAddRelaxed(1)));
     }
@@ -354,7 +352,7 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObj
     // and throw an error if there is a signal/method defined as an override.
     QSet<QString> seenSignals;
     seenSignals << QStringLiteral("destroyed") << QStringLiteral("parentChanged") << QStringLiteral("objectNameChanged");
-    QQmlPropertyCache *parentCache = cache;
+    QQmlPropertyCache *parentCache = cache.data();
     while ((parentCache = parentCache->parent())) {
         if (int pSigCount = parentCache->signalCount()) {
             int pSigOffset = parentCache->signalOffset();
@@ -440,15 +438,13 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObj
                         return QQmlCompileError(s->location, QQmlPropertyCacheCreatorBase::tr("Invalid signal parameter type: %1").arg(customTypeName));
 
                     if (qmltype.isComposite()) {
-                        QQmlTypeData *tdata = enginePrivate->typeLoader.getType(qmltype.sourceUrl());
+                        QQmlRefPointer<QQmlTypeData> tdata = enginePrivate->typeLoader.getType(qmltype.sourceUrl());
                         Q_ASSERT(tdata);
                         Q_ASSERT(tdata->isComplete());
 
                         auto compilationUnit = tdata->compilationUnit();
 
                         paramTypes[i + 1] = compilationUnit->metaTypeId;
-
-                        tdata->release();
                     } else {
                         paramTypes[i + 1] = qmltype.typeId();
                     }
@@ -523,7 +519,7 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObj
 
             Q_ASSERT(qmltype.isValid());
             if (qmltype.isComposite()) {
-                QQmlTypeData *tdata = enginePrivate->typeLoader.getType(qmltype.sourceUrl());
+                QQmlRefPointer<QQmlTypeData> tdata = enginePrivate->typeLoader.getType(qmltype.sourceUrl());
                 Q_ASSERT(tdata);
                 Q_ASSERT(tdata->isComplete());
 
@@ -534,8 +530,6 @@ inline QQmlCompileError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObj
                 } else {
                     propertyType = compilationUnit->listMetaTypeId;
                 }
-
-                tdata->release();
             } else {
                 if (p->type == QV4::CompiledData::Property::Custom) {
                     propertyType = qmltype.typeId();
diff --git a/src/qml/compiler/qqmlpropertyvalidator.cpp b/src/qml/compiler/qqmlpropertyvalidator.cpp
index ffd3b5975a..a8f589e620 100644
--- a/src/qml/compiler/qqmlpropertyvalidator.cpp
+++ b/src/qml/compiler/qqmlpropertyvalidator.cpp
@@ -45,8 +45,9 @@
 
 QT_BEGIN_NAMESPACE
 
-QQmlPropertyValidator::QQmlPropertyValidator(QQmlEnginePrivate *enginePrivate, const QQmlImports &imports, QV4::CompiledData::CompilationUnit *compilationUnit)
+QQmlPropertyValidator::QQmlPropertyValidator(QQmlEnginePrivate *enginePrivate, const QQmlImports &imports, const QQmlRefPointer<QV4::CompiledData::CompilationUnit> &compilationUnit)
     : enginePrivate(enginePrivate)
+    , compilationUnit(compilationUnit)
     , imports(imports)
     , qmlUnit(compilationUnit->data)
     , resolvedTypes(compilationUnit->resolvedTypes)
@@ -629,7 +630,7 @@ QQmlCompileError QQmlPropertyValidator::validateObjectBinding(QQmlPropertyData *
 
         const QV4::CompiledData::Object *targetObject = qmlUnit->objectAt(binding->value.objectIndex);
         if (auto *typeRef = resolvedTypes.value(targetObject->inheritedTypeNameIndex)) {
-            QQmlPropertyCache *cache = typeRef->createPropertyCache(QQmlEnginePrivate::get(enginePrivate));
+            QQmlRefPointer<QQmlPropertyCache> cache = typeRef->createPropertyCache(QQmlEnginePrivate::get(enginePrivate));
             const QMetaObject *mo = cache->firstCppMetaObject();
             QQmlType qmlType;
             while (mo && !qmlType.isValid()) {
diff --git a/src/qml/compiler/qqmlpropertyvalidator_p.h b/src/qml/compiler/qqmlpropertyvalidator_p.h
index e37b8141f4..a8f75a0a7e 100644
--- a/src/qml/compiler/qqmlpropertyvalidator_p.h
+++ b/src/qml/compiler/qqmlpropertyvalidator_p.h
@@ -58,7 +58,7 @@ class QQmlPropertyValidator
 {
     Q_DECLARE_TR_FUNCTIONS(QQmlPropertyValidator)
 public:
-    QQmlPropertyValidator(QQmlEnginePrivate *enginePrivate, const QQmlImports &imports, QV4::CompiledData::CompilationUnit *compilationUnit);
+    QQmlPropertyValidator(QQmlEnginePrivate *enginePrivate, const QQmlImports &imports, const QQmlRefPointer<QV4::CompiledData::CompilationUnit> &compilationUnit);
 
     QVector<QQmlCompileError> validate();
 
@@ -74,6 +74,7 @@ private:
     QString stringAt(int index) const { return qmlUnit->stringAt(index); }
 
     QQmlEnginePrivate *enginePrivate;
+    QQmlRefPointer<QV4::CompiledData::CompilationUnit> compilationUnit;
     const QQmlImports &imports;
     const QV4::CompiledData::Unit *qmlUnit;
     const QV4::CompiledData::ResolvedTypeReferenceMap &resolvedTypes;
diff --git a/src/qml/compiler/qqmltypecompiler.cpp b/src/qml/compiler/qqmltypecompiler.cpp
index 34c2c3ffc9..d1e4041b7e 100644
--- a/src/qml/compiler/qqmltypecompiler.cpp
+++ b/src/qml/compiler/qqmltypecompiler.cpp
@@ -67,7 +67,7 @@ QQmlTypeCompiler::QQmlTypeCompiler(QQmlEnginePrivate *engine, QQmlTypeData *type
 {
 }
 
-QV4::CompiledData::CompilationUnit *QQmlTypeCompiler::compile()
+QQmlRefPointer<QV4::CompiledData::CompilationUnit> QQmlTypeCompiler::compile()
 {
     // Build property caches and VME meta object data
 
@@ -147,7 +147,7 @@ QV4::CompiledData::CompilationUnit *QQmlTypeCompiler::compile()
         document->jsModule.fileName = typeData->urlString();
         document->jsModule.finalUrl = typeData->finalUrlString();
         QmlIR::JSCodeGen v4CodeGenerator(document->code, &document->jsGenerator, &document->jsModule, &document->jsParserEngine,
-                                         document->program, typeNameCache, &document->jsGenerator.stringTable, engine->v8engine()->illegalNames());
+                                         document->program, typeNameCache.data(), &document->jsGenerator.stringTable, engine->v8engine()->illegalNames());
         v4CodeGenerator.setUseFastLookups(false);
         QQmlJSCodeGenerator jsCodeGen(this, &v4CodeGenerator);
         if (!jsCodeGen.generateCodeForComponents())
@@ -165,7 +165,7 @@ QV4::CompiledData::CompilationUnit *QQmlTypeCompiler::compile()
     // The js unit owns the data and will free the qml unit.
     document->javaScriptCompilationUnit->data = qmlUnit;
 
-    QV4::CompiledData::CompilationUnit *compilationUnit = document->javaScriptCompilationUnit;
+    QQmlRefPointer<QV4::CompiledData::CompilationUnit> compilationUnit = document->javaScriptCompilationUnit;
     compilationUnit = document->javaScriptCompilationUnit;
     compilationUnit->typeNameCache = typeNameCache;
     compilationUnit->resolvedTypes = resolvedTypes;
@@ -339,14 +339,12 @@ bool SignalHandlerConverter::convertSignalHandlerExpressionsToFunctionDeclaratio
             if (!type.isValid()) {
                 if (imports->resolveType(propertyName, &type, nullptr, nullptr, nullptr)) {
                     if (type.isComposite()) {
-                        QQmlTypeData *tdata = enginePrivate->typeLoader.getType(type.sourceUrl());
+                        QQmlRefPointer<QQmlTypeData> tdata = enginePrivate->typeLoader.getType(type.sourceUrl());
                         Q_ASSERT(tdata);
                         Q_ASSERT(tdata->isComplete());
 
                         auto compilationUnit = tdata->compilationUnit();
                         type = QQmlMetaType::qmlType(compilationUnit->metaTypeId);
-
-                        tdata->release();
                     }
                 }
             }
diff --git a/src/qml/compiler/qqmltypecompiler_p.h b/src/qml/compiler/qqmltypecompiler_p.h
index b8eddcb9b2..537f87ab4c 100644
--- a/src/qml/compiler/qqmltypecompiler_p.h
+++ b/src/qml/compiler/qqmltypecompiler_p.h
@@ -92,7 +92,7 @@ public:
     QV4::CompiledData::ResolvedTypeReferenceMap resolvedTypes;
     // ---
 
-    QV4::CompiledData::CompilationUnit *compile();
+    QQmlRefPointer<QV4::CompiledData::CompilationUnit> compile();
 
     QList<QQmlError> compilationErrors() const { return errors; }
     void recordError(QQmlError error);
diff --git a/src/qml/compiler/qv4compileddata.cpp b/src/qml/compiler/qv4compileddata.cpp
index a17ee43446..52783e9970 100644
--- a/src/qml/compiler/qv4compileddata.cpp
+++ b/src/qml/compiler/qv4compileddata.cpp
@@ -643,7 +643,7 @@ QString Binding::valueAsScriptString(const Unit *unit) const
 /*!
 Returns the property cache, if one alread exists.  The cache is not referenced.
 */
-QQmlPropertyCache *ResolvedTypeReference::propertyCache() const
+QQmlRefPointer<QQmlPropertyCache> ResolvedTypeReference::propertyCache() const
 {
     if (type.isValid())
         return typePropertyCache;
@@ -654,7 +654,7 @@ QQmlPropertyCache *ResolvedTypeReference::propertyCache() const
 /*!
 Returns the property cache, creating one if it doesn't already exist.  The cache is not referenced.
 */
-QQmlPropertyCache *ResolvedTypeReference::createPropertyCache(QQmlEngine *engine)
+QQmlRefPointer<QQmlPropertyCache> ResolvedTypeReference::createPropertyCache(QQmlEngine *engine)
 {
     if (typePropertyCache) {
         return typePropertyCache;
diff --git a/src/qml/compiler/qv4compileddata_p.h b/src/qml/compiler/qv4compileddata_p.h
index 66707456cd..ee71a5c6c4 100644
--- a/src/qml/compiler/qv4compileddata_p.h
+++ b/src/qml/compiler/qv4compileddata_p.h
@@ -949,7 +949,7 @@ public:
 
     // QML specific fields
     QQmlPropertyCacheVector propertyCaches;
-    QQmlPropertyCache *rootPropertyCache() const { return propertyCaches.at(/*root object*/0); }
+    QQmlRefPointer<QQmlPropertyCache> rootPropertyCache() const { return propertyCaches.at(/*root object*/0); }
 
     QQmlRefPointer<QQmlTypeNameCache> typeNameCache;
 
@@ -1045,8 +1045,8 @@ struct ResolvedTypeReference
     // therefore cannot have a property cache installed when instantiated.
     bool isFullyDynamicType;
 
-    QQmlPropertyCache *propertyCache() const;
-    QQmlPropertyCache *createPropertyCache(QQmlEngine *);
+    QQmlRefPointer<QQmlPropertyCache> propertyCache() const;
+    QQmlRefPointer<QQmlPropertyCache> createPropertyCache(QQmlEngine *);
     bool addToHash(QCryptographicHash *hash, QQmlEngine *engine);
 
     void doDynamicTypeCheck();