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>

4 files changed, 131 insertions, 0 deletions

diff --git a/tests/auto/qml/qqmlmoduleplugin/moduleWithStaticPlugin/qmldir b/tests/auto/qml/qqmlmoduleplugin/moduleWithStaticPlugin/qmldir
new file mode 100644
index 0000000000..104c4bf673
--- /dev/null
+++ b/tests/auto/qml/qqmlmoduleplugin/moduleWithStaticPlugin/qmldir
@@ -0,0 +1,2 @@
+module moduleWithStaticPlugin
+plugin secondStaticPlugin
diff --git a/tests/auto/qml/qqmlmoduleplugin/moduleWithWaitingPlugin/qmldir b/tests/auto/qml/qqmlmoduleplugin/moduleWithWaitingPlugin/qmldir
new file mode 100644
index 0000000000..45a02b2ffe
--- /dev/null
+++ b/tests/auto/qml/qqmlmoduleplugin/moduleWithWaitingPlugin/qmldir
@@ -0,0 +1,2 @@
+module moduleWithWaitingPlugin
+plugin pluginThatWaits
diff --git a/tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.cpp b/tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.cpp
index 8600e1e8ab..9abff7b2f6 100644
--- a/tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.cpp
+++ b/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"
diff --git a/tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.pro b/tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.pro
index 43bd112415..118ca26ee9 100644
--- a/tests/auto/qml/qqmlmoduleplugin/tst_qqmlmoduleplugin.pro
+++ b/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