Skip to content

Commit

Permalink
Merge pull request #1251 from overte-org/fix/entity_shutdown_crash
Browse files Browse the repository at this point in the history 
Fix script-related crashes on exiting a domain
  • Loading branch information
@ksuprynowicz
ksuprynowicz authored Nov 23, 2024
2 parents 9503fae + d9c5821 commit 4e5eda5
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 51 deletions.
14 changes: 10 additions & 4 deletions libraries/entities-renderer/src/EntityTreeRenderer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
87 changes: 44 additions & 43 deletions libraries/script-engine/src/ScriptManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,10 @@ void ScriptManager::runInThread() {

void ScriptManager::executeOnScriptThread(std::function<void()> function, const Qt::ConnectionType& type ) {
if (QThread::currentThread() != thread()) {
QMetaObject::invokeMethod(this, "executeOnScriptThread", type, Q_ARG(std::function<void()>, function));
// Lambda is necessary there to keep shared_ptr counter above zero
QMetaObject::invokeMethod(this, [=, manager = shared_from_this()]{
manager->executeOnScriptThread(function, type);
});
return;
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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__);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2495,9 +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

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);
}, blockingCall ? Qt::BlockingQueuedConnection : Qt::QueuedConnection);
return;
}
#ifdef THREAD_DEBUGGING
Expand Down Expand Up @@ -2590,12 +2594,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
Expand Down Expand Up @@ -2660,10 +2662,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
Expand Down Expand Up @@ -2699,11 +2701,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
Expand Down
11 changes: 7 additions & 4 deletions libraries/script-engine/src/ScriptManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,24 +378,27 @@ 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*);
class StaticInitializerNode {
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);

class StaticTypesInitializerNode {
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);

Expand Down

0 comments on commit 4e5eda5

Please sign in to comment.