Merge pull request #7236 from howard-stearns/entity-script-safety

entity script safety

libraries/script-engine/src/ScriptEngine.cpp

@@ -485,10 +485,10 @@ void ScriptEngine::removeEventHandler(const EntityItemID& entityID, const QStrin
         return;
     }
     RegisteredEventHandlers& handlersOnEntity = _registeredHandlers[entityID];
-    QScriptValueList& handlersForEvent = handlersOnEntity[eventName];
+    CallbackList& handlersForEvent = handlersOnEntity[eventName];
     // QScriptValue does not have operator==(), so we can't use QList::removeOne and friends. So iterate.
     for (int i = 0; i < handlersForEvent.count(); ++i) {
-        if (handlersForEvent[i].equals(handler)) {
+        if (handlersForEvent[i].function.equals(handler)) {
             handlersForEvent.removeAt(i);
             return; // Design choice: since comparison is relatively expensive, just remove the first matching handler.
         }
@@ -516,6 +516,13 @@ void ScriptEngine::addEventHandler(const EntityItemID& entityID, const QString&
         // Connect up ALL the handlers to the global entities object's signals.
         // (We could go signal by signal, or even handler by handler, but I don't think the efficiency is worth the complexity.)
         auto entities = DependencyManager::get<EntityScriptingInterface>();
+        // Bug? These handlers are deleted when entityID is deleted, which is nice.
+        // But if they are created by an entity script on a different entity, should they also be deleted when the entity script unloads?
+        // E.g., suppose a bow has an entity script that causes arrows to be created with a potential lifetime greater than the bow,
+        // and that the entity script adds (e.g., collision) handlers to the arrows. Should those handlers fire if the bow is unloaded?
+        // Also, what about when the entity script is REloaded?
+        // For now, we are leaving them around. Changing that would require some non-trivial digging around to find the
+        // handlers that were added while a given currentEntityIdentifier was in place. I don't think this is dangerous. Just perhaps unexpected. -HRS
         connect(entities.data(), &EntityScriptingInterface::deletingEntity, this, [this](const EntityItemID& entityID) {
             _registeredHandlers.remove(entityID);
         });
@@ -563,8 +570,9 @@ void ScriptEngine::addEventHandler(const EntityItemID& entityID, const QString&
     if (!_registeredHandlers.contains(entityID)) {
         _registeredHandlers[entityID] = RegisteredEventHandlers();
     }
-    QScriptValueList& handlersForEvent = _registeredHandlers[entityID][eventName];
+    CallbackList& handlersForEvent = _registeredHandlers[entityID][eventName];
-    handlersForEvent << handler; // Note that the same handler can be added many times. See removeEntityEventHandler().
+    CallbackData handlerData = {handler, currentEntityIdentifier};
+    handlersForEvent << handlerData; // Note that the same handler can be added many times. See removeEntityEventHandler().
 }
 
 
@@ -705,13 +713,30 @@ void ScriptEngine::run() {
 // NOTE: This is private because it must be called on the same thread that created the timers, which is why
 // we want to only call it in our own run "shutdown" processing.
 void ScriptEngine::stopAllTimers() {
-    QMutableHashIterator<QTimer*, QScriptValue> i(_timerFunctionMap);
+    QMutableHashIterator<QTimer*, CallbackData> i(_timerFunctionMap);
     while (i.hasNext()) {
         i.next();
         QTimer* timer = i.key();
         stopTimer(timer);
     }
 }
+void ScriptEngine::stopAllTimersForEntityScript(const EntityItemID& entityID) {
+     // We could maintain a separate map of entityID => QTimer, but someone will have to prove to me that it's worth the complexity. -HRS
+    QVector<QTimer*> toDelete;
+    QMutableHashIterator<QTimer*, CallbackData> i(_timerFunctionMap);
+    while (i.hasNext()) {
+        i.next();
+        if (i.value().definingEntityIdentifier != entityID) {
+            continue;
+        }
+        QTimer* timer = i.key();
+        toDelete << timer; // don't delete while we're iterating. save it.
+    }
+    for (auto timer:toDelete) { // now reap 'em
+        stopTimer(timer);
+    }
+
+}
 
 void ScriptEngine::stop() {
     if (!_isFinished) {
@@ -743,13 +768,14 @@ void ScriptEngine::callAnimationStateHandler(QScriptValue callback, AnimVariantM
     QScriptValue javascriptParameters = parameters.animVariantMapToScriptValue(this, names, useNames);
     QScriptValueList callingArguments;
     callingArguments << javascriptParameters;
+    assert(currentEntityIdentifier.isInvalidID()); // No animation state handlers from entity scripts.
     QScriptValue result = callback.call(QScriptValue(), callingArguments);
     resultHandler(result);
 }
 
 void ScriptEngine::timerFired() {
     QTimer* callingTimer = reinterpret_cast<QTimer*>(sender());
-    QScriptValue timerFunction = _timerFunctionMap.value(callingTimer);
+    CallbackData timerData = _timerFunctionMap.value(callingTimer);
 
     if (!callingTimer->isActive()) {
         // this timer is done, we can kill it
@@ -758,11 +784,12 @@ void ScriptEngine::timerFired() {
     }
 
     // call the associated JS function, if it exists
-    if (timerFunction.isValid()) {
+    if (timerData.function.isValid()) {
-        timerFunction.call();
+        callWithEnvironment(timerData.definingEntityIdentifier, timerData.function, timerData.function, QScriptValueList());
     }
 }
 
+
 QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int intervalMS, bool isSingleShot) {
     // create the timer, add it to the map, and start it
     QTimer* newTimer = new QTimer(this);
@@ -773,7 +800,8 @@ QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int
     // make sure the timer stops when the script does
     connect(this, &ScriptEngine::scriptEnding, newTimer, &QTimer::stop);
 
-    _timerFunctionMap.insert(newTimer, function);
+    CallbackData timerData = {function, currentEntityIdentifier};
+    _timerFunctionMap.insert(newTimer, timerData);
 
     newTimer->start(intervalMS);
     return newTimer;
@@ -859,6 +887,7 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac
     }
 
     BatchLoader* loader = new BatchLoader(urls);
+    EntityItemID capturedEntityIdentifier = currentEntityIdentifier;
 
     auto evaluateScripts = [=](const QMap<QUrl, QString>& data) {
         auto parentURL = _parentURL;
@@ -870,13 +899,16 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac
                 // Set the parent url so that path resolution will be relative
                 // to this script's url during its initial evaluation
                 _parentURL = url.toString();
-                QScriptValue result = evaluate(contents, url.toString());
+                auto operation = [&]() {
+                    evaluate(contents, url.toString());
+                };
+                doWithEnvironment(capturedEntityIdentifier, operation);
             }
         }
         _parentURL = parentURL;
 
         if (callback.isFunction()) {
-            QScriptValue(callback).call();
+            callWithEnvironment(capturedEntityIdentifier, QScriptValue(callback), QScriptValue(), QScriptValueList());
         }
 
         loader->deleteLater();
@@ -917,6 +949,11 @@ void ScriptEngine::load(const QString& loadFile) {
             << "loadFile:" << loadFile << "parent script:" << getFilename();
         return; // bail early
     }
+    if (!currentEntityIdentifier.isInvalidID()) {
+        qCWarning(scriptengine) << "Script.load() from entity script is ignored... "
+            << "loadFile:" << loadFile << "parent script:" << getFilename();
+        return; // bail early
+    }
 
     QUrl url = resolvePath(loadFile);
     if (_isReloading) {
@@ -933,7 +970,7 @@ void ScriptEngine::load(const QString& loadFile) {
 }
 
 // Look up the handler associated with eventName and entityID. If found, evalute the argGenerator thunk and call the handler with those args
-void ScriptEngine::forwardHandlerCall(const EntityItemID& entityID, const QString& eventName, QScriptValueList eventHanderArgs) {
+void ScriptEngine::forwardHandlerCall(const EntityItemID& entityID, const QString& eventName, QScriptValueList eventHandlerArgs) {
     if (QThread::currentThread() != thread()) {
         qDebug() << "*** ERROR *** ScriptEngine::forwardHandlerCall() called on wrong thread [" << QThread::currentThread() << "], invoking on correct thread [" << thread() << "]";
         assert(false);
@@ -946,10 +983,13 @@ void ScriptEngine::forwardHandlerCall(const EntityItemID& entityID, const QStrin
     if (!handlersOnEntity.contains(eventName)) {
         return;
     }
-    QScriptValueList handlersForEvent = handlersOnEntity[eventName];
+    CallbackList handlersForEvent = handlersOnEntity[eventName];
     if (!handlersForEvent.isEmpty()) {
         for (int i = 0; i < handlersForEvent.count(); ++i) {
-            handlersForEvent[i].call(QScriptValue(), eventHanderArgs);
+            // handlersForEvent[i] can tonain many handlers that may have each been added by different interface or entity scripts,
+            // and the entity scripts may be for entities other than the one this is a handler for.
+            // Fortunately, the definingEntityIdentifier captured the entity script id (if any) when the handler was added.
+            callWithEnvironment(handlersForEvent[i].definingEntityIdentifier, handlersForEvent[i].function, QScriptValue(), eventHandlerArgs);
         }
     }
 }
@@ -1055,9 +1095,13 @@ void ScriptEngine::entityScriptContentAvailable(const EntityItemID& entityID, co
         QString file = QUrl(scriptOrURL).toLocalFile();
         lastModified = (quint64)QFileInfo(file).lastModified().toMSecsSinceEpoch();
     }
+    QScriptValue entityScriptConstructor, entityScriptObject;
+    auto initialization = [&]{
+        entityScriptConstructor = evaluate(contents, fileName);
+        entityScriptObject = entityScriptConstructor.construct();
+    };
+    doWithEnvironment(entityID, initialization);
 
-    QScriptValue entityScriptConstructor = evaluate(contents, fileName);
-    QScriptValue entityScriptObject = entityScriptConstructor.construct();
     EntityScriptDetails newDetails = { scriptOrURL, entityScriptObject, lastModified };
     _entityScripts[entityID] = newDetails;
     if (isURL) {
@@ -1087,6 +1131,7 @@ void ScriptEngine::unloadEntityScript(const EntityItemID& entityID) {
     if (_entityScripts.contains(entityID)) {
         callEntityScriptMethod(entityID, "unload");
         _entityScripts.remove(entityID);
+        stopAllTimersForEntityScript(entityID);
     }
 }
 
@@ -1142,6 +1187,32 @@ void ScriptEngine::refreshFileScript(const EntityItemID& entityID) {
     recurseGuard = false;
 }
 
+// Execute operation in the appropriate context for (the possibly empty) entityID.
+// Even if entityID is supplied as currentEntityIdentifier, this still documents the source
+// of the code being executed (e.g., if we ever sandbox different entity scripts, or provide different
+// global values for different entity scripts).
+void ScriptEngine::doWithEnvironment(const EntityItemID& entityID, std::function<void()> operation) {
+    EntityItemID oldIdentifier = currentEntityIdentifier;
+    currentEntityIdentifier = entityID;
+
+#if DEBUG_CURRENT_ENTITY
+    QScriptValue oldData = this->globalObject().property("debugEntityID");
+    this->globalObject().setProperty("debugEntityID", entityID.toScriptValue(this)); // Make the entityID available to javascript as a global.
+    operation();
+    this->globalObject().setProperty("debugEntityID", oldData);
+#else
+    operation();
+#endif
+
+    currentEntityIdentifier = oldIdentifier;
+}
+void ScriptEngine::callWithEnvironment(const EntityItemID& entityID, QScriptValue function, QScriptValue thisObject, QScriptValueList args) {
+    auto operation = [&]() {
+        function.call(thisObject, args);
+    };
+    doWithEnvironment(entityID, operation);
+}
+
 void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QString& methodName, const QStringList& params) {
     if (QThread::currentThread() != thread()) {
 #ifdef THREAD_DEBUGGING
@@ -1168,7 +1239,7 @@ void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QS
             QScriptValueList args;
             args << entityID.toScriptValue(this);
             args << qScriptValueFromSequence(this, params);
-            entityScript.property(methodName).call(entityScript, args);
+            callWithEnvironment(entityID, entityScript.property(methodName), entityScript, args);
         }
 
     }
@@ -1200,7 +1271,7 @@ void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QS
             QScriptValueList args;
             args << entityID.toScriptValue(this);
             args << event.toScriptValue(this);
-            entityScript.property(methodName).call(entityScript, args);
+            callWithEnvironment(entityID, entityScript.property(methodName), entityScript, args);
         }
     }
 }
@@ -1234,7 +1305,7 @@ void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QS
             args << entityID.toScriptValue(this);
             args << otherID.toScriptValue(this);
             args << collisionToScriptValue(this, collision);
-            entityScript.property(methodName).call(entityScript, args);
+            callWithEnvironment(entityID, entityScript.property(methodName), entityScript, args);
         }
     }
 }

libraries/script-engine/src/ScriptEngine.h

@@ -42,7 +42,14 @@ const QString NO_SCRIPT("");
 
 const unsigned int SCRIPT_DATA_CALLBACK_USECS = floor(((1.0f / 60.0f) * 1000 * 1000) + 0.5f);
 
-typedef QHash<QString, QScriptValueList> RegisteredEventHandlers;
+class CallbackData {
+public:
+    QScriptValue function;
+    EntityItemID definingEntityIdentifier;
+};
+
+typedef QList<CallbackData> CallbackList;
+typedef QHash<QString, CallbackList> RegisteredEventHandlers;
 
 class EntityScriptDetails {
 public:
@@ -169,7 +176,7 @@ protected:
     std::atomic<bool> _isRunning { false };
     int _evaluatesPending { 0 };
     bool _isInitialized { false };
-    QHash<QTimer*, QScriptValue> _timerFunctionMap;
+    QHash<QTimer*, CallbackData> _timerFunctionMap;
     QSet<QUrl> _includedURLs;
     bool _wantSignals { true };
     QHash<EntityItemID, EntityScriptDetails> _entityScripts;
@@ -181,6 +188,7 @@ protected:
     bool evaluatePending() const { return _evaluatesPending > 0; }
     void timerFired();
     void stopAllTimers();
+    void stopAllTimersForEntityScript(const EntityItemID& entityID);
     void refreshFileScript(const EntityItemID& entityID);
 
     void setParentURL(const QString& parentURL) { _parentURL = parentURL; }
@@ -203,6 +211,10 @@ protected:
     void forwardHandlerCall(const EntityItemID& entityID, const QString& eventName, QScriptValueList eventHanderArgs);
     Q_INVOKABLE void entityScriptContentAvailable(const EntityItemID& entityID, const QString& scriptOrURL, const QString& contents, bool isURL, bool success);
 
+    EntityItemID currentEntityIdentifier {}; // Contains the defining entity script entity id during execution, if any. Empty for interface script execution.
+    void doWithEnvironment(const EntityItemID& entityID, std::function<void()> operation);
+    void callWithEnvironment(const EntityItemID& entityID, QScriptValue function, QScriptValue thisObject, QScriptValueList args);
+
     friend class ScriptEngines;
     static std::atomic<bool> _stoppingAllScripts;
 };