Fix crash when calling QQmlEngine::clearComponentCache()

We must protect various resources in the type loader with our existing
lock. The QQmlTypeLoaderQmldirContent is now value based, so that we can
release the lock on the shared cache early. Copying it involves
adjusting the refcount of the QHash and QString instances in the
QQmlDirParser.

The safety of this was verified with a TSAN build and the example
supplied in the task. It crashed reliably with TASN errors first and
with this patch it runs without errors.

Task-number: QTBUG-41465
Change-Id: I616843c4b8bdfd65d1277d4faa8cb884d8e77df8
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

6 files changed, 71 insertions, 61 deletions

diff --git a/src/qml/qml/qqmldirparser_p.h b/src/qml/qml/qqmldirparser_p.h
index 95370398ad..820c40238d 100644
--- a/src/qml/qml/qqmldirparser_p.h
+++ b/src/qml/qml/qqmldirparser_p.h
@@ -63,8 +63,6 @@ class QQmlError;
 class QQmlEngine;
 class Q_QML_PRIVATE_EXPORT QQmlDirParser
 {
-    Q_DISABLE_COPY(QQmlDirParser)
-
 public:
     QQmlDirParser();
     ~QQmlDirParser();
diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp
index 4054d2f0be..49f25e89fe 100644
--- a/src/qml/qml/qqmlengine.cpp
+++ b/src/qml/qml/qqmlengine.cpp
@@ -1104,7 +1104,9 @@ QQmlEngine::~QQmlEngine()
 void QQmlEngine::clearComponentCache()
 {
     Q_D(QQmlEngine);
+    d->typeLoader.lock();
     d->typeLoader.clearCache();
+    d->typeLoader.unlock();
 }
 
 /*!
diff --git a/src/qml/qml/qqmlimport.cpp b/src/qml/qml/qqmlimport.cpp
index 8e6cbcbd7e..005db4248e 100644
--- a/src/qml/qml/qqmlimport.cpp
+++ b/src/qml/qml/qqmlimport.cpp
@@ -318,17 +318,17 @@ public:
                              QQmlImportDatabase *database,
                              QString *outQmldirFilePath, QString *outUrl);
 
-    static bool validateQmldirVersion(const QQmlTypeLoaderQmldirContent *qmldir, const QString &uri, int vmaj, int vmin,
+    static bool validateQmldirVersion(const QQmlTypeLoaderQmldirContent &qmldir, const QString &uri, int vmaj, int vmin,
                                       QList<QQmlError> *errors);
 
     bool importExtension(const QString &absoluteFilePath, const QString &uri,
                          int vmaj, int vmin,
                          QQmlImportDatabase *database,
-                         const QQmlTypeLoaderQmldirContent *qmldir,
+                         const QQmlTypeLoaderQmldirContent &qmldir,
                          QList<QQmlError> *errors);
 
     bool getQmldirContent(const QString &qmldirIdentifier, const QString &uri,
-                          const QQmlTypeLoaderQmldirContent **qmldir, QList<QQmlError> *errors);
+                          QQmlTypeLoaderQmldirContent *qmldir, QList<QQmlError> *errors);
 
     QString resolvedUri(const QString &dir_arg, QQmlImportDatabase *database);
 
@@ -668,14 +668,14 @@ bool QQmlImports::resolveType(const QHashedStringRef &type,
     return false;
 }
 
-bool QQmlImportInstance::setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent *qmldir, QQmlImportNamespace *nameSpace, QList<QQmlError> *errors)
+bool QQmlImportInstance::setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent &qmldir, QQmlImportNamespace *nameSpace, QList<QQmlError> *errors)
 {
     Q_ASSERT(resolvedUrl.endsWith(Slash));
     url = resolvedUrl;
 
-    qmlDirComponents = qmldir->components();
+    qmlDirComponents = qmldir.components();
 
-    const QQmlDirScripts &scripts = qmldir->scripts();
+    const QQmlDirScripts &scripts = qmldir.scripts();
     if (!scripts.isEmpty()) {
         // Verify that we haven't imported these scripts already
         for (QList<QQmlImportInstance *>::const_iterator it = nameSpace->imports.constBegin();
@@ -1068,26 +1068,26 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath,
                                          const QString &uri,
                                          int vmaj, int vmin,
                                          QQmlImportDatabase *database,
-                                         const QQmlTypeLoaderQmldirContent *qmldir,
+                                         const QQmlTypeLoaderQmldirContent &qmldir,
                                          QList<QQmlError> *errors)
 {
-    Q_ASSERT(qmldir);
+    Q_ASSERT(qmldir.hasContent());
 
     if (qmlImportTrace())
         qDebug().nospace() << "QQmlImports(" << qPrintable(base) << ")::importExtension: "
                            << "loaded " << qmldirFilePath;
 
-    if (designerSupportRequired && !qmldir->designerSupported()) {
+    if (designerSupportRequired && !qmldir.designerSupported()) {
         if (errors) {
             QQmlError error;
-            error.setDescription(QQmlImportDatabase::tr("module does not support the designer \"%1\"").arg(qmldir->typeNamespace()));
+            error.setDescription(QQmlImportDatabase::tr("module does not support the designer \"%1\"").arg(qmldir.typeNamespace()));
             error.setUrl(QUrl::fromLocalFile(qmldirFilePath));
             errors->prepend(error);
         }
         return false;
     }
 
-    int qmldirPluginCount = qmldir->plugins().count();
+    int qmldirPluginCount = qmldir.plugins().count();
     if (qmldirPluginCount == 0)
         return true;
 
@@ -1098,7 +1098,7 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath,
         // listed plugin inside qmldir. And for this reason, mixing dynamic and static plugins inside a
         // single module is not recommended.
 
-        QString typeNamespace = qmldir->typeNamespace();
+        QString typeNamespace = qmldir.typeNamespace();
         QString qmldirPath = qmldirFilePath;
         int slash = qmldirPath.lastIndexOf(Slash);
         if (slash > 0)
@@ -1108,7 +1108,7 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath,
         int staticPluginsFound = 0;
 
 #if defined(QT_SHARED)
-        const auto qmldirPlugins = qmldir->plugins();
+        const auto qmldirPlugins = qmldir.plugins();
         for (const QQmlDirParser::Plugin &plugin : qmldirPlugins) {
             QString resolvedFilePath = database->resolvePlugin(typeLoader, qmldirPath, plugin.path, plugin.name);
             if (!resolvedFilePath.isEmpty()) {
@@ -1174,7 +1174,7 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath,
                 if (qmldirPluginCount > 1 && staticPluginsFound > 0)
                     error.setDescription(QQmlImportDatabase::tr("could not resolve all plugins for module \"%1\"").arg(uri));
                 else
-                    error.setDescription(QQmlImportDatabase::tr("module \"%1\" plugin \"%2\" not found").arg(uri).arg(qmldir->plugins()[dynamicPluginsFound].name));
+                    error.setDescription(QQmlImportDatabase::tr("module \"%1\" plugin \"%2\" not found").arg(uri).arg(qmldir.plugins()[dynamicPluginsFound].name));
                 error.setUrl(QUrl::fromLocalFile(qmldirFilePath));
                 errors->prepend(error);
             }
@@ -1187,17 +1187,17 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath,
 }
 
 bool QQmlImportsPrivate::getQmldirContent(const QString &qmldirIdentifier, const QString &uri,
-                                          const QQmlTypeLoaderQmldirContent **qmldir, QList<QQmlError> *errors)
+                                          QQmlTypeLoaderQmldirContent *qmldir, QList<QQmlError> *errors)
 {
     Q_ASSERT(errors);
     Q_ASSERT(qmldir);
 
     *qmldir = typeLoader->qmldirContent(qmldirIdentifier);
-    if (*qmldir) {
+    if ((*qmldir).hasContent()) {
         // Ensure that parsing was successful
-        if ((*qmldir)->hasError()) {
+        if ((*qmldir).hasError()) {
             QUrl url = QUrl::fromLocalFile(qmldirIdentifier);
-            const QList<QQmlError> qmldirErrors = (*qmldir)->errors(uri);
+            const QList<QQmlError> qmldirErrors = (*qmldir).errors(uri);
             for (int i = 0; i < qmldirErrors.size(); ++i) {
                 QQmlError error = qmldirErrors.at(i);
                 error.setUrl(url);
@@ -1323,14 +1323,14 @@ bool QQmlImportsPrivate::locateQmldir(const QString &uri, int vmaj, int vmin, QQ
     return false;
 }
 
-bool QQmlImportsPrivate::validateQmldirVersion(const QQmlTypeLoaderQmldirContent *qmldir, const QString &uri, int vmaj, int vmin,
+bool QQmlImportsPrivate::validateQmldirVersion(const QQmlTypeLoaderQmldirContent &qmldir, const QString &uri, int vmaj, int vmin,
                                                QList<QQmlError> *errors)
 {
     int lowest_min = INT_MAX;
     int highest_min = INT_MIN;
 
     typedef QQmlDirComponents::const_iterator ConstIterator;
-    const QQmlDirComponents &components = qmldir->components();
+    const QQmlDirComponents &components = qmldir.components();
 
     ConstIterator cend = components.constEnd();
     for (ConstIterator cit = components.constBegin(); cit != cend; ++cit) {
@@ -1354,7 +1354,7 @@ bool QQmlImportsPrivate::validateQmldirVersion(const QQmlTypeLoaderQmldirContent
     }
 
     typedef QList<QQmlDirParser::Script>::const_iterator SConstIterator;
-    const QQmlDirScripts &scripts = qmldir->scripts();
+    const QQmlDirScripts &scripts = qmldir.scripts();
 
     SConstIterator send = scripts.constEnd();
     for (SConstIterator sit = scripts.constBegin(); sit != send; ++sit) {
@@ -1446,14 +1446,14 @@ bool QQmlImportsPrivate::addLibraryImport(const QString& uri, const QString &pre
     Q_ASSERT(inserted);
 
     if (!incomplete) {
-        const QQmlTypeLoaderQmldirContent *qmldir = nullptr;
+        QQmlTypeLoaderQmldirContent qmldir;
 
         if (!qmldirIdentifier.isEmpty()) {
             if (!getQmldirContent(qmldirIdentifier, uri, &qmldir, errors))
                 return false;
 
-            if (qmldir) {
-                if (!importExtension(qmldir->pluginLocation(), uri, vmaj, vmin, database, qmldir, errors))
+            if (qmldir.hasContent()) {
+                if (!importExtension(qmldir.pluginLocation(), uri, vmaj, vmin, database, qmldir, errors))
                     return false;
 
                 if (!inserted->setQmldirContent(qmldirUrl, qmldir, nameSpace, errors))
@@ -1471,7 +1471,7 @@ bool QQmlImportsPrivate::addLibraryImport(const QString& uri, const QString &pre
                     error.setDescription(QQmlImportDatabase::tr("module \"%1\" is not installed").arg(uri));
                 errors->prepend(error);
                 return false;
-            } else if ((vmaj >= 0) && (vmin >= 0) && qmldir) {
+            } else if ((vmaj >= 0) && (vmin >= 0) && qmldir.hasContent()) {
                 // Verify that the qmldir content is valid for this version
                 if (!validateQmldirVersion(qmldir, uri, vmaj, vmin, errors))
                     return false;
@@ -1565,12 +1565,12 @@ bool QQmlImportsPrivate::addFileImport(const QString& uri, const QString &prefix
     Q_ASSERT(inserted);
 
     if (!incomplete && !qmldirIdentifier.isEmpty()) {
-        const QQmlTypeLoaderQmldirContent *qmldir = nullptr;
+        QQmlTypeLoaderQmldirContent qmldir;
         if (!getQmldirContent(qmldirIdentifier, importUri, &qmldir, errors))
             return false;
 
-        if (qmldir) {
-            if (!importExtension(qmldir->pluginLocation(), importUri, vmaj, vmin, database, qmldir, errors))
+        if (qmldir.hasContent()) {
+            if (!importExtension(qmldir.pluginLocation(), importUri, vmaj, vmin, database, qmldir, errors))
                 return false;
 
             if (!inserted->setQmldirContent(url, qmldir, nameSpace, errors))
@@ -1589,14 +1589,14 @@ bool QQmlImportsPrivate::updateQmldirContent(const QString &uri, const QString &
     Q_ASSERT(nameSpace);
 
     if (QQmlImportInstance *import = nameSpace->findImport(uri)) {
-        const QQmlTypeLoaderQmldirContent *qmldir = nullptr;
+        QQmlTypeLoaderQmldirContent qmldir;
         if (!getQmldirContent(qmldirIdentifier, uri, &qmldir, errors))
             return false;
 
-        if (qmldir) {
+        if (qmldir.hasContent()) {
             int vmaj = import->majversion;
             int vmin = import->minversion;
-            if (!importExtension(qmldir->pluginLocation(), uri, vmaj, vmin, database, qmldir, errors))
+            if (!importExtension(qmldir.pluginLocation(), uri, vmaj, vmin, database, qmldir, errors))
                 return false;
 
             if (import->setQmldirContent(qmldirUrl, qmldir, nameSpace, errors)) {
diff --git a/src/qml/qml/qqmlimport_p.h b/src/qml/qml/qqmlimport_p.h
index b70bb5253c..2437979ef8 100644
--- a/src/qml/qml/qqmlimport_p.h
+++ b/src/qml/qml/qqmlimport_p.h
@@ -85,7 +85,7 @@ struct QQmlImportInstance
     QQmlDirComponents qmlDirComponents; // a copy of the components listed in the qmldir
     QQmlDirScripts qmlDirScripts; // a copy of the scripts in the qmldir
 
-    bool setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent *qmldir,
+    bool setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent &qmldir,
                           QQmlImportNamespace *nameSpace, QList<QQmlError> *errors);
 
     static QQmlDirScripts getVersionedScripts(const QQmlDirScripts &qmldirscripts, int vmaj, int vmin);
diff --git a/src/qml/qml/qqmltypeloader.cpp b/src/qml/qml/qqmltypeloader.cpp
index 5b954605e0..2b778b0b63 100644
--- a/src/qml/qml/qqmltypeloader.cpp
+++ b/src/qml/qml/qqmltypeloader.cpp
@@ -1370,8 +1370,8 @@ bool QQmlTypeLoader::Blob::updateQmldir(QQmlQmldirData *data, const QV4::Compile
     if (!importQualifier.isEmpty()) {
         // Does this library contain any qualified scripts?
         QUrl libraryUrl(qmldirUrl);
-        const QQmlTypeLoaderQmldirContent *qmldir = typeLoader()->qmldirContent(qmldirIdentifier);
-        const auto qmldirScripts = qmldir->scripts();
+        const QQmlTypeLoaderQmldirContent qmldir = typeLoader()->qmldirContent(qmldirIdentifier);
+        const auto qmldirScripts = qmldir.scripts();
         for (const QQmlDirParser::Script &script : qmldirScripts) {
             QUrl scriptUrl = libraryUrl.resolved(QUrl(script.fileName));
             QQmlScriptBlob *blob = typeLoader()->getScript(scriptUrl);
@@ -1418,8 +1418,8 @@ bool QQmlTypeLoader::Blob::addImport(const QV4::CompiledData::Import *import, QL
             if (!importQualifier.isEmpty()) {
                 // Does this library contain any qualified scripts?
                 QUrl libraryUrl(qmldirUrl);
-                const QQmlTypeLoaderQmldirContent *qmldir = typeLoader()->qmldirContent(qmldirFilePath);
-                const auto qmldirScripts = qmldir->scripts();
+                const QQmlTypeLoaderQmldirContent qmldir = typeLoader()->qmldirContent(qmldirFilePath);
+                const auto qmldirScripts = qmldir.scripts();
                 for (const QQmlDirParser::Script &script : qmldirScripts) {
                     QUrl scriptUrl = libraryUrl.resolved(QUrl(script.fileName));
                     QQmlScriptBlob *blob = typeLoader()->getScript(scriptUrl);
@@ -1584,6 +1584,7 @@ QString QQmlTypeLoaderQmldirContent::typeNamespace() const
 
 void QQmlTypeLoaderQmldirContent::setContent(const QString &location, const QString &content)
 {
+    m_hasContent = true;
     m_location = location;
     m_parser.parse(content);
 }
@@ -1808,6 +1809,7 @@ QString QQmlTypeLoader::absoluteFilePath(const QString &path)
     int lastSlash = path.lastIndexOf(QLatin1Char('/'));
     QString dirPath(path.left(lastSlash));
 
+    LockHolder<QQmlTypeLoader> holder(this);
     if (!m_importDirCache.contains(dirPath)) {
         bool exists = QDir(dirPath).exists();
         QCache<QString, bool> *entry = exists ? new QCache<QString, bool> : nullptr;
@@ -1871,6 +1873,7 @@ bool QQmlTypeLoader::directoryExists(const QString &path)
         --length;
     QString dirPath(path.left(length));
 
+    LockHolder<QQmlTypeLoader> holder(this);
     if (!m_importDirCache.contains(dirPath)) {
         bool exists = QDir(dirPath).exists();
         QCache<QString, bool> *files = exists ? new QCache<QString, bool> : nullptr;
@@ -1889,8 +1892,10 @@ Return a QQmlTypeLoaderQmldirContent for absoluteFilePath.  The QQmlTypeLoaderQm
 
 It can also be a remote path for a remote directory import, but it will have been cached by now in this case.
 */
-const QQmlTypeLoaderQmldirContent *QQmlTypeLoader::qmldirContent(const QString &filePathIn)
+const QQmlTypeLoaderQmldirContent QQmlTypeLoader::qmldirContent(const QString &filePathIn)
 {
+    LockHolder<QQmlTypeLoader> holder(this);
+
     QString filePath;
 
     // Try to guess if filePathIn is already a URL. This is necessarily fragile, because
@@ -1904,39 +1909,39 @@ const QQmlTypeLoaderQmldirContent *QQmlTypeLoader::qmldirContent(const QString &
         filePath = filePathIn;
     } else {
         filePath = QQmlFile::urlToLocalFileOrQrc(url);
-        if (filePath.isEmpty()) // Can't load the remote here, but should be cached
-            return *(m_importQmlDirCache.value(filePathIn));
+        if (filePath.isEmpty()) { // Can't load the remote here, but should be cached
+            if (auto entry = m_importQmlDirCache.value(filePathIn))
+                return **entry;
+            else
+                return QQmlTypeLoaderQmldirContent();
+        }
     }
 
-    QQmlTypeLoaderQmldirContent *qmldir;
     QQmlTypeLoaderQmldirContent **val = m_importQmlDirCache.value(filePath);
-    if (!val) {
-        qmldir = new QQmlTypeLoaderQmldirContent;
+    if (val)
+        return **val;
+    QQmlTypeLoaderQmldirContent *qmldir = new QQmlTypeLoaderQmldirContent;
 
 #define ERROR(description) { QQmlError e; e.setDescription(description); qmldir->setError(e); }
 #define NOT_READABLE_ERROR QString(QLatin1String("module \"$$URI$$\" definition \"%1\" not readable"))
 #define CASE_MISMATCH_ERROR QString(QLatin1String("cannot load module \"$$URI$$\": File name case mismatch for \"%1\""))
 
-        QFile file(filePath);
-        if (!QQml_isFileCaseCorrect(filePath)) {
-            ERROR(CASE_MISMATCH_ERROR.arg(filePath));
-        } else if (file.open(QFile::ReadOnly)) {
-            QByteArray data = file.readAll();
-            qmldir->setContent(filePath, QString::fromUtf8(data));
-        } else {
-            ERROR(NOT_READABLE_ERROR.arg(filePath));
-        }
+    QFile file(filePath);
+    if (!QQml_isFileCaseCorrect(filePath)) {
+        ERROR(CASE_MISMATCH_ERROR.arg(filePath));
+    } else if (file.open(QFile::ReadOnly)) {
+        QByteArray data = file.readAll();
+        qmldir->setContent(filePath, QString::fromUtf8(data));
+    } else {
+        ERROR(NOT_READABLE_ERROR.arg(filePath));
+    }
 
 #undef ERROR
 #undef NOT_READABLE_ERROR
 #undef CASE_MISMATCH_ERROR
 
-        m_importQmlDirCache.insert(filePath, qmldir);
-    } else {
-        qmldir = *val;
-    }
-
-    return qmldir;
+    m_importQmlDirCache.insert(filePath, qmldir);
+    return *qmldir;
 }
 
 void QQmlTypeLoader::setQmldirContent(const QString &url, const QString &content)
diff --git a/src/qml/qml/qqmltypeloader_p.h b/src/qml/qml/qqmltypeloader_p.h
index df79d13f1c..4665d342c1 100644
--- a/src/qml/qml/qqmltypeloader_p.h
+++ b/src/qml/qml/qqmltypeloader_p.h
@@ -228,12 +228,16 @@ class QQmlTypeLoaderQmldirContent
 {
 private:
     friend class QQmlTypeLoader;
-    QQmlTypeLoaderQmldirContent();
 
     void setContent(const QString &location, const QString &content);
     void setError(const QQmlError &);
 
 public:
+    QQmlTypeLoaderQmldirContent();
+    QQmlTypeLoaderQmldirContent(const QQmlTypeLoaderQmldirContent &) = default;
+    QQmlTypeLoaderQmldirContent &operator=(const QQmlTypeLoaderQmldirContent &) = default;
+
+    bool hasContent() const { return m_hasContent; }
     bool hasError() const;
     QList<QQmlError> errors(const QString &uri) const;
 
@@ -250,6 +254,7 @@ public:
 private:
     QQmlDirParser m_parser;
     QString m_location;
+    bool m_hasContent = false;
 };
 
 class Q_QML_PRIVATE_EXPORT QQmlTypeLoader
@@ -304,7 +309,7 @@ public:
     QString absoluteFilePath(const QString &path);
     bool directoryExists(const QString &path);
 
-    const QQmlTypeLoaderQmldirContent *qmldirContent(const QString &filePath);
+    const QQmlTypeLoaderQmldirContent qmldirContent(const QString &filePath);
     void setQmldirContent(const QString &filePath, const QString &content);
 
     void clearCache();