From ab21945a5462d89bba7c4ed38848f42ce37693e9 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Mon, 22 May 2023 00:15:54 +0200 Subject: [PATCH] Fixed crashes that happened when clearing cache --- interface/src/Application.cpp | 8 ++- interface/src/Application.h | 2 +- .../src/controllers/ScriptingInterface.cpp | 3 ++ .../src/controllers/UserInputMapper.cpp | 53 +++++++++++++++++++ .../src/controllers/UserInputMapper.h | 25 +++++++++ .../impl/endpoints/ScriptEndpoint.h | 1 + .../src/AbstractScriptingServicesInterface.h | 2 +- libraries/shared/src/shared/QtHelpers.cpp | 2 + 8 files changed, 93 insertions(+), 3 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 4449e95d55..b5984c6b65 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -7453,7 +7453,7 @@ void Application::addingEntityWithCertificate(const QString& certificateID, cons ledger->updateLocation(certificateID, placeName); } -void Application::registerScriptEngineWithApplicationServices(const ScriptManagerPointer& scriptManager) { +void Application::registerScriptEngineWithApplicationServices(ScriptManagerPointer& scriptManager) { auto scriptEngine = scriptManager->engine(); scriptManager->setEmitScriptUpdatesFunction([this]() { @@ -7587,6 +7587,12 @@ void Application::registerScriptEngineWithApplicationServices(const ScriptManage } auto scriptingInterface = DependencyManager::get(); scriptEngine->registerGlobalObject("Controller", scriptingInterface.data()); + scriptManager->connect(scriptManager.get(), &ScriptManager::scriptEnding, [scriptManager]() { + // Request removal of controller routes with callbacks to a given script engine + auto userInputMapper = DependencyManager::get(); + userInputMapper->scheduleScriptEndpointCleanup(scriptManager->engine().get()); + // V8TODO: Maybe we should wait until removal is finished if there are still crashes + }); UserInputMapper::registerControllerTypes(scriptEngine.get()); auto recordingInterface = DependencyManager::get(); diff --git a/interface/src/Application.h b/interface/src/Application.h index fbd6ef8cae..005ec3cb18 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -254,7 +254,7 @@ public: NodeToOctreeSceneStats* getOcteeSceneStats() { return &_octreeServerSceneStats; } virtual controller::ScriptingInterface* getControllerScriptingInterface() { return _controllerScriptingInterface; } - virtual void registerScriptEngineWithApplicationServices(const ScriptManagerPointer& scriptManager) override; + virtual void registerScriptEngineWithApplicationServices(ScriptManagerPointer& scriptManager) override; virtual void copyCurrentViewFrustum(ViewFrustum& viewOut) const override { copyDisplayViewFrustum(viewOut); } virtual QThread* getMainThread() override { return thread(); } diff --git a/libraries/controllers/src/controllers/ScriptingInterface.cpp b/libraries/controllers/src/controllers/ScriptingInterface.cpp index 9796c5cb49..0c8869c895 100644 --- a/libraries/controllers/src/controllers/ScriptingInterface.cpp +++ b/libraries/controllers/src/controllers/ScriptingInterface.cpp @@ -45,6 +45,9 @@ STATIC_SCRIPT_TYPES_INITIALIZER((+[](ScriptManager* manager) { auto scriptEngine = manager->engine().get(); scriptRegisterMetaType(scriptEngine); + manager->connect(manager, &ScriptManager::scriptEnding, [manager]() { + ; + }); })); static QRegularExpression SANITIZE_NAME_EXPRESSION{ "[\\(\\)\\.\\s]" }; diff --git a/libraries/controllers/src/controllers/UserInputMapper.cpp b/libraries/controllers/src/controllers/UserInputMapper.cpp index 30b47d7ea9..0a23f6e386 100644 --- a/libraries/controllers/src/controllers/UserInputMapper.cpp +++ b/libraries/controllers/src/controllers/UserInputMapper.cpp @@ -270,6 +270,9 @@ void UserInputMapper::update(float deltaTime) { channel = Pose(); } + // Remove callbacks to script engines that are being destroyed + runScriptEndpointCleanup(); + // Run the mappings code runMappings(); @@ -855,6 +858,12 @@ void UserInputMapper::unloadMapping(const QString& jsonFile) { } } +void UserInputMapper::scheduleScriptEndpointCleanup(ScriptEngine* engine) { + _lock.lock(); + scriptEnginesRequestingCleanup.enqueue(engine); + _lock.unlock(); +} + static const QString JSON_NAME = QStringLiteral("name"); static const QString JSON_CHANNELS = QStringLiteral("channels"); static const QString JSON_CHANNEL_FROM = QStringLiteral("from"); @@ -1249,6 +1258,50 @@ void UserInputMapper::disableMapping(const Mapping::Pointer& mapping) { } } +void UserInputMapper::runScriptEndpointCleanup() { + _lock.lock(); + QList routesToRemove; + while (!scriptEnginesRequestingCleanup.empty()){ + auto engine = scriptEnginesRequestingCleanup.dequeue(); + QList routeLists = {&_deviceRoutes, &_standardRoutes}; + auto iterator = _mappingsByName.begin(); + while (iterator != _mappingsByName.end()) { + if (iterator->second) { + routeLists.append(&iterator->second->routes); + } + iterator++; + } + for (auto routeList: routeLists) { + for (auto route: *routeList) { + auto source = std::dynamic_pointer_cast(route->source); + if (source && source->getEngine() == engine) { + qDebug() << "UserInputMapper::runScriptEndpointCleanup source"; + routesToRemove.append(route); + } + auto destination = std::dynamic_pointer_cast(route->destination); + if (destination && destination->getEngine() == engine) { + qDebug() << "UserInputMapper::runScriptEndpointCleanup destination"; + routesToRemove.append(route); + } + } + } + } + while (!routesToRemove.empty()) { + qDebug() << "UserInputMapper::runScriptEndpointCleanup routesToRemove"; + auto route = routesToRemove.first(); + _deviceRoutes.remove(route); + _standardRoutes.remove(route); + auto iterator = _mappingsByName.begin(); + while (iterator != _mappingsByName.end()) { + iterator->second->routes.remove(route); + iterator++; + } + + routesToRemove.removeAll(route); + } + _lock.unlock(); +} + void UserInputMapper::setActionState(Action action, float value, bool valid) { Locker locker(_lock); _actionStates[toInt(action)] = value; diff --git a/libraries/controllers/src/controllers/UserInputMapper.h b/libraries/controllers/src/controllers/UserInputMapper.h index 1cf766a53a..401f77a74c 100644 --- a/libraries/controllers/src/controllers/UserInputMapper.h +++ b/libraries/controllers/src/controllers/UserInputMapper.h @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -126,6 +127,17 @@ namespace controller { void unloadMappings(const QStringList& jsonFiles); void unloadMapping(const QString& jsonFile); + /** + * @brief Request cleaning up endpoints on script engine shutdown + * + * Script endpoints need to be removed before script engine they belong to gets deleted, because otherwise + * script callback will cause a crash. Script engine invokes this function during shutdown and then waits + * for confirmation before being shut down. + * + * @param engine Pointer to the script engine that will be shut down + */ + void scheduleScriptEndpointCleanup(ScriptEngine* engine); + AxisValue getValue(const Input& input) const; Pose getPose(const Input& input) const; @@ -167,6 +179,16 @@ namespace controller { static bool applyRoute(const RoutePointer& route, bool force = false); void enableMapping(const MappingPointer& mapping); void disableMapping(const MappingPointer& mapping); + + /** + * @brief Clean up endpoints on script engine shutdown + * + * Script endpoints need to be removed before script engine they belong to gets deleted, because otherwise + * script callback will cause a crash. This function is called from UserInputMapper::runMappings. + * + */ + void runScriptEndpointCleanup(); + EndpointPointer endpointFor(const QJSValue& endpoint); EndpointPointer endpointFor(const ScriptValue& endpoint); EndpointPointer compositeEndpointFor(EndpointPointer first, EndpointPointer second); @@ -200,6 +222,9 @@ namespace controller { InputCalibrationData inputCalibrationData; + // Contains pointers to script engines that are requesting callback cleanup during their shutdown process + QQueue scriptEnginesRequestingCleanup; + mutable std::recursive_mutex _lock; }; diff --git a/libraries/controllers/src/controllers/impl/endpoints/ScriptEndpoint.h b/libraries/controllers/src/controllers/impl/endpoints/ScriptEndpoint.h index 5bbe04dd40..240da44716 100644 --- a/libraries/controllers/src/controllers/impl/endpoints/ScriptEndpoint.h +++ b/libraries/controllers/src/controllers/impl/endpoints/ScriptEndpoint.h @@ -33,6 +33,7 @@ public: virtual void apply(const Pose& newValue, const Pointer& source) override; virtual bool isPose() const override { return _returnPose; } + virtual const ScriptEngine* getEngine() const { return _callable.engine().get(); } protected: Q_INVOKABLE void updateValue(); diff --git a/libraries/script-engine/src/AbstractScriptingServicesInterface.h b/libraries/script-engine/src/AbstractScriptingServicesInterface.h index 53bfbbe68a..207d109e79 100644 --- a/libraries/script-engine/src/AbstractScriptingServicesInterface.h +++ b/libraries/script-engine/src/AbstractScriptingServicesInterface.h @@ -26,7 +26,7 @@ using ScriptManagerPointer = std::shared_ptr; class AbstractScriptingServicesInterface { public: /// Registers application specific services with a script engine. - virtual void registerScriptEngineWithApplicationServices(const ScriptManagerPointer& scriptEngine) = 0; + virtual void registerScriptEngineWithApplicationServices(ScriptManagerPointer& scriptEngine) = 0; }; diff --git a/libraries/shared/src/shared/QtHelpers.cpp b/libraries/shared/src/shared/QtHelpers.cpp index ed618ae8a1..1804fa41a1 100644 --- a/libraries/shared/src/shared/QtHelpers.cpp +++ b/libraries/shared/src/shared/QtHelpers.cpp @@ -83,6 +83,8 @@ bool blockingInvokeMethod( } PROFILE_RANGE(app, function); + // V8TODO: this causes a deadlock when main thread calls blocking invoke method on entity script thread, + // for example when clearing cache. Some sort of mutex is needed to prevent this. return QMetaObject::invokeMethod(obj, member, Qt::BlockingQueuedConnection, ret, val0, val1, val2, val3, val4, val5, val6, val7, val8, val9); }