From acc658ee803d5faac93f4df5788d0ec453f18cac Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Tue, 28 Feb 2023 20:11:33 +0100 Subject: [PATCH] Fixed deadlock in Create App --- cmake/modules/FindV8.cmake | 6 +++++- libraries/entities/src/EntityScriptingInterface.cpp | 11 +++++++++-- libraries/entities/src/EntityScriptingInterface.h | 3 ++- .../script-engine/src/v8/ScriptValueV8Wrapper.cpp | 2 ++ 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/cmake/modules/FindV8.cmake b/cmake/modules/FindV8.cmake index c180b5eb27..68ed55b09d 100644 --- a/cmake/modules/FindV8.cmake +++ b/cmake/modules/FindV8.cmake @@ -264,6 +264,7 @@ find_library(V8_ICUI18N_LIBRARY_RELEASE MESSAGE("V8_ICUI18N_LIBRARY_RELEASE: ${V8_ICUI18N_LIBRARY_RELEASE}") # Base build with snapshot +MESSAGE("1") if(MSVC) if(V8_LIBRARY_DEBUG AND V8_LIBRARY_RELEASE AND V8_SNAPSHOT_LIBRARY_DEBUG AND V8_SNAPSHOT_LIBRARY_RELEASE) set(V8_LIBRARY @@ -280,10 +281,13 @@ if(MSVC) ) endif() else() - if(CMAKE_BUILD_TYPE EQUAL "Debug") + MESSAGE("2") + if(CMAKE_BUILD_TYPE STREQUAL "Debug") + MESSAGE("3") if(V8_LIBRARY_DEBUG AND V8_PLATFORM_LIBRARY_DEBUG AND V8_SAMPLER_LIBRARY_DEBUG) set(V8_LIBRARY ${V8_LIBRARY_DEBUG} ${V8_PLATFORM_LIBRARY_DEBUG} ${V8_SAMPLER_LIBRARY_DEBUG}) # ${V8_ICU_LIBRARY_DEBUG}) elseif(V8_LIBRARY_DEBUG AND V8_PLATFORM_LIBRARY_DEBUG) + MESSAGE("4") set(V8_LIBRARY ${V8_LIBRARY_DEBUG} ${V8_PLATFORM_LIBRARY_DEBUG}) # ${V8_SAMPLER_LIBRARY_DEBUG}) # ${V8_ICU_LIBRARY_DEBUG}) else() if(V8_LIBRARY_RELEASE AND V8_PLATFORM_LIBRARY_RELEASE AND V8_SAMPLER_LIBRARY_RELEASE) diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index a267e7d1ba..01892b2783 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -1633,12 +1634,15 @@ bool EntityScriptingInterface::queryPropertyMetadata(const QUuid& entityID, } } -bool EntityScriptingInterface::getServerScriptStatus(const QUuid& entityID, const ScriptValue& callback) { +//bool EntityScriptingInterface::getServerScriptStatus(const QUuid& entityID, const ScriptValue& callback) { +bool EntityScriptingInterface::getServerScriptStatus(const QUuid& entityID, ScriptValue callback) { auto client = DependencyManager::get(); auto request = client->createScriptStatusRequest(entityID); auto engine = callback.engine(); auto manager = engine->manager(); + Q_ASSERT(QThread::currentThread() == engine->thread()); + Q_ASSERT(QThread::currentThread() == engine->manager()->thread()); if (!manager) { engine->raiseException(engine->makeError(engine->newValue("This script does not belong to a ScriptManager"))); engine->maybeEmitUncaughtException(__FUNCTION__); @@ -1647,13 +1651,16 @@ bool EntityScriptingInterface::getServerScriptStatus(const QUuid& entityID, cons connect(request, &GetScriptStatusRequest::finished, manager, [callback](GetScriptStatusRequest* request) mutable { auto engine = callback.engine(); - // V8TODO: it seems to sometimes be called on a wrong thread, reading to script engine crashes. I added an assert for now + // V8TODO: it seems to sometimes be called on a wrong thread, reading to script engine crashes. I added an assert for now. + // V8TODO: somehow the asserts are not happening here, but destructor still runs on main thread sometimes and deadlocks. Q_ASSERT(QThread::currentThread() == engine->thread()); Q_ASSERT(QThread::currentThread() == engine->manager()->thread()); QString statusString = EntityScriptStatus_::valueToKey(request->getStatus()); ScriptValueList args { engine->newValue(request->getResponseReceived()), engine->newValue(request->getIsRunning()), engine->newValue(statusString.toLower()), engine->newValue(request->getErrorInfo()) }; callback.call(ScriptValue(), args); request->deleteLater(); + // This causes ScriptValueProxy to be released, and thus its destructor is called on script engine thread and not main thread + callback = ScriptValue(); }); request->start(); return true; diff --git a/libraries/entities/src/EntityScriptingInterface.h b/libraries/entities/src/EntityScriptingInterface.h index 60fb8d03ae..c47b4064c7 100644 --- a/libraries/entities/src/EntityScriptingInterface.h +++ b/libraries/entities/src/EntityScriptingInterface.h @@ -871,7 +871,8 @@ public slots: * @param {string} errorInfo - "" if there is a server entity script running, otherwise it may contain extra * information on the error. */ - Q_INVOKABLE bool getServerScriptStatus(const QUuid& entityID, const ScriptValue& callback); + //Q_INVOKABLE bool getServerScriptStatus(const QUuid& entityID, const ScriptValue& callback); + Q_INVOKABLE bool getServerScriptStatus(const QUuid& entityID, ScriptValue callback); /*@jsdoc * Gets metadata for certain entity properties such as script and serverScripts. diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp index c5ce5d954a..1b964cd946 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp @@ -20,6 +20,8 @@ void ScriptValueV8Wrapper::release() { + // V8TODO: maybe add an assert to check if it happens on script engine thread? + // With v8::Locker in V8ScriptValue such requirement shouldn't be necessary but deleting on different threadwww can cause deadlocks sometimes delete this; }