Fix most of the crash causes on script engine reload/shutdown

interface/src/Application.cpp

@@ -7556,8 +7556,9 @@ void Application::registerScriptEngineWithApplicationServices(ScriptManagerPoint
         *connection = scriptManager->connect(scriptManager.get(), &ScriptManager::scriptEnding, [scriptManager, connection]() {
             // Request removal of controller routes with callbacks to a given script engine
             auto userInputMapper = DependencyManager::get<UserInputMapper>();
-            userInputMapper->scheduleScriptEndpointCleanup(scriptManager->engine().get());
+            // scheduleScriptEndpointCleanup will have the last instance of shared pointer to script manager
-            // V8TODO: Maybe we should wait until endpoint cleanup is finished before deleting the script engine if there are still crashes
+            // so script manager will get deleted as soon as cleanup is done
+            userInputMapper->scheduleScriptEndpointCleanup(scriptManager);
             QObject::disconnect(*connection);
         });
     }

libraries/controllers/src/controllers/Actions.cpp

@@ -30,7 +30,7 @@ namespace controller {
     }
 
     EndpointPointer ActionsDevice::createEndpoint(const Input& input) const {
-        return std::make_shared<ActionEndpoint>(input);
+        return ActionEndpoint::newEndpoint(input);
     }
 
     /*@jsdoc

libraries/controllers/src/controllers/InputDevice.cpp

@@ -91,7 +91,7 @@ namespace controller {
     }
 
     EndpointPointer InputDevice::createEndpoint(const Input& input) const {
-        return std::make_shared<InputEndpoint>(input);
+        return InputEndpoint::newEndpoint(input);
     }
 
 }

libraries/controllers/src/controllers/StateController.cpp

@@ -51,7 +51,7 @@ Input::NamedVector StateController::getAvailableInputs() const {
 EndpointPointer StateController::createEndpoint(const Input& input) const {
     auto name = stateVariables[input.getChannel()];
     ReadLambda& readLambda = const_cast<QHash<QString, ReadLambda>&>(_namedReadLambdas)[name];
-    return std::make_shared<LambdaRefEndpoint>(readLambda);
+    return LambdaRefEndpoint::newEndpoint(readLambda);
 }
 
 }

libraries/controllers/src/controllers/UserInputMapper.cpp

@@ -32,6 +32,7 @@
 #include <ScriptEngine.h>
 #include <ScriptEngineCast.h>
 #include <ScriptValue.h>
+#include <ScriptManager.h>
 
 #include "impl/conditionals/AndConditional.h"
 #include "impl/conditionals/NotConditional.h"
@@ -92,9 +93,9 @@ void UserInputMapper::registerDevice(InputDevice::Pointer device) {
             if (input.device == STANDARD_DEVICE) {
                 endpoint = std::make_shared<StandardEndpoint>(input);
             } else if (input.device == ACTIONS_DEVICE) {
-                endpoint = std::make_shared<ActionEndpoint>(input);
+                endpoint = ActionEndpoint::newEndpoint(input);
             } else {
-                endpoint = std::make_shared<InputEndpoint>(input);
+                endpoint = InputEndpoint::newEndpoint(input);
             }
         }
         _inputsByEndpoint[endpoint] = input;
@@ -663,7 +664,7 @@ Endpoint::Pointer UserInputMapper::endpointFor(const QJSValue& endpoint) {
     }
 
     if (endpoint.isCallable()) {
-        auto result = std::make_shared<JSEndpoint>(endpoint);
+        auto result = JSEndpoint::newEndpoint(endpoint);
         return result;
     }
 
@@ -677,7 +678,7 @@ Endpoint::Pointer UserInputMapper::endpointFor(const ScriptValue& endpoint) {
     }
 
     if (endpoint.isFunction()) {
-        auto result = std::make_shared<ScriptEndpoint>(endpoint);
+        auto result = ScriptEndpoint::newEndpoint(endpoint);
         return result;
     }
 
@@ -692,7 +693,7 @@ Endpoint::Pointer UserInputMapper::endpointFor(const ScriptValue& endpoint) {
             }
             children.push_back(destination);
         }
-        return std::make_shared<AnyEndpoint>(children);
+        return AnyEndpoint::newEndpoint(children);
     }
 
 
@@ -715,7 +716,7 @@ Endpoint::Pointer UserInputMapper::compositeEndpointFor(Endpoint::Pointer first,
     Endpoint::Pointer result;
     auto iterator = _compositeEndpoints.find(pair);
     if (_compositeEndpoints.end() == iterator) {
-        result = std::make_shared<CompositeEndpoint>(first, second);
+        result = CompositeEndpoint::newEndpoint(first, second);
         _compositeEndpoints[pair] = result;
     } else {
         result = iterator->second;
@@ -858,9 +859,9 @@ void UserInputMapper::unloadMapping(const QString& jsonFile) {
     }
 }
 
-void UserInputMapper::scheduleScriptEndpointCleanup(ScriptEngine* engine) {
+void UserInputMapper::scheduleScriptEndpointCleanup(std::shared_ptr<ScriptManager> manager) {
     _lock.lock();
-    scriptEnginesRequestingCleanup.enqueue(engine);
+    scriptManagersRequestingCleanup.enqueue(manager);
     _lock.unlock();
 }
 
@@ -1025,7 +1026,7 @@ Filter::List UserInputMapper::parseFilters(const QJsonValue& value) {
 
 Endpoint::Pointer UserInputMapper::parseDestination(const QJsonValue& value) {
     if (value.isArray()) {
-        ArrayEndpoint::Pointer result = std::make_shared<ArrayEndpoint>();
+        ArrayEndpoint::Pointer result = std::dynamic_pointer_cast<ArrayEndpoint>(ArrayEndpoint::newEndpoint());
         auto array = value.toArray();
         for (const auto& arrayItem : array) {
             Endpoint::Pointer destination = parseEndpoint(arrayItem);
@@ -1052,7 +1053,7 @@ Endpoint::Pointer UserInputMapper::parseAxis(const QJsonValue& value) {
                     Endpoint::Pointer first = parseEndpoint(axisArray.first());
                     Endpoint::Pointer second = parseEndpoint(axisArray.last());
                     if (first && second) {
-                        return std::make_shared<CompositeEndpoint>(first, second);
+                        return CompositeEndpoint::newEndpoint(first, second);
                     }
                 }
             }
@@ -1072,7 +1073,7 @@ Endpoint::Pointer UserInputMapper::parseAny(const QJsonValue& value) {
             }
             children.push_back(destination);
         }
-        return std::make_shared<AnyEndpoint>(children);
+        return AnyEndpoint::newEndpoint(children);
     }
     return Endpoint::Pointer();
 }
@@ -1261,8 +1262,8 @@ void UserInputMapper::disableMapping(const Mapping::Pointer& mapping) {
 void UserInputMapper::runScriptEndpointCleanup() {
     _lock.lock();
     QList<RoutePointer> routesToRemove;
-    while (!scriptEnginesRequestingCleanup.empty()){
+    while (!scriptManagersRequestingCleanup.empty()){
-        auto engine = scriptEnginesRequestingCleanup.dequeue();
+        auto manager = scriptManagersRequestingCleanup.dequeue();
         QList<RouteList*> routeLists = {&_deviceRoutes, &_standardRoutes};
         auto iterator = _mappingsByName.begin();
         while (iterator != _mappingsByName.end()) {
@@ -1274,12 +1275,12 @@ void UserInputMapper::runScriptEndpointCleanup() {
         for (auto routeList: routeLists) {
             for (auto route: *routeList) {
                 auto source = std::dynamic_pointer_cast<ScriptEndpoint>(route->source);
-                if (source && source->getEngine() == engine) {
+                if (source && source->getEngine() == manager->engine().get()) {
                     qDebug() << "UserInputMapper::runScriptEndpointCleanup source";
                     routesToRemove.append(route);
                 }
                 auto destination = std::dynamic_pointer_cast<ScriptEndpoint>(route->destination);
-                if (destination && destination->getEngine() == engine) {
+                if (destination && destination->getEngine() == manager->engine().get()) {
                     qDebug() << "UserInputMapper::runScriptEndpointCleanup destination";
                     routesToRemove.append(route);
                 }

libraries/controllers/src/controllers/UserInputMapper.h

@@ -35,6 +35,7 @@
 #include "StateController.h"
 
 class ScriptEngine;
+class ScriptManager;
 class ScriptValue;
 
 namespace controller {
@@ -136,7 +137,7 @@ namespace controller {
          *
          * @param engine Pointer to the script engine that will be shut down
          */
-        void scheduleScriptEndpointCleanup(ScriptEngine* engine);
+        void scheduleScriptEndpointCleanup(std::shared_ptr<ScriptManager> manager);
 
         AxisValue getValue(const Input& input) const;
         Pose getPose(const Input& input) const;
@@ -223,7 +224,7 @@ namespace controller {
         InputCalibrationData inputCalibrationData;
 
         // Contains pointers to script engines that are requesting callback cleanup during their shutdown process
-        QQueue<ScriptEngine*> scriptEnginesRequestingCleanup;
+        QQueue<std::shared_ptr<ScriptManager>> scriptManagersRequestingCleanup;
 
         mutable std::recursive_mutex _lock;
     };

libraries/controllers/src/controllers/impl/Endpoint.h

@@ -27,7 +27,9 @@ namespace controller {
     * Encapsulates a particular input / output,
     * i.e. Hydra.Button0, Standard.X, Action.Yaw
     */
-    class Endpoint : public QObject {
+
+// All the derived classes need to have private constructors
+class Endpoint : public QObject, public std::enable_shared_from_this<Endpoint> {
         Q_OBJECT;
     public:
         using Pointer = std::shared_ptr<Endpoint>;
@@ -36,7 +38,6 @@ namespace controller {
         using ReadLambda = std::function<float()>;
         using WriteLambda = std::function<void(float)>;
 
-        Endpoint(const Input& input) : _input(input) {}
         virtual AxisValue value() { return peek(); }
         virtual AxisValue peek() const = 0;
         virtual void apply(AxisValue value, const Pointer& source) = 0;
@@ -51,19 +52,21 @@ namespace controller {
         const Input& getInput() { return _input;  }
 
     protected:
+        Endpoint(const Input& input) : _input(input) {}
         Input _input;
     };
 
     class LambdaEndpoint : public Endpoint {
     public:
         using Endpoint::apply;
-        LambdaEndpoint(ReadLambda readLambda, WriteLambda writeLambda = [](float) {})
-            : Endpoint(Input::INVALID_INPUT), _readLambda(readLambda), _writeLambda(writeLambda) { }
 
         virtual AxisValue peek() const override { return AxisValue(_readLambda(), 0); }
         virtual void apply(AxisValue value, const Pointer& source) override { _writeLambda(value.value); }
 
     private:
+        LambdaEndpoint(ReadLambda readLambda, WriteLambda writeLambda = [](float) {})
+            : Endpoint(Input::INVALID_INPUT), _readLambda(readLambda), _writeLambda(writeLambda) { }
+
         ReadLambda _readLambda;
         WriteLambda _writeLambda;
     };
@@ -72,15 +75,20 @@ namespace controller {
 
     class LambdaRefEndpoint : public Endpoint {
     public:
+        static std::shared_ptr<Endpoint> newEndpoint(const ReadLambda& readLambda, const WriteLambda& writeLambda = DEFAULT_WRITE_LAMBDA) {
+            return std::shared_ptr<Endpoint>(new LambdaRefEndpoint(readLambda, writeLambda));
+        };
+
         using Endpoint::apply;
-        LambdaRefEndpoint(const ReadLambda& readLambda, const WriteLambda& writeLambda = DEFAULT_WRITE_LAMBDA)
-            : Endpoint(Input::INVALID_INPUT), _readLambda(readLambda), _writeLambda(writeLambda) {
-        }
 
         virtual AxisValue peek() const override { return AxisValue(_readLambda(), 0); }
         virtual void apply(AxisValue value, const Pointer& source) override { _writeLambda(value.value); }
 
     private:
+        LambdaRefEndpoint(const ReadLambda& readLambda, const WriteLambda& writeLambda = DEFAULT_WRITE_LAMBDA)
+            : Endpoint(Input::INVALID_INPUT), _readLambda(readLambda), _writeLambda(writeLambda) {
+        }
+
         const ReadLambda& _readLambda;
         const WriteLambda& _writeLambda;
     };
@@ -88,10 +96,6 @@ namespace controller {
 
     class VirtualEndpoint : public Endpoint {
     public:
-        VirtualEndpoint(const Input& id = Input::INVALID_INPUT)
-            : Endpoint(id) {
-        }
-
         virtual AxisValue peek() const override { return _currentValue; }
         virtual void apply(AxisValue value, const Pointer& source) override { _currentValue = value; }
 
@@ -100,6 +104,10 @@ namespace controller {
             _currentPose = value;
         }
     protected:
+        VirtualEndpoint(const Input& id = Input::INVALID_INPUT)
+            : Endpoint(id) {
+        }
+
         AxisValue _currentValue { 0.0f, 0, false };
         Pose _currentPose {};
     };

libraries/controllers/src/controllers/impl/endpoints/ActionEndpoint.h

@@ -21,7 +21,9 @@ namespace controller {
 
 class ActionEndpoint : public Endpoint {
 public:
-    ActionEndpoint(const Input& id = Input::INVALID_INPUT) : Endpoint(id) { }
+    static std::shared_ptr<Endpoint> newEndpoint(const Input& id = Input::INVALID_INPUT) {
+        return std::shared_ptr<Endpoint>(new ActionEndpoint(id));
+    };
 
     virtual AxisValue peek() const override { return _currentValue; }
     virtual void apply(AxisValue newValue, const Pointer& source) override;
@@ -32,6 +34,8 @@ public:
     virtual void reset() override;
 
 private:
+    ActionEndpoint(const Input& id = Input::INVALID_INPUT) : Endpoint(id) { }
+
     AxisValue _currentValue { 0.0f, 0, false };
     Pose _currentPose{};
 };

libraries/controllers/src/controllers/impl/endpoints/AnyEndpoint.h

@@ -17,8 +17,11 @@ namespace controller {
 class AnyEndpoint : public Endpoint {
     friend class UserInputMapper;
 public:
+    static std::shared_ptr<Endpoint> newEndpoint(Endpoint::List children) {
+        return std::shared_ptr<Endpoint>(new AnyEndpoint(children));
+    };
+
     using Endpoint::apply;
-    AnyEndpoint(Endpoint::List children);
     virtual AxisValue peek() const override;
     virtual AxisValue value() override;
     virtual void apply(AxisValue newValue, const Endpoint::Pointer& source) override;
@@ -26,6 +29,8 @@ public:
     virtual bool readable() const override;
 
 private:
+    AnyEndpoint(Endpoint::List children);
+
     Endpoint::List _children;
 };
 

libraries/controllers/src/controllers/impl/endpoints/ArrayEndpoint.h

@@ -17,9 +17,12 @@ namespace controller {
 class ArrayEndpoint : public Endpoint {
     friend class UserInputMapper;
 public:
+    static std::shared_ptr<Endpoint> newEndpoint() {
+        return std::shared_ptr<Endpoint>(new ArrayEndpoint());
+    };
+
     using Endpoint::apply;
     using Pointer = std::shared_ptr<ArrayEndpoint>;
-    ArrayEndpoint() : Endpoint(Input::INVALID_INPUT) { }
 
     virtual AxisValue peek() const override { return AxisValue(); }
 
@@ -34,6 +37,7 @@ public:
     virtual bool readable() const override { return false; }
 
 private:
+    ArrayEndpoint() : Endpoint(Input::INVALID_INPUT) { }
     Endpoint::List _children;
 };
 

libraries/controllers/src/controllers/impl/endpoints/CompositeEndpoint.h

@@ -15,14 +15,19 @@
 namespace controller {
     class CompositeEndpoint : public Endpoint, Endpoint::Pair {
     public:
+        static std::shared_ptr<Endpoint> newEndpoint(Endpoint::Pointer first, Endpoint::Pointer second) {
+            return std::shared_ptr<Endpoint>(new CompositeEndpoint(first, second));
+        };
+
         using Endpoint::apply;
-        CompositeEndpoint(Endpoint::Pointer first, Endpoint::Pointer second);
 
         virtual AxisValue peek() const override;
         virtual AxisValue value() override;
         virtual void apply(AxisValue newValue, const Pointer& source) override;
         virtual bool readable() const override;
 
+    private:
+        CompositeEndpoint(Endpoint::Pointer first, Endpoint::Pointer second);
     };
 
 }

libraries/controllers/src/controllers/impl/endpoints/InputEndpoint.h

@@ -16,9 +16,9 @@ namespace controller {
 
 class InputEndpoint : public Endpoint {
 public:
-    InputEndpoint(const Input& id = Input::INVALID_INPUT)
+    static std::shared_ptr<Endpoint> newEndpoint(const Input& id = Input::INVALID_INPUT) {
-        : Endpoint(id) {
+        return std::shared_ptr<Endpoint>(new InputEndpoint(id));
-    }
+    };
 
     virtual AxisValue peek() const override;
     virtual AxisValue value() override;
@@ -34,6 +34,10 @@ public:
     virtual void reset() override { _read = false; }
 
 private:
+    InputEndpoint(const Input& id = Input::INVALID_INPUT)
+        : Endpoint(id) {
+    }
+
     bool _read { false };
 };
 

libraries/controllers/src/controllers/impl/endpoints/JSEndpoint.h

@@ -19,15 +19,19 @@ namespace controller {
 
 class JSEndpoint : public Endpoint {
 public:
-    using Endpoint::apply;
+    static std::shared_ptr<Endpoint> newEndpoint(const QJSValue& callable) {
-    JSEndpoint(const QJSValue& callable)
+        return std::shared_ptr<Endpoint>(new JSEndpoint(callable));
-        : Endpoint(Input::INVALID_INPUT), _callable(callable) {
+    };
-    }
 
+    using Endpoint::apply;
     virtual AxisValue peek() const override;
     virtual void apply(AxisValue newValue, const Pointer& source) override;
 
 private:
+    JSEndpoint(const QJSValue& callable)
+        : Endpoint(Input::INVALID_INPUT), _callable(callable) {
+    }
+
     mutable QJSValue _callable;
 };
 

libraries/controllers/src/controllers/impl/endpoints/ScriptEndpoint.cpp

@@ -45,7 +45,10 @@ AxisValue ScriptEndpoint::peek() const {
 
 void ScriptEndpoint::updateValue() {
     if (QThread::currentThread() != thread()) {
-        QMetaObject::invokeMethod(this, "updateValue", Qt::QueuedConnection);
+        auto pointer = shared_from_this();
+        QMetaObject::invokeMethod(this, [pointer]{
+            std::dynamic_pointer_cast<ScriptEndpoint>(pointer)->updateValue();
+        });
         return;
     }
 

libraries/controllers/src/controllers/impl/endpoints/ScriptEndpoint.h

@@ -22,9 +22,9 @@ class ScriptEndpoint : public Endpoint {
     Q_OBJECT;
 public:
     using Endpoint::apply;
-    ScriptEndpoint(const ScriptValue& callable)
+    static std::shared_ptr<Endpoint> newEndpoint(const ScriptValue& callable) {
-        : Endpoint(Input::INVALID_INPUT), _callable(callable) {
+        return std::shared_ptr<Endpoint>(new ScriptEndpoint(callable));
-    }
+    };
 
     virtual AxisValue peek() const override;
     virtual void apply(AxisValue newValue, const Pointer& source) override;
@@ -42,6 +42,9 @@ protected:
     Q_INVOKABLE void updatePose();
     Q_INVOKABLE virtual void internalApply(const Pose& newValue, int sourceID);
 private:
+    ScriptEndpoint(const ScriptValue& callable)
+        : Endpoint(Input::INVALID_INPUT), _callable(callable) {
+    }
     ScriptValue _callable;
     float _lastValueRead { 0.0f };
     AxisValue _lastValueWritten { 0.0f, 0, false };

libraries/entities-renderer/src/EntityTreeRenderer.cpp

@@ -226,6 +226,10 @@ void EntityTreeRenderer::resetPersistentEntitiesScriptEngine() {
             manager->stop();
             manager->waitTillDoneRunning();
             manager->disconnectNonEssentialSignals();
+            // TODO: script manager pointer is still in use somewhere after the cleanup in lambda.
+            //  To prevent memory leaks on multiple reloads we would need to find all the usages and remove them.
+            //  Script engines are correctly deleted later during shutdown currently.
+            qDebug() << "_nonPersistentEntitiesScriptManager lambda finished, script manager pointer use count: " << manager.use_count();
         });
     }
     _persistentEntitiesScriptManager = scriptManagerFactory(ScriptManager::ENTITY_CLIENT_SCRIPT, NO_SCRIPT,
@@ -248,6 +252,10 @@ void EntityTreeRenderer::resetNonPersistentEntitiesScriptEngine() {
             manager->stop();
             manager->waitTillDoneRunning();
             manager->disconnectNonEssentialSignals();
+            // TODO: script manager pointer is still in use somewhere after the cleanup in lambda.
+            //  To prevent memory leaks on multiple reloads we would need to find all the usages and remove them.
+            //  Script engines are correctly deleted later during shutdown currently.
+            qDebug() << "_nonPersistentEntitiesScriptManager lambda finished, script manager pointer use count: " << manager.use_count();
         });
     }
     _nonPersistentEntitiesScriptManager = scriptManagerFactory(ScriptManager::ENTITY_CLIENT_SCRIPT, NO_SCRIPT,

libraries/script-engine/src/ScriptEngine.h

@@ -423,6 +423,11 @@ public:
      */
     virtual void stopProfilingAndSave() = 0;
 
+    /**
+     * @brief Cleanup function that disconnects signals connected to script proxies to avoid use-after-delete crash when shutting down script engine.
+     */
+    virtual void disconnectSignalProxies() = 0;
+
 public:
     // helper to detect and log warnings when other code invokes QScriptEngine/BaseScriptEngine in thread-unsafe ways
     bool IS_THREADSAFE_INVOCATION(const QString& method);

libraries/script-engine/src/ScriptEngines.cpp

@@ -21,6 +21,7 @@
 #include <UserActivityLogger.h>
 #include <PathUtils.h>
 #include <shared/FileUtils.h>
+#include <QtConcurrent/QtConcurrent>
 
 #include "ScriptCache.h"
 #include "ScriptEngine.h"
@@ -404,42 +405,49 @@ QStringList ScriptEngines::getRunningScripts() {
 }
 
 void ScriptEngines::stopAllScripts(bool restart) {
-    QReadLocker lock(&_scriptManagersHashLock);
+    QtConcurrent::run([this, restart] {
+        QHash<QUrl, ScriptManagerPointer> scriptManagersHashCopy;
 
-    if (_isReloading) {
+        {
-        return;
+            QReadLocker lock(&_scriptManagersHashLock);
-    }
+            scriptManagersHashCopy = _scriptManagersHash;
-
-    for (QHash<QUrl, ScriptManagerPointer>::const_iterator it = _scriptManagersHash.constBegin();
-        it != _scriptManagersHash.constEnd(); it++) {
-        ScriptManagerPointer scriptManager = it.value();
-        // skip already stopped scripts
-        if (scriptManager->isFinished() || scriptManager->isStopping()) {
-            continue;
         }
 
-        bool isOverrideScript = it.key().toString().compare(this->_defaultScriptsOverride.toString()) == 0;
+        if (_isReloading) {
-        // queue user scripts if restarting
+            return;
-        if (restart && (scriptManager->isUserLoaded() || isOverrideScript)) {
-            _isReloading = true;
-            ScriptManager::Type type = scriptManager->getType();
-
-            connect(scriptManager.get(), &ScriptManager::finished, this, [this, type, isOverrideScript](QString scriptName) {
-                reloadScript(scriptName, !isOverrideScript)->setType(type);
-            });
         }
 
-        // stop all scripts
+        for (QHash<QUrl, ScriptManagerPointer>::const_iterator it = scriptManagersHashCopy.constBegin();
-        scriptManager->stop();
+                it != scriptManagersHashCopy.constEnd(); it++) {
-    }
+            ScriptManagerPointer scriptManager = it.value();
+            // skip already stopped scripts
+            if (scriptManager->isFinished() || scriptManager->isStopping()) {
+                continue;
+            }
 
-    if (restart) {
+            bool isOverrideScript = it.key().toString().compare(this->_defaultScriptsOverride.toString()) == 0;
-        qCDebug(scriptengine) << "stopAllScripts -- emitting scriptsReloading";
+            // queue user scripts if restarting
-        QTimer::singleShot(RELOAD_ALL_SCRIPTS_TIMEOUT, this, [&] {
+            if (restart && (scriptManager->isUserLoaded() || isOverrideScript)) {
-            _isReloading = false;
+                _isReloading = true;
-        });
+                ScriptManager::Type type = scriptManager->getType();
-        emit scriptsReloading();
+
-    }
+                connect(scriptManager.get(), &ScriptManager::finished, this,
+                        [this, type, isOverrideScript](QString scriptName) {
+                            reloadScript(scriptName, !isOverrideScript)->setType(type);
+                        });
+            }
+
+            // stop all scripts
+            scriptManager->stop();
+            scriptManager->waitTillDoneRunning();
+        }
+
+        if (restart) {
+            qCDebug(scriptengine) << "stopAllScripts -- emitting scriptsReloading";
+            QTimer::singleShot(RELOAD_ALL_SCRIPTS_TIMEOUT, this, [&] { _isReloading = false; });
+            emit scriptsReloading();
+        }
+    });
 }
 
 bool ScriptEngines::stopScript(const QString& rawScriptURL, bool restart) {
@@ -612,6 +620,7 @@ void ScriptEngines::onScriptFinished(const QString& rawScriptURL, ScriptManagerP
         }
     }
 
+    // Could this cause deadlocks when script engine invokes a blocking method on main thread?
     manager->waitTillDoneRunning();
     removeScriptEngine(manager);
 

libraries/script-engine/src/ScriptManager.cpp

@@ -67,6 +67,7 @@
 #include <AddressManager.h>
 #include <NetworkingConstants.h>
 #include <ThreadHelpers.h>
+#include <iostream>
 
 const QString ScriptManager::_SETTINGS_ENABLE_EXTENDED_EXCEPTIONS {
     "com.highfidelity.experimental.enableExtendedJSExceptions"
@@ -366,7 +367,12 @@ bool ScriptManager::isDebugMode() const {
 #endif
 }
 
-ScriptManager::~ScriptManager() {}
+ScriptManager::~ScriptManager() {
+    qDebug() << "ScriptManager::~ScriptManager() : Script manager deleted, type: " << _type << " name: " << _fileNameString;
+    if (_type == ScriptManager::Type::ENTITY_CLIENT) {
+        printf("ScriptManager::~ScriptManager");
+    }
+}
 
 void ScriptManager::disconnectNonEssentialSignals() {
     disconnect();
@@ -464,11 +470,14 @@ void ScriptManager::waitTillDoneRunning(bool shutdown) {
         }
 #else
         auto startedWaiting = usecTimestampNow();
-        while (workerThread->isRunning()) {
+        while (!_isDoneRunning) {
             // If the final evaluation takes too long, then tell the script engine to stop running
             auto elapsedUsecs = usecTimestampNow() - startedWaiting;
+            // TODO: This part was very unsafe and was causing crashes all the time.
+            //  I disabled it for now until we find a safer solution.
+            //  With it disabled now we get clean shutdowns and restarts.
             // V8TODO: temporarily increased script timeout. Maybe use different timeouts for release and unoptimized debug?
-            static const auto MAX_SCRIPT_EVALUATION_TIME = USECS_PER_SECOND;
+            /*static const auto MAX_SCRIPT_EVALUATION_TIME = 10 * USECS_PER_SECOND;
             if (elapsedUsecs > MAX_SCRIPT_EVALUATION_TIME) {
                 workerThread->quit();
 
@@ -485,12 +494,12 @@ void ScriptManager::waitTillDoneRunning(bool shutdown) {
 
                 // Wait for the scripting thread to stop running, as
                 // flooding it with aborts/exceptions will persist it longer
-                static const auto MAX_SCRIPT_QUITTING_TIME = 0.5 * MSECS_PER_SECOND;
+                static const auto MAX_SCRIPT_QUITTING_TIME = 50 * MSECS_PER_SECOND;
                 if (!workerThread->wait(MAX_SCRIPT_QUITTING_TIME)) {
                     Q_ASSERT(false);
                     workerThread->terminate();
                 }
-            }
+            }*/
 
             if (shutdown) {
                 // NOTE: This will be called on the main application thread (among other threads) from stopAllScripts.
@@ -502,7 +511,7 @@ void ScriptManager::waitTillDoneRunning(bool shutdown) {
             }
 
             // Avoid a pure busy wait
-            QThread::yieldCurrentThread();
+            QThread::msleep(1);
         }
 #endif
 
@@ -985,6 +994,7 @@ void ScriptManager::run() {
 
         PROFILE_RANGE(script, "ScriptMainLoop");
 
+//#define SCRIPT_DELAY_DEBUG
 #ifdef SCRIPT_DELAY_DEBUG
         {
             auto actuallySleptUntil = clock::now();
@@ -1064,6 +1074,13 @@ void ScriptManager::run() {
     _isRunning = false;
     emit runningStateChanged();
     emit doneRunning();
+    _engine->disconnectSignalProxies();
+    // Process all remaining events
+    {
+        QEventLoop loop;
+        loop.processEvents();
+    }
+    _isDoneRunning = true;
 }
 
 // NOTE: This is private because it must be called on the same thread that created the timers, which is why
@@ -1121,6 +1138,7 @@ void ScriptManager::timerFired() {
         return; // bail early
     }
 
+//#define SCRIPT_TIMER_PERFORMANCE_STATISTICS
 #ifdef SCRIPT_TIMER_PERFORMANCE_STATISTICS
     _timerCallCounter++;
     if (_timerCallCounter % 100 == 0) {

libraries/script-engine/src/ScriptManager.h

@@ -1197,6 +1197,14 @@ public:
      */
     void setAbortOnUncaughtException(bool value) { _abortOnUncaughtException = value; }
 
+    /**
+     * @brief Returns true after script finished running and doneRunning signal was called
+     *
+     * @return true If the script and doneRunning signal was called
+     * @return false If the script has not finished running yet
+     */
+    bool isDoneRunning() { return _isDoneRunning; };
+
 public slots:
 
     /**
@@ -1527,6 +1535,7 @@ protected:
     std::atomic<bool> _isFinished { false };
     std::atomic<bool> _isRunning { false };
     std::atomic<bool> _isStopping { false };
+    std::atomic<bool> _isDoneRunning { false };
     bool _areMetaTypesInitialized { false };
     bool _isInitialized { false };
     QHash<QTimer*, CallbackData> _timerFunctionMap;

libraries/script-engine/src/v8/ScriptEngineV8.cpp

@@ -248,12 +248,27 @@ ScriptEngineV8::ScriptEngineV8(ScriptManager *manager) : ScriptEngine(manager),
 
 ScriptEngineV8::~ScriptEngineV8() {
     deleteUnusedValueWrappers();
+#ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+    _wasDestroyed = true;
+#endif
+    qDebug() << "ScriptEngineV8::~ScriptEngineV8: script engine destroyed";
 }
 
 void ScriptEngineV8::perManagerLoopIterationCleanup() {
     deleteUnusedValueWrappers();
 }
 
+void ScriptEngineV8::disconnectSignalProxies() {
+    _signalProxySetLock.lockForRead();
+    while (!_signalProxySet.empty()) {
+        _signalProxySetLock.unlock();
+        delete *_signalProxySet.begin();
+        _signalProxySetLock.lockForRead();
+    }
+    _signalProxySetLock.unlock();
+}
+
+
 void ScriptEngineV8::deleteUnusedValueWrappers() {
     while (!_scriptValueWrappersToDelete.empty()) {
         auto wrapper = _scriptValueWrappersToDelete.dequeue();
@@ -485,6 +500,9 @@ void ScriptEngineV8::registerGetterSetter(const QString& name, ScriptEngine::Fun
 }
 
 v8::Local<v8::Context> ScriptEngineV8::getContext() {
+#ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+    Q_ASSERT(!_wasDestroyed);
+#endif
     v8::EscapableHandleScope handleScope(_v8Isolate);
     Q_ASSERT(!_contexts.isEmpty());
     return handleScope.Escape(_contexts.last().get()->toV8Value());

libraries/script-engine/src/v8/ScriptEngineV8.h

@@ -48,6 +48,7 @@ class ScriptManager;
 class ScriptObjectV8Proxy;
 class ScriptMethodV8Proxy;
 class ScriptValueV8Wrapper;
+class ScriptSignalV8Proxy;
 
 template <typename T> class V8ScriptValueTemplate;
 typedef V8ScriptValueTemplate<v8::Value> V8ScriptValue;
@@ -57,6 +58,8 @@ using ScriptContextV8Pointer = std::shared_ptr<ScriptContextV8Wrapper>;
 
 const double GARBAGE_COLLECTION_TIME_LIMIT_S = 1.0;
 
+#define OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+
 Q_DECLARE_METATYPE(ScriptEngine::FunctionSignature)
 
 /// [V8] Implements ScriptEngine for V8 and translates calls for QScriptEngine
@@ -144,6 +147,7 @@ public:  // ScriptEngine implementation
     void scheduleValueWrapperForDeletion(ScriptValueV8Wrapper* wrapper) {_scriptValueWrappersToDelete.enqueue(wrapper);}
     void deleteUnusedValueWrappers();
     virtual void perManagerLoopIterationCleanup() override;
+    virtual void disconnectSignalProxies() override;
 
     // helper to detect and log warnings when other code invokes QScriptEngine/BaseScriptEngine in thread-unsafe ways
     inline bool IS_THREADSAFE_INVOCATION(const QString& method) { return ScriptEngine::IS_THREADSAFE_INVOCATION(method); }
@@ -276,12 +280,21 @@ private:
     std::atomic<size_t> scriptValueProxyCount{0};
 #endif
 
+#ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+    bool _wasDestroyed{false};
+#endif
     // Pointers to profiling classes. These are valid only when profiler is running, otherwise null
     // Smart pointer cannot be used here since profiler has private destructor
     v8::CpuProfiler *_profiler{nullptr};
     v8::ProfilerId _profilerId{0};
 
+    // Set of script signal proxy pointers. Used for disconnecting signals on cleanup.
+    // V8TODO: later it would be also worth to make sure that script proxies themselves get deleted together with script engine
+    QReadWriteLock _signalProxySetLock;
+    QSet<ScriptSignalV8Proxy*> _signalProxySet;
+
     friend ScriptValueV8Wrapper;
+    friend ScriptSignalV8Proxy;
 };
 
 // This class is used to automatically add context to script engine's context list that is used by C++ calls

libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp

@@ -1157,6 +1157,9 @@ ScriptSignalV8Proxy::ScriptSignalV8Proxy(ScriptEngineV8* engine, QObject* object
     _objectLifetime.Reset(isolate, lifetime.get());
     _objectLifetime.SetWeak(this, weakHandleCallback, v8::WeakCallbackType::kParameter);
     _v8Context.Reset(isolate, _engine->getContext());
+    _engine->_signalProxySetLock.lockForWrite();
+    _engine->_signalProxySet.insert(this);
+    _engine->_signalProxySetLock.unlock();
 }
 
 ScriptSignalV8Proxy::~ScriptSignalV8Proxy() {
@@ -1166,6 +1169,12 @@ ScriptSignalV8Proxy::~ScriptSignalV8Proxy() {
     v8::HandleScope handleScope(isolate);
     _objectLifetime.Reset();
     _v8Context.Reset();
+#ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+    Q_ASSERT(!_engine->_wasDestroyed);
+#endif
+    _engine->_signalProxySetLock.lockForWrite();
+    _engine->_signalProxySet.remove(this);
+    _engine->_signalProxySetLock.unlock();
 }
 
 void ScriptSignalV8Proxy::weakHandleCallback(const v8::WeakCallbackInfo<ScriptSignalV8Proxy>& info) {

libraries/script-engine/src/v8/ScriptObjectV8Proxy.h

@@ -270,6 +270,9 @@ public:  // API
     //Moved to public temporarily for debugging:
     QString fullName() const;
 
+    // Disconnects all signals from the proxy
+    void disconnectAll() { QObject::disconnect(this, nullptr, nullptr, nullptr); };
+
 private:  // storage
 
     ScriptEngineV8* _engine;

libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp

@@ -248,6 +248,9 @@ ScriptEnginePointer ScriptValueV8Wrapper::engine() const {
     if (!_engine) {
         return ScriptEnginePointer();
     }
+#ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+    Q_ASSERT(!_engine->_wasDestroyed);
+#endif
     return _engine->shared_from_this();
 }