Merge pull request #7692 from ZappoMan/resetScriptEngineOnDomainChange

Reset entities script engine on domain change

examples/entityScripts/exampleSelfCallingTimeoutNoCleanup.js

@@ -0,0 +1,56 @@
+//
+//  exampleSelfCallingTimeoutNoCleanup.js
+//  examples/entityScripts
+//
+//  Created by Brad Hefta-Gaub on 4/18/16.
+//  Copyright 2016 High Fidelity, Inc.
+//
+//  This is an example of an entity script which hooks the update signal
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+(function() {
+    var _this;
+
+    // this is the "constructor" for the entity as a JS object we don't do much here, but we do want to remember
+    // our this object, so we can access it in cases where we're called without a this (like in the case of various global signals)
+    ExampleUpdate = function() { 
+         _this = this;
+    };
+
+    ExampleUpdate.prototype = {
+
+        timeOutFunction: function() {
+            var entityID = _this.entityID;
+            print("timeOutFunction in entityID:" + entityID);
+            Script.setTimeout(function() {
+                _this.timeOutFunction();
+            }, 3000);
+        },
+
+        // preload() will be called when the entity has become visible (or known) to the interface
+        // it gives us a chance to set our local JavaScript object up. In this case it means:
+        //   * remembering our entityID, so we can access it in cases where we're called without an entityID
+        //   * connecting to the update signal so we can check our grabbed state
+        preload: function(entityID) {
+            print("preload - entityID:" + entityID);
+            this.entityID = entityID;
+
+            print("preload - entityID:" + entityID + "-- calling timeOutFunction()....");
+            _this.timeOutFunction();
+        },
+
+        // unload() will be called when our entity is no longer available. It may be because we were deleted,
+        // or because we've left the domain or quit the application. In all cases we want to unhook our connection
+        // to the update signal
+        unload: function(entityID) {
+            print("unload - entityID:" + entityID);
+            print("NOTE --- WE DID NOT CALL clear our timeout");
+        },
+    };
+
+    // entity scripts always need to return a newly constructed object of our type
+    return new ExampleUpdate();
+})

examples/entityScripts/exampleTimeoutNoCleanup.js

@@ -0,0 +1,50 @@
+//
+//  exampleTimeoutNoCleanup.js
+//  examples/entityScripts
+//
+//  Created by Brad Hefta-Gaub on 4/18/16.
+//  Copyright 2016 High Fidelity, Inc.
+//
+//  This is an example of an entity script which hooks the update signal
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+(function() {
+    var _this;
+
+    // this is the "constructor" for the entity as a JS object we don't do much here, but we do want to remember
+    // our this object, so we can access it in cases where we're called without a this (like in the case of various global signals)
+    ExampleUpdate = function() { 
+         _this = this;
+    };
+
+    ExampleUpdate.prototype = {
+
+        // preload() will be called when the entity has become visible (or known) to the interface
+        // it gives us a chance to set our local JavaScript object up. In this case it means:
+        //   * remembering our entityID, so we can access it in cases where we're called without an entityID
+        //   * connecting to the update signal so we can check our grabbed state
+        preload: function(entityID) {
+            print("preload - entityID:" + entityID);
+            this.entityID = entityID;
+
+            Script.setInterval(function() {
+                    var entityID = _this.entityID;
+                    print("timer interval in entityID:" + entityID);
+                }, 3000);
+        },
+
+        // unload() will be called when our entity is no longer available. It may be because we were deleted,
+        // or because we've left the domain or quit the application. In all cases we want to unhook our connection
+        // to the update signal
+        unload: function(entityID) {
+            print("unload - entityID:" + entityID);
+            print("NOTE --- WE DID NOT CALL clear our timeout");
+        },
+    };
+
+    // entity scripts always need to return a newly constructed object of our type
+    return new ExampleUpdate();
+})

examples/entityScripts/exampleUpdate.js

@@ -0,0 +1,54 @@
+//
+//  exampleUpdate.js
+//  examples/entityScripts
+//
+//  Created by Brad Hefta-Gaub on 4/18/16.
+//  Copyright 2016 High Fidelity, Inc.
+//
+//  This is an example of an entity script which hooks the update signal
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+(function() {
+    var _this;
+
+    // this is the "constructor" for the entity as a JS object we don't do much here, but we do want to remember
+    // our this object, so we can access it in cases where we're called without a this (like in the case of various global signals)
+    ExampleUpdate = function() { 
+         _this = this;
+    };
+
+    ExampleUpdate.prototype = {
+
+        // update() will be called regulary, because we've hooked the update signal in our preload() function.
+        // we will check the avatars hand positions and if either hand is in our bounding box, we will notice that
+        update: function() {
+            // because the update() signal doesn't have a valid this, we need to use our memorized _this to access our entityID
+            var entityID = _this.entityID;
+            print("update in entityID:" + entityID);
+        },
+
+        // preload() will be called when the entity has become visible (or known) to the interface
+        // it gives us a chance to set our local JavaScript object up. In this case it means:
+        //   * remembering our entityID, so we can access it in cases where we're called without an entityID
+        //   * connecting to the update signal so we can check our grabbed state
+        preload: function(entityID) {
+            print("preload - entityID:" + entityID);
+            this.entityID = entityID;
+            Script.update.connect(this.update);
+        },
+
+        // unload() will be called when our entity is no longer available. It may be because we were deleted,
+        // or because we've left the domain or quit the application. In all cases we want to unhook our connection
+        // to the update signal
+        unload: function(entityID) {
+            print("unload - entityID:" + entityID);
+            Script.update.disconnect(this.update);
+        },
+    };
+
+    // entity scripts always need to return a newly constructed object of our type
+    return new ExampleUpdate();
+})

examples/entityScripts/exampleUpdateNoDisconnect.js

@@ -0,0 +1,54 @@
+//
+//  exampleUpdateNoDisconnect.js
+//  examples/entityScripts
+//
+//  Created by Brad Hefta-Gaub on 4/18/16.
+//  Copyright 2016 High Fidelity, Inc.
+//
+//  This is an example of an entity script which hooks the update signal
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+(function() {
+    var _this;
+
+    // this is the "constructor" for the entity as a JS object we don't do much here, but we do want to remember
+    // our this object, so we can access it in cases where we're called without a this (like in the case of various global signals)
+    ExampleUpdate = function() { 
+         _this = this;
+    };
+
+    ExampleUpdate.prototype = {
+
+        // update() will be called regulary, because we've hooked the update signal in our preload() function.
+        // we will check the avatars hand positions and if either hand is in our bounding box, we will notice that
+        update: function() {
+            // because the update() signal doesn't have a valid this, we need to use our memorized _this to access our entityID
+            var entityID = _this.entityID;
+            print("update in entityID:" + entityID);
+        },
+
+        // preload() will be called when the entity has become visible (or known) to the interface
+        // it gives us a chance to set our local JavaScript object up. In this case it means:
+        //   * remembering our entityID, so we can access it in cases where we're called without an entityID
+        //   * connecting to the update signal so we can check our grabbed state
+        preload: function(entityID) {
+            print("preload - entityID:" + entityID);
+            this.entityID = entityID;
+            Script.update.connect(this.update);
+        },
+
+        // unload() will be called when our entity is no longer available. It may be because we were deleted,
+        // or because we've left the domain or quit the application. In all cases we want to unhook our connection
+        // to the update signal
+        unload: function(entityID) {
+            print("unload - entityID:" + entityID);
+            print("NOTE --- WE DID NOT CALL Script.update.disconnect()");
+        },
+    };
+
+    // entity scripts always need to return a newly constructed object of our type
+    return new ExampleUpdate();
+})

interface/src/Application.cpp

@@ -1178,8 +1178,6 @@ void Application::cleanupBeforeQuit() {
     }
     _keyboardFocusHighlight = nullptr;
 
-    getEntities()->clear(); // this will allow entity scripts to properly shutdown
-
     auto nodeList = DependencyManager::get<NodeList>();
 
     // send the domain a disconnect packet, force stoppage of domain-server check-ins
@@ -1190,6 +1188,7 @@ void Application::cleanupBeforeQuit() {
     nodeList->getPacketReceiver().setShouldDropPackets(true);
 
     getEntities()->shutdown(); // tell the entities system we're shutting down, so it will stop running scripts
+
     DependencyManager::get<ScriptEngines>()->saveScripts();
     DependencyManager::get<ScriptEngines>()->shutdownScripting(); // stop all currently running global scripts
     DependencyManager::destroy<ScriptEngines>();
@@ -4184,8 +4183,13 @@ void Application::updateWindowTitle() const {
 }
 
 void Application::clearDomainOctreeDetails() {
+
+    // if we're about to quit, we really don't need to do any of these things...
+    if (_aboutToQuit) {
+        return;
+    }
+
     qCDebug(interfaceapp) << "Clearing domain octree details...";
-    // reset the environment so that we don't erroneously end up with multiple
 
     resetPhysicsReadyInformation();
 
@@ -4209,7 +4213,6 @@ void Application::clearDomainOctreeDetails() {
 
 void Application::domainChanged(const QString& domainHostname) {
     updateWindowTitle();
-    clearDomainOctreeDetails();
     // disable physics until we have enough information about our new location to not cause craziness.
     resetPhysicsReadyInformation();
 }

libraries/entities-renderer/src/EntityTreeRenderer.cpp

@@ -75,9 +75,29 @@ EntityTreeRenderer::~EntityTreeRenderer() {
     //       it is registered with ScriptEngines, which will call deleteLater for us.
 }
 
+int EntityTreeRenderer::_entitiesScriptEngineCount = 0;
+
+void EntityTreeRenderer::setupEntitiesScriptEngine() {
+    QSharedPointer<ScriptEngine> oldEngine = _entitiesScriptEngine; // save the old engine through this function, so the EntityScriptingInterface doesn't have problems with it.
+    _entitiesScriptEngine = QSharedPointer<ScriptEngine>(new ScriptEngine(NO_SCRIPT, QString("Entities %1").arg(++_entitiesScriptEngineCount)), &QObject::deleteLater);
+    _scriptingServices->registerScriptEngineWithApplicationServices(_entitiesScriptEngine.data());
+    _entitiesScriptEngine->runInThread();
+    DependencyManager::get<EntityScriptingInterface>()->setEntitiesScriptEngine(_entitiesScriptEngine.data());
+}
+
 void EntityTreeRenderer::clear() {
     leaveAllEntities();
-    _entitiesScriptEngine->unloadAllEntityScripts();
+    if (_entitiesScriptEngine) {
+        _entitiesScriptEngine->unloadAllEntityScripts();
+        _entitiesScriptEngine->stop();
+    }
+
+    if (_wantScripts && !_shuttingDown) {
+        // NOTE: you can't actually need to delete it here because when we call setupEntitiesScriptEngine it will
+        //       assign a new instance to our shared pointer, which will deref the old instance and ultimately call
+        //       the custom deleter which calls deleteLater
+        setupEntitiesScriptEngine();
+    }
 
     auto scene = _viewState->getMain3DScene();
     render::PendingChanges pendingChanges;
@@ -94,7 +114,7 @@ void EntityTreeRenderer::reloadEntityScripts() {
     _entitiesScriptEngine->unloadAllEntityScripts();
     foreach(auto entity, _entitiesInScene) {
         if (!entity->getScript().isEmpty()) {
-            _entitiesScriptEngine->loadEntityScript(entity->getEntityItemID(), entity->getScript(), true);
+            ScriptEngine::loadEntityScript(_entitiesScriptEngine, entity->getEntityItemID(), entity->getScript(), true);
         }
     }
 }
@@ -105,10 +125,7 @@ void EntityTreeRenderer::init() {
     entityTree->setFBXService(this);
 
     if (_wantScripts) {
-        _entitiesScriptEngine = new ScriptEngine(NO_SCRIPT, "Entities");
-        _scriptingServices->registerScriptEngineWithApplicationServices(_entitiesScriptEngine);
-        _entitiesScriptEngine->runInThread();
-        DependencyManager::get<EntityScriptingInterface>()->setEntitiesScriptEngine(_entitiesScriptEngine);
+        setupEntitiesScriptEngine();
     }
 
     forceRecheckEntities(); // setup our state to force checking our inside/outsideness of entities
@@ -122,6 +139,8 @@ void EntityTreeRenderer::init() {
 void EntityTreeRenderer::shutdown() {
     _entitiesScriptEngine->disconnectNonEssentialSignals(); // disconnect all slots/signals from the script engine, except essential
     _shuttingDown = true;
+
+    clear(); // always clear() on shutdown
 }
 
 void EntityTreeRenderer::setTree(OctreePointer newTree) {
@@ -763,7 +782,7 @@ void EntityTreeRenderer::checkAndCallPreload(const EntityItemID& entityID, const
         if (entity && entity->shouldPreloadScript()) {
             QString scriptUrl = entity->getScript();
             scriptUrl = ResourceManager::normalizeURL(scriptUrl);
-            _entitiesScriptEngine->loadEntityScript(entityID, scriptUrl, reload);
+            ScriptEngine::loadEntityScript(_entitiesScriptEngine, entityID, scriptUrl, reload);
             entity->scriptHasPreloaded();
         }
     }

libraries/entities-renderer/src/EntityTreeRenderer.h

@@ -126,6 +126,8 @@ protected:
     }
 
 private:
+    void setupEntitiesScriptEngine();
+
     void addEntityToScene(EntityItemPointer entity);
     bool findBestZoneAndMaybeContainingEntities(const glm::vec3& avatarPosition, QVector<EntityItemID>* entitiesContainingAvatar);
 
@@ -155,7 +157,7 @@ private:
     NetworkTexturePointer _ambientTexture;
 
     bool _wantScripts;
-    ScriptEngine* _entitiesScriptEngine;
+    QSharedPointer<ScriptEngine> _entitiesScriptEngine;
 
     bool isCollisionOwner(const QUuid& myNodeID, EntityTreePointer entityTree,
                           const EntityItemID& id, const Collision& collision);
@@ -196,6 +198,8 @@ private:
     QHash<EntityItemID, EntityItemPointer> _entitiesInScene;
     // For Scene.shouldRenderEntities
     QList<EntityItemID> _entityIDsLastInScene;
+
+    static int _entitiesScriptEngineCount;
 };
 
 

libraries/entities/src/EntityScriptingInterface.cpp

@@ -414,7 +414,13 @@ void EntityScriptingInterface::deleteEntity(QUuid id) {
     }
 }
 
+void EntityScriptingInterface::setEntitiesScriptEngine(EntitiesScriptEngineProvider* engine) {
+    std::lock_guard<std::mutex> lock(_entitiesScriptEngineLock);
+    _entitiesScriptEngine = engine;
+}
+
 void EntityScriptingInterface::callEntityMethod(QUuid id, const QString& method, const QStringList& params) {
+    std::lock_guard<std::mutex> lock(_entitiesScriptEngineLock);
     if (_entitiesScriptEngine) {
         EntityItemID entityID{ id };
         _entitiesScriptEngine->callEntityScriptMethod(entityID, method, params);

libraries/entities/src/EntityScriptingInterface.h

@@ -71,7 +71,7 @@ public:
 
     void setEntityTree(EntityTreePointer modelTree);
     EntityTreePointer getEntityTree() { return _entityTree; }
-    void setEntitiesScriptEngine(EntitiesScriptEngineProvider* engine) { _entitiesScriptEngine = engine; }
+    void setEntitiesScriptEngine(EntitiesScriptEngineProvider* engine);
     float calculateCost(float mass, float oldVelocity, float newVelocity);
 public slots:
 
@@ -214,6 +214,8 @@ private:
         bool precisionPicking, const QVector<EntityItemID>& entityIdsToInclude, const QVector<EntityItemID>& entityIdsToDiscard);
 
     EntityTreePointer _entityTree;
+
+    std::mutex _entitiesScriptEngineLock;
     EntitiesScriptEngineProvider* _entitiesScriptEngine { nullptr };
     
     bool _bidOnSimulationOwnership { false };

libraries/script-engine/src/ScriptEngine.cpp

@@ -18,6 +18,7 @@
 #include <QtNetwork/QNetworkReply>
 #include <QtScript/QScriptEngine>
 #include <QtScript/QScriptValue>
+#include <QtScript/QScriptValueIterator>
 #include <QtCore/QStringList>
 
 #include <AudioConstants.h>
@@ -143,7 +144,6 @@ ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNam
 
 ScriptEngine::~ScriptEngine() {
     qCDebug(scriptengine) << "Script Engine shutting down (destructor) for script:" << getFilename();
-
     auto scriptEngines = DependencyManager::get<ScriptEngines>();
     if (scriptEngines) {
         scriptEngines->removeScriptEngine(this);
@@ -1047,39 +1047,25 @@ void ScriptEngine::forwardHandlerCall(const EntityItemID& entityID, const QStrin
 
 // since all of these operations can be asynch we will always do the actual work in the response handler
 // for the download
-void ScriptEngine::loadEntityScript(const EntityItemID& entityID, const QString& entityScript, bool forceRedownload) {
-    if (QThread::currentThread() != thread()) {
+void ScriptEngine::loadEntityScript(QWeakPointer<ScriptEngine> theEngine, const EntityItemID& entityID, const QString& entityScript, bool forceRedownload) {
+    // NOTE: If the script content is not currently in the cache, the LAMBDA here will be called on the Main Thread
+    //       which means we're guaranteed that it's not the correct thread for the ScriptEngine. This means
+    //       when we get into entityScriptContentAvailable() we will likely invokeMethod() to get it over
+    //       to the "Entities" ScriptEngine thread.
+    DependencyManager::get<ScriptCache>()->getScriptContents(entityScript, [theEngine, entityID](const QString& scriptOrURL, const QString& contents, bool isURL, bool success) {
+        QSharedPointer<ScriptEngine> strongEngine = theEngine.toStrongRef();
+        if (strongEngine) {
 #ifdef THREAD_DEBUGGING
-        qDebug() << "*** WARNING *** ScriptEngine::loadEntityScript() called on wrong thread ["
-            << QThread::currentThread() << "], invoking on correct thread [" << thread() << "]  "
-            "entityID:" << entityID << "entityScript:" << entityScript <<"forceRedownload:" << forceRedownload;
+            qDebug() << "ScriptEngine::entityScriptContentAvailable() IN LAMBDA contentAvailable on thread ["
+                << QThread::currentThread() << "] expected thread [" << strongEngine->thread() << "]";
 #endif
-
-        QMetaObject::invokeMethod(this, "loadEntityScript",
-                                  Q_ARG(const EntityItemID&, entityID),
-                                  Q_ARG(const QString&, entityScript),
-                                  Q_ARG(bool, forceRedownload));
-        return;
-    }
-#ifdef THREAD_DEBUGGING
-    qDebug() << "ScriptEngine::loadEntityScript() called on correct thread [" << thread() << "]  "
-        "entityID:" << entityID << "entityScript:" << entityScript << "forceRedownload:" << forceRedownload;
-#endif
-
-    // If we've been called our known entityScripts should not know about us..
-    assert(!_entityScripts.contains(entityID));
-
-#ifdef THREAD_DEBUGGING
-    qDebug() << "ScriptEngine::loadEntityScript() calling scriptCache->getScriptContents() on thread ["
-        << QThread::currentThread() << "] expected thread [" << thread() << "]";
-#endif
-    DependencyManager::get<ScriptCache>()->getScriptContents(entityScript, [=](const QString& scriptOrURL, const QString& contents, bool isURL, bool success) {
-#ifdef THREAD_DEBUGGING
-        qDebug() << "ScriptEngine::entityScriptContentAvailable() IN LAMBDA contentAvailable on thread ["
-            << QThread::currentThread() << "] expected thread [" << thread() << "]";
-#endif
-
-        this->entityScriptContentAvailable(entityID, scriptOrURL, contents, isURL, success);
+            strongEngine->entityScriptContentAvailable(entityID, scriptOrURL, contents, isURL, success);
+        } else {
+            // FIXME - I'm leaving this in for testing, so that QA can confirm that sometimes the script contents
+            //         returns after the ScriptEngine has been deleted, we can remove this after QA verifies the
+            //         repro case.
+            qDebug() << "ScriptCache::getScriptContents() returned after our ScriptEngine was deleted... script:" << scriptOrURL;
+        }
     }, forceRedownload);
 }
 
@@ -1213,6 +1199,16 @@ void ScriptEngine::unloadAllEntityScripts() {
         callEntityScriptMethod(entityID, "unload");
     }
     _entityScripts.clear();
+
+#ifdef DEBUG_ENGINE_STATE
+    qDebug() << "---- CURRENT STATE OF ENGINE: --------------------------";
+    QScriptValueIterator it(globalObject());
+    while (it.hasNext()) {
+        it.next();
+        qDebug() << it.name() << ":" << it.value().toString();
+    }
+    qDebug() << "--------------------------------------------------------";
+#endif // DEBUG_ENGINE_STATE
 }
 
 void ScriptEngine::refreshFileScript(const EntityItemID& entityID) {

libraries/script-engine/src/ScriptEngine.h

@@ -124,7 +124,7 @@ public:
     Q_INVOKABLE QUrl resolvePath(const QString& path) const;
 
     // Entity Script Related methods
-    Q_INVOKABLE void loadEntityScript(const EntityItemID& entityID, const QString& entityScript, bool forceRedownload = false); // will call the preload method once loaded
+    static void loadEntityScript(QWeakPointer<ScriptEngine> theEngine, const EntityItemID& entityID, const QString& entityScript, bool forceRedownload);
     Q_INVOKABLE void unloadEntityScript(const EntityItemID& entityID); // will call unload method
     Q_INVOKABLE void unloadAllEntityScripts();
     Q_INVOKABLE void callEntityScriptMethod(const EntityItemID& entityID, const QString& methodName, const QStringList& params = QStringList());