Fix dead lock / race in QML type loader when importing plugins

When importing modules - in the QML loader thread - with plugins we keep
globally track of the Qt plugins that we have loaded that contain QML
modules, to ensure that we don't call the engine-independent
registerTypes() function on the plugin multiple times. After
registerTypes() we may also call initializeEngine() on the plugin for
the engine-specific initialization, which - as a QQmlEngine is provided
as parameter - must happen in the gui thread. For that we issue a
thread-blocking call that waits until the gui thread has woken up and
processed the event/call.

During that time the global plugin lock is held by that QML loader
thread.

If meanwhile the gui thread instantiates a second QQmlEngine and
attempts to issue a synchronous type compilation (using
QQmlComponent::CompilationMode::PreferSynchronous), then gui thread is
blocking and waiting for its own QML loader thread to complete the type
compilation, which may involve processing an import that requires
loading a plugin. Now this second QML loader thread is blocked by trying
to acquire the global plugin registry lock
(qmlEnginePluginsWithRegisteredTypes()->mutex) in qqmlimports.cpp.

Now the first QML loader thread is blocked because the gui thread is not
processing the call events for the first engine. The gui thread is
blocked waiting for the second QML loader thread, which in turn is stuck
trying to acquire the lock held by the first QML loader thread.

The provided test case triggers this scenario, although through a
slightly different way. It's not possible to wait in the gui thread for
the plugin lock to be held in a loader thread via the registerTypes
callback, as that also acquires the QQmlMetaType lock that will
interfere with the test-case. However the same plugin lock issue appears
when the first QML engine is located in a different thread altogether.
In that case the dispatch to the engine thread /works/, but it won't be
the gui thread but instead the secondary helper thread of the test case
that will sit in our initializeEngine() callback.

This bug was spotted in production customer code with backtraces
pointing into the three locations described above: One QML loader thread
blocking on a call to the gui thread, the gui thread blocking on a
second QML loader thread and that one blocking on acquisition of the
plugin lock held by the first.

Fortunately it is not necessary to hold on to the global plugin lock
when doing the engine specific initialization. That allows the second
QML loader thread to complete its work and finally resume the GUI
thread's event loop.

Change-Id: If757b3fc9b473f42b266427e55d7a1572b937515
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

src/qml/qml/qqmlimport.cpp

@@ -2056,29 +2056,38 @@ bool QQmlImportDatabase::importStaticPlugin(QObject *instance, const QString &ba
     // Dynamic plugins are differentiated by their filepath. For static plugins we
     // don't have that information so we use their address as key instead.
     const QString uniquePluginID = QString::asprintf("%p", instance);
-    StringRegisteredPluginMap *plugins = qmlEnginePluginsWithRegisteredTypes();
-    QMutexLocker lock(&plugins->mutex);
+    {
+        StringRegisteredPluginMap *plugins = qmlEnginePluginsWithRegisteredTypes();
+        QMutexLocker lock(&plugins->mutex);
 
-    // Plugin types are global across all engines and should only be
-    // registered once. But each engine still needs to be initialized.
-    bool typesRegistered = plugins->contains(uniquePluginID);
-    bool engineInitialized = initializedPlugins.contains(uniquePluginID);
+        // Plugin types are global across all engines and should only be
+        // registered once. But each engine still needs to be initialized.
+        bool typesRegistered = plugins->contains(uniquePluginID);
 
-    if (typesRegistered) {
-        Q_ASSERT_X(plugins->value(uniquePluginID).uri == uri,
-                   "QQmlImportDatabase::importStaticPlugin",
-                   "Internal error: Static plugin imported previously with different uri");
-    } else {
-        RegisteredPlugin plugin;
-        plugin.uri = uri;
-        plugin.loader = 0;
-        plugins->insert(uniquePluginID, plugin);
+        if (typesRegistered) {
+            Q_ASSERT_X(plugins->value(uniquePluginID).uri == uri,
+                       "QQmlImportDatabase::importStaticPlugin",
+                       "Internal error: Static plugin imported previously with different uri");
+        } else {
+            RegisteredPlugin plugin;
+            plugin.uri = uri;
+            plugin.loader = 0;
+            plugins->insert(uniquePluginID, plugin);
 
-        if (!registerPluginTypes(instance, basePath, uri, typeNamespace, vmaj, errors))
-            return false;
+            if (!registerPluginTypes(instance, basePath, uri, typeNamespace, vmaj, errors))
+                return false;
+        }
+
+        // Release the lock on plugins early as we're done with the global part. Releasing the lock
+        // also allows other QML loader threads to acquire the lock while this thread is blocking
+        // in the initializeEngine call to the gui thread (which in turn may be busy waiting for
+        // other QML loader threads and thus not process the initializeEngine call).
     }
 
-    if (!engineInitialized) {
+    // The plugin's per-engine initialization does not need lock protection, as this function is
+    // only called from the engine specific loader thread and importDynamicPlugin as well as
+    // importStaticPlugin are the only places of access.
+    if (!initializedPlugins.contains(uniquePluginID)) {
         initializedPlugins.insert(uniquePluginID);
 
         if (QQmlExtensionInterface *eiface = qobject_cast<QQmlExtensionInterface *>(instance)) {
@@ -2100,68 +2109,77 @@ bool QQmlImportDatabase::importDynamicPlugin(const QString &filePath, const QStr
     QFileInfo fileInfo(filePath);
     const QString absoluteFilePath = fileInfo.absoluteFilePath();
 
+    QObject *instance = nullptr;
     bool engineInitialized = initializedPlugins.contains(absoluteFilePath);
-    StringRegisteredPluginMap *plugins = qmlEnginePluginsWithRegisteredTypes();
-    QMutexLocker lock(&plugins->mutex);
-    bool typesRegistered = plugins->contains(absoluteFilePath);
-
-    if (typesRegistered) {
-        Q_ASSERT_X(plugins->value(absoluteFilePath).uri == uri,
-                   "QQmlImportDatabase::importDynamicPlugin",
-                   "Internal error: Plugin imported previously with different uri");
-    }
-
-    if (!engineInitialized || !typesRegistered) {
-        if (!QQml_isFileCaseCorrect(absoluteFilePath)) {
-            if (errors) {
-                QQmlError error;
-                error.setDescription(tr("File name case mismatch for \"%1\"").arg(absoluteFilePath));
-                errors->prepend(error);
-            }
-            return false;
+    {
+        StringRegisteredPluginMap *plugins = qmlEnginePluginsWithRegisteredTypes();
+        QMutexLocker lock(&plugins->mutex);
+        bool typesRegistered = plugins->contains(absoluteFilePath);
+
+        if (typesRegistered) {
+            Q_ASSERT_X(plugins->value(absoluteFilePath).uri == uri,
+                       "QQmlImportDatabase::importDynamicPlugin",
+                       "Internal error: Plugin imported previously with different uri");
         }
 
-        QPluginLoader* loader = 0;
-        if (!typesRegistered) {
-            loader = new QPluginLoader(absoluteFilePath);
-
-            if (!loader->load()) {
+        if (!engineInitialized || !typesRegistered) {
+            if (!QQml_isFileCaseCorrect(absoluteFilePath)) {
                 if (errors) {
                     QQmlError error;
-                    error.setDescription(loader->errorString());
+                    error.setDescription(tr("File name case mismatch for \"%1\"").arg(absoluteFilePath));
                     errors->prepend(error);
                 }
-                delete loader;
                 return false;
             }
-        } else {
-            loader = plugins->value(absoluteFilePath).loader;
-        }
 
-       QObject *instance = loader->instance();
+            QPluginLoader* loader = 0;
+            if (!typesRegistered) {
+                loader = new QPluginLoader(absoluteFilePath);
 
-        if (!typesRegistered) {
-            RegisteredPlugin plugin;
-            plugin.uri = uri;
-            plugin.loader = loader;
-            plugins->insert(absoluteFilePath, plugin);
+                if (!loader->load()) {
+                    if (errors) {
+                        QQmlError error;
+                        error.setDescription(loader->errorString());
+                        errors->prepend(error);
+                    }
+                    delete loader;
+                    return false;
+                }
+            } else {
+                loader = plugins->value(absoluteFilePath).loader;
+            }
 
-            // Continue with shared code path for dynamic and static plugins:
-            if (!registerPluginTypes(instance, fileInfo.absolutePath(), uri, typeNamespace, vmaj, errors))
-                return false;
+            instance = loader->instance();
+
+            if (!typesRegistered) {
+                RegisteredPlugin plugin;
+                plugin.uri = uri;
+                plugin.loader = loader;
+                plugins->insert(absoluteFilePath, plugin);
+
+                // Continue with shared code path for dynamic and static plugins:
+                if (!registerPluginTypes(instance, fileInfo.absolutePath(), uri, typeNamespace, vmaj, errors))
+                    return false;
+            }
         }
 
-        if (!engineInitialized) {
-             // things on the engine (eg. adding new global objects) have to be done for every
-             // engine.
-             // XXX protect against double initialization
-             initializedPlugins.insert(absoluteFilePath);
-
-             if (QQmlExtensionInterface *eiface = qobject_cast<QQmlExtensionInterface *>(instance)) {
-                 QQmlEnginePrivate *ep = QQmlEnginePrivate::get(engine);
-                 ep->typeLoader.initializeEngine(eiface, uri.toUtf8().constData());
-             }
-         }
+    // Release the lock on plugins early as we're done with the global part. Releasing the lock
+    // also allows other QML loader threads to acquire the lock while this thread is blocking
+    // in the initializeEngine call to the gui thread (which in turn may be busy waiting for
+    // other QML loader threads and thus not process the initializeEngine call).
+    }
+
+
+    if (!engineInitialized) {
+        // The plugin's per-engine initialization does not need lock protection, as this function is
+        // only called from the engine specific loader thread and importDynamicPlugin as well as
+        // importStaticPlugin are the only places of access.
+        initializedPlugins.insert(absoluteFilePath);
+
+        if (QQmlExtensionInterface *eiface = qobject_cast<QQmlExtensionInterface *>(instance)) {
+            QQmlEnginePrivate *ep = QQmlEnginePrivate::get(engine);
+            ep->typeLoader.initializeEngine(eiface, uri.toUtf8().constData());
+        }
     }
 
     return true;

tests/auto/qml/qqmlmoduleplugin/moduleWithStaticPlugin/qmldir

@@ -0,0 +1,2 @@
+module moduleWithStaticPlugin
+plugin secondStaticPlugin

tests/auto/qml/qqmlmoduleplugin/moduleWithWaitingPlugin/qmldir

@@ -0,0 +1,2 @@
+module moduleWithWaitingPlugin
+plugin pluginThatWaits

tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.cpp

@@ -29,6 +29,9 @@
 #include <qdir.h>
 #include <QtQml/qqmlengine.h>
 #include <QtQml/qqmlcomponent.h>
+#include <QtQml/qqmlextensionplugin.h>
+#include <QtCore/qjsondocument.h>
+#include <QtCore/qjsonarray.h>
 #include <QDebug>
 
 #if defined(Q_OS_MAC)
@@ -73,12 +76,80 @@ private slots:
     void importsChildPlugin();
     void importsChildPlugin2();
     void importsChildPlugin21();
+    void parallelPluginImport();
 
 private:
     QString m_importsDirectory;
     QString m_dataImportsDirectory;
 };
 
+class PluginThatWaits : public QQmlExtensionPlugin
+{
+public:
+    static QByteArray metaData;
+
+    static QMutex initializeEngineEntered;
+    static QWaitCondition waitingForInitializeEngineEntry;
+    static QMutex leavingInitializeEngine;
+    static QWaitCondition waitingForInitializeEngineLeave;
+
+    void registerTypes(const char *uri) override
+    {
+        qmlRegisterModule(uri, 1, 0);
+    }
+
+    void initializeEngine(QQmlEngine *engine, const char *uri) override
+    {
+        initializeEngineEntered.lock();
+        leavingInitializeEngine.lock();
+        waitingForInitializeEngineEntry.wakeOne();
+        initializeEngineEntered.unlock();
+        waitingForInitializeEngineLeave.wait(&leavingInitializeEngine);
+        leavingInitializeEngine.unlock();
+    }
+};
+QByteArray PluginThatWaits::metaData;
+QMutex PluginThatWaits::initializeEngineEntered;
+QWaitCondition PluginThatWaits::waitingForInitializeEngineEntry;
+QMutex PluginThatWaits::leavingInitializeEngine;
+QWaitCondition PluginThatWaits::waitingForInitializeEngineLeave;
+
+class SecondStaticPlugin : public QQmlExtensionPlugin
+{
+public:
+    static QByteArray metaData;
+
+    void registerTypes(const char *uri) override
+    {
+        qmlRegisterModule(uri, 1, 0);
+    }
+};
+QByteArray SecondStaticPlugin::metaData;
+
+template <typename PluginType>
+void registerStaticPlugin(const char *uri)
+{
+    QStaticPlugin plugin;
+    plugin.instance = []() {
+        static PluginType plugin;
+        return static_cast<QObject*>(&plugin);
+    };
+
+    QJsonObject md;
+    md.insert(QStringLiteral("IID"), QQmlExtensionInterface_iid);
+    QJsonArray uris;
+    uris.append(uri);
+    md.insert(QStringLiteral("uri"), uris);
+
+    PluginType::metaData.append(QLatin1String("QTMETADATA  "));
+    PluginType::metaData.append(QJsonDocument(md).toBinaryData());
+
+    plugin.rawMetaData = []() {
+        return PluginType::metaData.constData();
+    };
+    qRegisterStaticPluginFunction(plugin);
+};
+
 void tst_qqmlmoduleplugin::initTestCase()
 {
     QQmlDataTest::initTestCase();
@@ -88,6 +159,9 @@ void tst_qqmlmoduleplugin::initTestCase()
     m_dataImportsDirectory = directory() + QStringLiteral("/imports");
     QVERIFY2(QFileInfo(m_dataImportsDirectory).isDir(),
              qPrintable(QString::fromLatin1("Imports directory '%1' does not exist.").arg(m_dataImportsDirectory)));
+
+    registerStaticPlugin<PluginThatWaits>("moduleWithWaitingPlugin");
+    registerStaticPlugin<SecondStaticPlugin>("moduleWithStaticPlugin");
 }
 
 #define VERIFY_ERRORS(errorfile) \
@@ -635,6 +709,51 @@ void tst_qqmlmoduleplugin::importsChildPlugin21()
     delete object;
 }
 
+void tst_qqmlmoduleplugin::parallelPluginImport()
+{
+    QMutexLocker locker(&PluginThatWaits::initializeEngineEntered);
+
+    QThread worker;
+    QObject::connect(&worker, &QThread::started, [&worker](){
+        // Engines in separate threads are tricky, but as long as we do not create a graphical
+        // object and move objects created by the engines across thread boundaries, this is safe.
+        // At the same time this allows us to place the engine's loader thread into the position
+        // where, without the fix for this bug, the global lock is acquired.
+        QQmlEngine engineInThread;
+
+        QQmlComponent component(&engineInThread);
+        component.setData("import moduleWithWaitingPlugin 1.0\nimport QtQml 2.0\nQtObject {}",
+                          QUrl());
+
+        QScopedPointer<QObject> obj(component.create());
+        QVERIFY(!obj.isNull());
+
+        worker.quit();
+    });
+    worker.start();
+
+    PluginThatWaits::waitingForInitializeEngineEntry.wait(&PluginThatWaits::initializeEngineEntered);
+
+    {
+        // After acquiring this lock, the engine in the other thread as well as its type loader
+        // thread are blocked. However they should not hold the global plugin lock
+        // qmlEnginePluginsWithRegisteredTypes()->mutex in qqmllimports.cpp, allowing for the load
+        // of a component in a different engine with its own plugin to proceed.
+        QMutexLocker continuationLock(&PluginThatWaits::leavingInitializeEngine);
+
+        QQmlEngine secondEngine;
+        QQmlComponent secondComponent(&secondEngine);
+        secondComponent.setData("import moduleWithStaticPlugin 1.0\nimport QtQml 2.0\nQtObject {}",
+                                QUrl());
+        QScopedPointer<QObject> o(secondComponent.create());
+        QVERIFY(!o.isNull());
+
+        PluginThatWaits::waitingForInitializeEngineLeave.wakeOne();
+    }
+
+    worker.wait();
+}
+
 QTEST_MAIN(tst_qqmlmoduleplugin)
 
 #include "tst_qqmlmoduleplugin.moc"

tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.pro

@@ -10,4 +10,12 @@ include (../../shared/util.pri)
 
 TESTDATA = data/* imports/* $$OUT_PWD/imports/*
 
+waitingPlugin.files = moduleWithWaitingPlugin
+waitingPlugin.prefix = /qt-project.org/imports/
+RESOURCES += waitingPlugin
+
+staticPlugin.files = moduleWithStaticPlugin
+staticPlugin.prefix = /qt-project.org/imports/
+RESOURCES += staticPlugin
+
 QT += core-private gui-private qml-private network testlib