From 9e26146a08a7d08ebb9ecc31cafef0cae96a1130 Mon Sep 17 00:00:00 2001 From: Karol Suprynowicz Date: Fri, 22 Nov 2024 15:54:20 +0100 Subject: [PATCH 1/2] Fix script-related crashes on exiting domain --- .../src/EntityTreeRenderer.cpp | 14 ++- libraries/script-engine/src/ScriptManager.cpp | 86 ++++++++++--------- libraries/script-engine/src/ScriptManager.h | 11 ++- 3 files changed, 61 insertions(+), 50 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 3951ffbce0..188c68521e 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -220,8 +220,11 @@ void EntityTreeRenderer::setupEntityScriptEngineSignals(const ScriptManagerPoint void EntityTreeRenderer::resetPersistentEntitiesScriptEngine() { // This runs script engine shutdown procedure in a separate thread, avoiding a deadlock when script engine is doing // a blocking call to main thread - if (_persistentEntitiesScriptManager) { - QtConcurrent::run([manager = _persistentEntitiesScriptManager] { + ScriptManagerPointer scriptManager = _persistentEntitiesScriptManager; + // Clear the pointer before lambda is run on another thread. + _persistentEntitiesScriptManager.reset(); + if (scriptManager) { + QtConcurrent::run([manager = scriptManager] { manager->unloadAllEntityScripts(true); manager->stop(); manager->waitTillDoneRunning(); @@ -251,8 +254,11 @@ void EntityTreeRenderer::resetPersistentEntitiesScriptEngine() { void EntityTreeRenderer::resetNonPersistentEntitiesScriptEngine() { // This runs script engine shutdown procedure in a separate thread, avoiding a deadlock when script engine is doing // a blocking call to main thread - if (_nonPersistentEntitiesScriptManager) { - QtConcurrent::run([manager = _nonPersistentEntitiesScriptManager] { + ScriptManagerPointer scriptManager = _nonPersistentEntitiesScriptManager; + // Release the pointer as soon as possible. + _nonPersistentEntitiesScriptManager.reset(); + if (scriptManager) { + QtConcurrent::run([manager = scriptManager] { manager->unloadAllEntityScripts(true); manager->stop(); manager->waitTillDoneRunning(); diff --git a/libraries/script-engine/src/ScriptManager.cpp b/libraries/script-engine/src/ScriptManager.cpp index 69d39344b0..28f544aa04 100644 --- a/libraries/script-engine/src/ScriptManager.cpp +++ b/libraries/script-engine/src/ScriptManager.cpp @@ -427,7 +427,10 @@ void ScriptManager::runInThread() { void ScriptManager::executeOnScriptThread(std::function function, const Qt::ConnectionType& type ) { if (QThread::currentThread() != thread()) { - QMetaObject::invokeMethod(this, "executeOnScriptThread", type, Q_ARG(std::function, function)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->executeOnScriptThread(function, type); + }); return; } @@ -859,10 +862,10 @@ void ScriptManager::removeEventHandler(const EntityItemID& entityID, const QStri qCDebug(scriptengine) << "*** WARNING *** ScriptManager::removeEventHandler() called on wrong thread [" << QThread::currentThread() << "], invoking on correct thread [" << thread() << "] " "entityID:" << entityID << " eventName:" << eventName; #endif - QMetaObject::invokeMethod(this, "removeEventHandler", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(const QString&, eventName), - Q_ARG(const ScriptValue&, handler)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->removeEventHandler(entityID, eventName, handler); + }); return; } #ifdef THREAD_DEBUGGING @@ -906,10 +909,10 @@ void ScriptManager::addEventHandler(const EntityItemID& entityID, const QString& "entityID:" << entityID << " eventName:" << eventName; #endif - QMetaObject::invokeMethod(this, "addEventHandler", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(const QString&, eventName), - Q_ARG(const ScriptValue&, handler)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->addEventHandler(entityID, eventName, handler); + }); return; } #ifdef THREAD_DEBUGGING @@ -1168,7 +1171,10 @@ void ScriptManager::stop(bool marshal) { _isStopping = true; // this can be done on any thread if (marshal) { - QMetaObject::invokeMethod(this, "stop"); + // Lambda is necessary there to keep shared_ptr counter above zero if this gets called from different thread + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->stop(false); + }); return; } @@ -2012,11 +2018,10 @@ bool ScriptManager::hasEntityScriptDetails(const EntityItemID& entityID) const { void ScriptManager::loadEntityScript(const EntityItemID& entityID, const QString& entityScript, bool forceRedownload) { if (QThread::currentThread() != thread()) { - QMetaObject::invokeMethod(this, "loadEntityScript", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(const QString&, entityScript), - Q_ARG(bool, forceRedownload) - ); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->loadEntityScript(entityID, entityScript, forceRedownload); + }); return; } PROFILE_RANGE(script, __FUNCTION__); @@ -2093,13 +2098,10 @@ void ScriptManager::entityScriptContentAvailable(const EntityItemID& entityID, c << contents << "isURL:" << isURL << "success:" << success; #endif - QMetaObject::invokeMethod(this, "entityScriptContentAvailable", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(const QString&, scriptOrURL), - Q_ARG(const QString&, contents), - Q_ARG(bool, isURL), - Q_ARG(bool, success), - Q_ARG(const QString&, status)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->entityScriptContentAvailable(entityID, scriptOrURL, contents, isURL, success, status); + }); return; } @@ -2443,9 +2445,10 @@ void ScriptManager::unloadEntityScript(const EntityItemID& entityID, bool should "entityID:" << entityID; #endif - QMetaObject::invokeMethod(this, "unloadEntityScript", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(bool, shouldRemoveFromMap)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->unloadEntityScript(entityID, shouldRemoveFromMap); + }); return; } #ifdef THREAD_DEBUGGING @@ -2496,8 +2499,10 @@ void ScriptManager::unloadAllEntityScripts(bool blockingCall) { qCDebug(scriptengine) << "*** WARNING *** ScriptManager::unloadAllEntityScripts() called on wrong thread [" << QThread::currentThread() << "], invoking on correct thread [" << thread() << "]"; #endif - QMetaObject::invokeMethod(this, "unloadAllEntityScripts", - blockingCall ? Qt::BlockingQueuedConnection : Qt::QueuedConnection); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->unloadAllEntityScripts(blockingCall ? Qt::BlockingQueuedConnection : Qt::QueuedConnection); + }); return; } #ifdef THREAD_DEBUGGING @@ -2590,12 +2595,10 @@ void ScriptManager::callEntityScriptMethod(const EntityItemID& entityID, const Q qCDebug(scriptengine) << "*** WARNING *** ScriptManager::callEntityScriptMethod() called on wrong thread [" << QThread::currentThread() << "], invoking on correct thread [" << thread() << "] " "entityID:" << entityID << "methodName:" << methodName; #endif - - QMetaObject::invokeMethod(this, "callEntityScriptMethod", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(const QString&, methodName), - Q_ARG(const QStringList&, params), - Q_ARG(const QUuid&, remoteCallerID)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->callEntityScriptMethod(entityID, methodName, params, remoteCallerID); + }); return; } #ifdef THREAD_DEBUGGING @@ -2660,10 +2663,10 @@ void ScriptManager::callEntityScriptMethod(const EntityItemID& entityID, const Q "entityID:" << entityID << "methodName:" << methodName << "event: mouseEvent"; #endif - QMetaObject::invokeMethod(this, "callEntityScriptMethod", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(const QString&, methodName), - Q_ARG(const PointerEvent&, event)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->callEntityScriptMethod(entityID, methodName, event); + }); return; } #ifdef THREAD_DEBUGGING @@ -2699,11 +2702,10 @@ void ScriptManager::callEntityScriptMethod(const EntityItemID& entityID, const Q "entityID:" << entityID << "methodName:" << methodName << "otherID:" << otherID << "collision: collision"; #endif - QMetaObject::invokeMethod(this, "callEntityScriptMethod", - Q_ARG(const EntityItemID&, entityID), - Q_ARG(const QString&, methodName), - Q_ARG(const EntityItemID&, otherID), - Q_ARG(const Collision&, collision)); + // Lambda is necessary there to keep shared_ptr counter above zero + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ + manager->callEntityScriptMethod(entityID, methodName, otherID, collision); + }); return; } #ifdef THREAD_DEBUGGING diff --git a/libraries/script-engine/src/ScriptManager.h b/libraries/script-engine/src/ScriptManager.h index 8bf6880e51..7185d3e615 100644 --- a/libraries/script-engine/src/ScriptManager.h +++ b/libraries/script-engine/src/ScriptManager.h @@ -378,8 +378,11 @@ class ScriptManager : public QObject, public EntitiesScriptEngineProvider, publi Q_ENUM(Type); static int processLevelMaxRetries; - ScriptManager(Context context, const QString& scriptContents = NO_SCRIPT, const QString& fileNameString = QString("about:ScriptEngine")); - ~ScriptManager(); +private: + // Constructor is private so that only properly generated shared pointer can be used + explicit ScriptManager(Context context, const QString& scriptContents = NO_SCRIPT, const QString& fileNameString = QString("about:ScriptEngine")); +public: + ~ScriptManager() override; // static initialization support typedef void (*ScriptManagerInitializer)(ScriptManager*); @@ -387,7 +390,7 @@ class ScriptManager : public QObject, public EntitiesScriptEngineProvider, publi public: ScriptManagerInitializer init; StaticInitializerNode* prev; - inline StaticInitializerNode(ScriptManagerInitializer&& pInit) : init(std::move(pInit)),prev(nullptr) { registerNewStaticInitializer(this); } + inline explicit StaticInitializerNode(ScriptManagerInitializer&& pInit) : init(std::move(pInit)),prev(nullptr) { registerNewStaticInitializer(this); } }; static void registerNewStaticInitializer(StaticInitializerNode* dest); @@ -395,7 +398,7 @@ class ScriptManager : public QObject, public EntitiesScriptEngineProvider, publi public: ScriptManagerInitializer init; StaticTypesInitializerNode* prev; - inline StaticTypesInitializerNode(ScriptManagerInitializer&& pInit) : init(std::move(pInit)),prev(nullptr) { registerNewStaticTypesInitializer(this); } + inline explicit StaticTypesInitializerNode(ScriptManagerInitializer&& pInit) : init(std::move(pInit)),prev(nullptr) { registerNewStaticTypesInitializer(this); } }; static void registerNewStaticTypesInitializer(StaticTypesInitializerNode* dest); From d9c5821e504dfbf03db84e9b288b8707ddd7846e Mon Sep 17 00:00:00 2001 From: Karol Suprynowicz Date: Sat, 23 Nov 2024 12:59:12 +0100 Subject: [PATCH 2/2] Fixed blocking call --- libraries/script-engine/src/ScriptManager.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libraries/script-engine/src/ScriptManager.cpp b/libraries/script-engine/src/ScriptManager.cpp index 28f544aa04..58fee1f06d 100644 --- a/libraries/script-engine/src/ScriptManager.cpp +++ b/libraries/script-engine/src/ScriptManager.cpp @@ -2498,11 +2498,10 @@ void ScriptManager::unloadAllEntityScripts(bool blockingCall) { #ifdef THREAD_DEBUGGING qCDebug(scriptengine) << "*** WARNING *** ScriptManager::unloadAllEntityScripts() called on wrong thread [" << QThread::currentThread() << "], invoking on correct thread [" << thread() << "]"; #endif - // Lambda is necessary there to keep shared_ptr counter above zero - QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{ - manager->unloadAllEntityScripts(blockingCall ? Qt::BlockingQueuedConnection : Qt::QueuedConnection); - }); + QMetaObject::invokeMethod(this, [=, manager = shared_from_this()] { + manager->unloadAllEntityScripts(blockingCall); + }, blockingCall ? Qt::BlockingQueuedConnection : Qt::QueuedConnection); return; } #ifdef THREAD_DEBUGGING