diff options
5 files changed, 215 insertions, 66 deletions
diff --git a/src/qml/qml/qqmlimport.cpp b/src/qml/qml/qqmlimport.cpp index c8d17c4b8e..f43ffb4c3c 100644 --- a/src/qml/qml/qqmlimport.cpp +++ b/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; 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 |