From 67e7a7375aba97e642e8d07b26b1340e44950b8f Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Sun, 5 Mar 2023 00:43:04 +0100 Subject: [PATCH] Further exception work on V8 * Get rid of maybeEmitUncaughtException * Mostly get rid of makeError * Introduce exception hierarchy, change exceptions to shared_ptr * Simplify exception throwing code --- .../entities/src/EntityScriptingInterface.cpp | 22 +- libraries/script-engine/src/ScriptEngine.h | 68 +++--- libraries/script-engine/src/ScriptException.h | 101 ++++++++ libraries/script-engine/src/ScriptManager.cpp | 62 ++--- libraries/script-engine/src/ScriptManager.h | 3 +- .../src/ScriptManagerScriptingInterface.cpp | 9 +- .../src/ScriptManagerScriptingInterface.h | 4 + .../script-engine/src/v8/ScriptEngineV8.cpp | 217 ++++-------------- .../script-engine/src/v8/ScriptEngineV8.h | 22 +- .../script-engine/src/tests/010_exception.js | 1 + 10 files changed, 249 insertions(+), 260 deletions(-) create mode 100644 tests/script-engine/src/tests/010_exception.js diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 01892b2783..605c0b8d86 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -132,7 +132,7 @@ void EntityScriptingInterface::releaseEntityPacketSenderMessages(bool wait) { void EntityScriptingInterface::attachDefaultEventHandlers(ScriptManager* manager) { // 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.) - + // 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, @@ -194,7 +194,7 @@ void EntityScriptingInterface::attachDefaultEventHandlers(ScriptManager* manager }; /*@jsdoc - *

The name of an entity event. When the entity event occurs, any function that has been registered for that event + *

The name of an entity event. When the entity event occurs, any function that has been registered for that event * via {@link Script.addEventHandler} is called with parameters per the entity event.

* * @@ -782,7 +782,7 @@ QUuid EntityScriptingInterface::cloneEntity(const QUuid& entityIDToClone) { } else if (cloneAvatarEntity) { return addEntityInternal(properties, entity::HostType::AVATAR); } else { - // setLastEdited timestamp to 0 to ensure this entity gets updated with the properties + // setLastEdited timestamp to 0 to ensure this entity gets updated with the properties // from the server-created entity, don't change this unless you know what you are doing properties.setLastEdited(0); bool success = addLocalEntityCopy(properties, newEntityID, true); @@ -1255,7 +1255,7 @@ void EntityScriptingInterface::setNonPersistentEntitiesScriptEngine(std::shared_ void EntityScriptingInterface::callEntityMethod(const QUuid& id, const QString& method, const QStringList& params) { PROFILE_RANGE(script_entities, __FUNCTION__); - + auto entity = getEntityTree()->findEntityByEntityItemID(id); if (entity) { std::lock_guard lock(_entitiesScriptEngineLock); @@ -1605,7 +1605,7 @@ bool EntityScriptingInterface::queryPropertyMetadata(const QUuid& entityID, #endif if (!handler.property("callback").isFunction()) { qDebug() << "!handler.callback.isFunction" << manager; - engine->raiseException(engine->makeError(engine->newValue("callback is not a function"), "TypeError")); + engine->raiseException("callback is not a function", "TypeError"); return false; } @@ -1628,8 +1628,7 @@ bool EntityScriptingInterface::queryPropertyMetadata(const QUuid& entityID, } else if (name == "serverScripts") { return request.serverScripts(entityID, handler); } else { - engine->raiseException(engine->makeError(engine->newValue("metadata for property " + name + " is not yet queryable"))); - engine->maybeEmitUncaughtException(__FUNCTION__); + engine->raiseException("metadata for property " + name + " is not yet queryable"); return false; } } @@ -1644,8 +1643,7 @@ bool EntityScriptingInterface::getServerScriptStatus(const QUuid& entityID, Scri 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__); + engine->raiseException("This script does not belong to a ScriptManager"); return false; } @@ -2015,17 +2013,17 @@ EntityItemPointer EntityScriptingInterface::checkForTreeEntityAndTypeMatch(const if (!_entityTree) { return EntityItemPointer(); } - + EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); if (!entity) { qCDebug(entities) << "EntityScriptingInterface::checkForTreeEntityAndTypeMatch - no entity with ID" << entityID; return entity; } - + if (entityType != EntityTypes::Unknown && entity->getType() != entityType) { return EntityItemPointer(); } - + return entity; } diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 5e60c79238..bbefed3147 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -179,14 +179,6 @@ public: */ virtual void clearExceptions() = 0; - /** - * @brief Creates a clone of the current exception - * - * @param detail Additional text to add to the report - * @return ScriptValue Result - */ - virtual ScriptValue cloneUncaughtException(const QString& detail = QString()) = 0; - /** * @brief Context of the currently running script * @@ -274,15 +266,6 @@ public: */ virtual ScriptValue checkScriptSyntax(ScriptProgramPointer program) = 0; - /** - * @brief Creates a ScriptValue that contains an error - * - * @param other - * @param type - * @return ScriptValue - */ - virtual ScriptValue makeError(const ScriptValue& other = ScriptValue(), const QString& type = "Error") = 0; - /** * @brief Pointer to the ScriptManager that controls this scripting engine * @@ -290,19 +273,6 @@ public: */ ScriptManager* manager() const { return _manager; } - /** - * @brief Emit the current uncaught exception if there is one - * - * If there's an uncaught exception, emit it, and clear the exception status. - * - * This fails if there's no uncaught exception, there's no ScriptManager, - * or the engine is evaluating. - * - * @param debugHint A debugging hint to be added to the error message - * @return true There was an uncaught exception, and it was emitted - * @return false There was no uncaught exception - */ - virtual bool maybeEmitUncaughtException(const QString& debugHint = QString()) = 0; virtual ScriptValue newArray(uint length = 0) = 0; virtual ScriptValue newArrayBuffer(const QByteArray& message) = 0; virtual ScriptValue newFunction(FunctionSignature fun, int length = 0) { @@ -323,14 +293,39 @@ public: virtual ScriptValue nullValue() = 0; + /** + * @brief Make a ScriptValue that contains an error + * + * This is used to throw an error inside the running script + * + * @param other + * @param type + * @return ScriptValue ScriptValue containing error + */ + virtual ScriptValue makeError(const ScriptValue& other, const QString& type = "Error") = 0; + + /** * @brief Causes an exception to be raised in the currently executing script * * @param exception Exception to be thrown in the script + * @param reason Explanatory text about why the exception happened, for logging * @return true Exception was successfully thrown * @return false Exception couldn't be thrown because no script is running */ - virtual bool raiseException(const ScriptValue& exception) = 0; + virtual bool raiseException(const ScriptValue& exception, const QString &reason = QString()) = 0; + + /** + * @brief Causes an exception to be raised in the currently executing script + * + * @param error Exception to be thrown in the script + * @param reason Explanatory text about why the exception happened, for logging + * @return true Exception was successfully thrown + * @return false Exception couldn't be thrown because no script is running + */ + virtual bool raiseException(const QString& error, const QString &reason = QString()) = 0; + + virtual void registerEnum(const QString& enumName, QMetaEnum newEnum) = 0; virtual void registerFunction(const QString& name, FunctionSignature fun, int numArguments = -1) = 0; virtual void registerFunction(const QString& parent, const QString& name, FunctionSignature fun, int numArguments = -1) = 0; @@ -346,11 +341,14 @@ public: virtual ScriptValue undefinedValue() = 0; /** - * @brief Last uncaught exception, if any + * @brief Last uncaught exception, if any. * - * @return ScriptValue Uncaught exception from the script + * The returned shared pointer is newly allocated by the function, + * and modifying it has no effect on the internal state of the ScriptEngine. + * + * @return std::shared_ptr Uncaught exception from the script */ - virtual ScriptException uncaughtException() const = 0; + virtual std::shared_ptr uncaughtException() const = 0; virtual void updateMemoryCost(const qint64& deltaSize) = 0; virtual void requestCollectGarbage() = 0; @@ -403,7 +401,7 @@ signals: * * @param exception Exception that was thrown */ - void exception(const ScriptException &exception); + void exception(std::shared_ptr exception); protected: ~ScriptEngine() {} // prevent explicit deletion of base class diff --git a/libraries/script-engine/src/ScriptException.h b/libraries/script-engine/src/ScriptException.h index 174099bb4d..89a48fa4b2 100644 --- a/libraries/script-engine/src/ScriptException.h +++ b/libraries/script-engine/src/ScriptException.h @@ -12,12 +12,15 @@ #include #include +#include "ScriptValue.h" + #pragma once /** * @brief Scripting exception * * Emitted from the scripting engine when an exception happens inside it. + * This is the base class. * */ class ScriptException { @@ -63,6 +66,89 @@ class ScriptException { QStringList backtrace; bool isEmpty() const { return errorMessage.isEmpty(); } + + /** + * @brief Clones this object. + * + * This is used in the scripting engine to ensure that while it can return different + * exception objects depending on what happened, the returned object is a copy that + * doesn't allow the caller to accidentally break the ScriptEngine's internal state. + * + * @return std::shared_ptr + */ + virtual std::shared_ptr clone() const { + return std::make_shared(*this); + } +}; + + +/** + * @brief Exception that ocurred inside the scripting engine on the c++ side + * + * This is something that went wrong inside the ScriptEngine or ScriptManager + * infrastructure. + * + */ +class ScriptEngineException : public ScriptException { + public: + + ScriptEngineException(QString message = "", QString info = "", int line = 0, int column = 0, QStringList backtraceList = QStringList()) : + ScriptException(message, info, line, column, backtraceList) { + + } + + + /** + * @brief Clones this object. + * + * This is used in the scripting engine to ensure that while it can return different + * exception objects depending on what happened, the returned object is a copy that + * doesn't allow the caller to accidentally break the ScriptEngine's internal state. + * + * @return std::shared_ptr + */ + virtual std::shared_ptr clone() const override { + return std::make_shared(*this); + } +}; + + + +/** + * @brief Exception that ocurred inside the running script + * + * This is something that went wrong inside the running script. + * + */ +class ScriptRuntimeException : public ScriptException { + public: + + ScriptRuntimeException(QString message = "", QString info = "", int line = 0, int column = 0, QStringList backtraceList = QStringList()) : + ScriptException(message, info, line, column, backtraceList) { + + } + + + /** + * @brief The actual value that was thrown by the script. + * + * The content is completely arbitrary. + * + */ + ScriptValue thrownValue; + + /** + * @brief Clones this object. + * + * This is used in the scripting engine to ensure that while it can return different + * exception objects depending on what happened, the returned object is a copy that + * doesn't allow the caller to accidentally break the ScriptEngine's internal state. + * + * @return std::shared_ptr + */ + virtual std::shared_ptr clone() const override { + return std::make_shared(*this); + } }; inline QDebug operator<<(QDebug debug, const ScriptException& e) { @@ -76,5 +162,20 @@ inline QDebug operator<<(QDebug debug, const ScriptException& e) { debug << e.backtrace; } + return debug; +} + +// Is this a bad practice? +inline QDebug operator<<(QDebug debug, std::shared_ptr e) { + debug << "Exception:" + << e->errorMessage + << (e->additionalInfo.isEmpty() ? QString("") : "[" + e->additionalInfo + "]") + << " at line " << e->errorLine << ", column " << e->errorColumn; + + if (e->backtrace.length()) { + debug << "Backtrace:"; + debug << e->backtrace; + } + return debug; } \ No newline at end of file diff --git a/libraries/script-engine/src/ScriptManager.cpp b/libraries/script-engine/src/ScriptManager.cpp index 46c3cacdf2..81b0e455fb 100644 --- a/libraries/script-engine/src/ScriptManager.cpp +++ b/libraries/script-engine/src/ScriptManager.cpp @@ -282,6 +282,11 @@ ScriptManager::ScriptManager(Context context, const QString& scriptContents, con processLevelMaxRetries = 1; } + +#if 0 + // V8TODO: Is this actually needed? Exceptions will already be logged on the + // script engine side. + // this is where all unhandled exceptions end up getting logged connect(this, &ScriptManager::unhandledException, this, [this](const ScriptValue& err) { qCCritical(scriptengine) << "Caught exception"; @@ -296,6 +301,7 @@ ScriptManager::ScriptManager(Context context, const QString& scriptContents, con logException(output); }); +#endif if (_type == Type::ENTITY_CLIENT || _type == Type::ENTITY_SERVER) { QObject::connect(this, &ScriptManager::update, this, [this]() { @@ -891,13 +897,11 @@ void ScriptManager::run() { { PROFILE_RANGE(script, _fileNameString); _returnValue = _engine->evaluate(_scriptContents, _fileNameString); - _engine->maybeEmitUncaughtException(__FUNCTION__); if (_engine->hasUncaughtException()) { qCWarning(scriptengine) << "Engine has uncaught exception, stopping"; stop(); - emit unhandledException(_engine->cloneUncaughtException(__FUNCTION__)); _engine->clearExceptions(); } } @@ -1023,7 +1027,7 @@ void ScriptManager::run() { if (!_engine->isEvaluating() && _engine->hasUncaughtException()) { qCWarning(scriptengine) << __FUNCTION__ << "---------- UNCAUGHT EXCEPTION --------"; qCWarning(scriptengine) << "runInThread" << _engine->uncaughtException(); - emit unhandledException(_engine->cloneUncaughtException(__FUNCTION__)); + emit unhandledException(_engine->uncaughtException()); _engine->clearExceptions(); } } @@ -1258,9 +1262,8 @@ QString ScriptManager::_requireResolve(const QString& moduleId, const QString& r } auto message = QString("Cannot find module '%1' (%2)").arg(displayId); - auto throwResolveError = [&](const ScriptValue& error) -> QString { - _engine->raiseException(error); - _engine->maybeEmitUncaughtException("require.resolve"); + auto throwResolveError = [&](const QString& error, const QString &details = QString()) -> QString { + _engine->raiseException(error, "require.resolve: " + details); return QString(); }; @@ -1269,7 +1272,7 @@ QString ScriptManager::_requireResolve(const QString& moduleId, const QString& r if (idLength < 1 || idLength > MAX_MODULE_ID_LENGTH) { auto details = QString("rejecting invalid module id size (%1 chars [1,%2])") .arg(idLength).arg(MAX_MODULE_ID_LENGTH); - return throwResolveError(_engine->makeError(_engine->newValue(message.arg(details)), "RangeError")); + return throwResolveError(details, "RangeError"); } // this regex matches: absolute, dotted or path-like URLs @@ -1296,15 +1299,15 @@ QString ScriptManager::_requireResolve(const QString& moduleId, const QString& r if (QFileInfo(unanchoredUrl.toLocalFile()).isFile()) { auto msg = QString("relative module ids must be anchored; use './%1' instead") .arg(moduleId); - return throwResolveError(_engine->makeError(_engine->newValue(message.arg(msg)))); + return throwResolveError(message.arg(msg)); } } - return throwResolveError(_engine->makeError(_engine->newValue(message.arg("system module not found")))); + return throwResolveError(message.arg("system module not found")); } } if (url.isRelative()) { - return throwResolveError(_engine->makeError(_engine->newValue(message.arg("could not resolve module id")))); + return throwResolveError(message.arg("could not resolve module id")); } // if it looks like a local file, verify that it's an allowed path and really a file @@ -1317,22 +1320,21 @@ QString ScriptManager::_requireResolve(const QString& moduleId, const QString& r bool disallowOutsideFiles = !PathUtils::defaultScriptsLocation().isParentOf(canonical) && !currentSandboxURL.isLocalFile(); if (disallowOutsideFiles && !PathUtils::isDescendantOf(canonical, currentSandboxURL)) { - return throwResolveError(_engine->makeError(_engine->newValue(message.arg( + return throwResolveError(message.arg( QString("path '%1' outside of origin script '%2' '%3'") .arg(PathUtils::stripFilename(url)) .arg(PathUtils::stripFilename(currentSandboxURL)) .arg(canonical.toString()) - )))); + )); } if (!file.exists()) { - return throwResolveError(_engine->makeError(_engine->newValue(message.arg("path does not exist: " + url.toLocalFile())))); + return throwResolveError(message.arg("path does not exist: " + url.toLocalFile())); } if (!file.isFile()) { - return throwResolveError(_engine->makeError(_engine->newValue(message.arg("path is not a file: " + url.toLocalFile())))); + return throwResolveError(message.arg("path is not a file: " + url.toLocalFile())); } } - _engine->maybeEmitUncaughtException(__FUNCTION__); return url.toString(); } @@ -1488,7 +1490,7 @@ ScriptValue ScriptManager::instantiateModule(const ScriptValue& module, const QS //_engine->scriptValueDebugDetails(module); result = _engine->evaluateInClosure(closure, _engine->newProgram( sourceCode, modulePath )); } - _engine->maybeEmitUncaughtException(__FUNCTION__); + return result; } @@ -1510,9 +1512,8 @@ ScriptValue ScriptManager::require(const QString& moduleId) { #ifdef DEBUG_JS_MODULES qCWarning(scriptengine_module) << "throwing module error:" << error.toString() << modulePath << error.property("stack").toString(); #endif - _engine->raiseException(error); + _engine->raiseException(error, "module error"); } - _engine->maybeEmitUncaughtException("module"); return _engine->nullValue(); }; @@ -1520,7 +1521,6 @@ ScriptValue ScriptManager::require(const QString& moduleId) { QString modulePath = _requireResolve(moduleId); if (modulePath.isNull() || _engine->hasUncaughtException()) { // the resolver already threw an exception -- bail early - _engine->maybeEmitUncaughtException(__FUNCTION__); return _engine->nullValue(); } @@ -1546,7 +1546,6 @@ ScriptValue ScriptManager::require(const QString& moduleId) { qCDebug(scriptengine_module) << QString("require - using cached module for '%1' (loaded: %2)") .arg(moduleId).arg(module.property("loaded").toString()); registerModuleWithParent(module, parent); - _engine->maybeEmitUncaughtException("cached module"); return exports; } @@ -1594,7 +1593,6 @@ ScriptValue ScriptManager::require(const QString& moduleId) { //qCDebug(scriptengine_module) << "//ScriptManager::require(" << moduleId << ")"; - _engine->maybeEmitUncaughtException(__FUNCTION__); //qCDebug(scriptengine_module) << "Exports: " << _engine->scriptValueDebugDetails(module.property("exports")); return module.property("exports"); } @@ -1670,7 +1668,9 @@ void ScriptManager::include(const QStringList& includeFiles, const ScriptValue& doWithEnvironment(capturedEntityIdentifier, capturedSandboxURL, operation); if(_engine->hasUncaughtException()) { - emit unhandledException(_engine->cloneUncaughtException("evaluateInclude")); + auto ex = _engine->uncaughtException(); + ex->additionalInfo += "; evaluateInClosure"; + emit unhandledException(ex); _engine->clearExceptions(); } } else { @@ -1995,11 +1995,14 @@ void ScriptManager::entityScriptContentAvailable(const EntityItemID& entityID, c //syntaxError.setProperty("detail", entityID.toString()); //V8TODO //emit unhandledException(syntaxError); + return; } if (!program) { setError("Bad program (isNull)", EntityScriptStatus::ERROR_RUNNING_SCRIPT); - emit unhandledException(_engine->makeError(_engine->newValue("program.isNull"))); + std::shared_ptr ex = std::make_shared("Program is Null", "Bad program in entityScriptContentAvailable"); + emit unhandledException(ex); + return; // done processing script } @@ -2163,7 +2166,10 @@ void ScriptManager::entityScriptContentAvailable(const EntityItemID& entityID, c entityScriptObject = entityScriptConstructor.construct(); if (_engine->hasUncaughtException()) { - entityScriptObject = _engine->cloneUncaughtException("(construct " + entityID.toString() + ")"); + // V8TODO: Why were we copying the uncaught exception here? Does anything + // actually make use of that? + // entityScriptObject = _engine->cloneUncaughtException("(construct " + entityID.toString() + ")"); + entityScriptObject = _engine->nullValue(); _engine->clearExceptions(); } }; @@ -2171,9 +2177,10 @@ void ScriptManager::entityScriptContentAvailable(const EntityItemID& entityID, c doWithEnvironment(entityID, sandboxURL, initialization); if (entityScriptObject.isError()) { - auto exception = entityScriptObject; - setError(formatException(exception, _enableExtendedJSExceptions.get()), EntityScriptStatus::ERROR_RUNNING_SCRIPT); - emit unhandledException(exception); + // auto exception = entityScriptObject; + // setError(formatException(exception, _enableExtendedJSExceptions.get()), EntityScriptStatus::ERROR_RUNNING_SCRIPT); + // emit unhandledException(exception); + // V8TODO: Is this needed? Wouldn't the ScriptManager have already emitted the exception? return; } @@ -2340,7 +2347,6 @@ void ScriptManager::doWithEnvironment(const EntityItemID& entityID, const QUrl& #else operation(); #endif - _engine->maybeEmitUncaughtException(!entityID.isNull() ? entityID.toString() : __FUNCTION__); currentEntityIdentifier = oldIdentifier; currentSandboxURL = oldSandboxURL; } diff --git a/libraries/script-engine/src/ScriptManager.h b/libraries/script-engine/src/ScriptManager.h index 60cdc0a670..514a182d20 100644 --- a/libraries/script-engine/src/ScriptManager.h +++ b/libraries/script-engine/src/ScriptManager.h @@ -46,6 +46,7 @@ #include "Quat.h" #include "ScriptUUID.h" #include "ScriptValue.h" +#include "ScriptException.h" #include "Vec3.h" static const QString NO_SCRIPT(""); @@ -1326,7 +1327,7 @@ signals: * * @param exception */ - void unhandledException(const ScriptValue& exception); + void unhandledException(std::shared_ptr exception); ///@} diff --git a/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp b/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp index f8a475e201..31b536b197 100644 --- a/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp +++ b/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp @@ -30,5 +30,12 @@ connect(_manager, &ScriptManager::doneRunning, this, &ScriptManagerScriptingInterface::doneRunning); connect(_manager, &ScriptManager::entityScriptDetailsUpdated, this, &ScriptManagerScriptingInterface::entityScriptDetailsUpdated); connect(_manager, &ScriptManager::entityScriptPreloadFinished, this, &ScriptManagerScriptingInterface::entityScriptPreloadFinished); - connect(_manager, &ScriptManager::unhandledException, this, &ScriptManagerScriptingInterface::unhandledException); + connect(_manager, &ScriptManager::unhandledException, this, &ScriptManagerScriptingInterface::scriptManagerException); + } + + void ScriptManagerScriptingInterface::scriptManagerException(std::shared_ptr exception) { + // V8TODO: What should we actually handle here? + + + // emit unhandledException(exception.thrownValue); } \ No newline at end of file diff --git a/libraries/script-engine/src/ScriptManagerScriptingInterface.h b/libraries/script-engine/src/ScriptManagerScriptingInterface.h index 9806c9ae2b..76cf1cbb1f 100644 --- a/libraries/script-engine/src/ScriptManagerScriptingInterface.h +++ b/libraries/script-engine/src/ScriptManagerScriptingInterface.h @@ -698,7 +698,11 @@ protected: Q_INVOKABLE void entityScriptContentAvailable(const EntityItemID& entityID, const QString& scriptOrURL, const QString& contents, bool isURL, bool success, const QString& status) { _manager->entityScriptContentAvailable(entityID, scriptOrURL, contents, isURL, success, status); } +private slots: + void scriptManagerException(std::shared_ptr exception); + private: + ScriptManager *_manager; }; \ No newline at end of file diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.cpp b/libraries/script-engine/src/v8/ScriptEngineV8.cpp index d9cdf09104..229524973e 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8.cpp +++ b/libraries/script-engine/src/v8/ScriptEngineV8.cpp @@ -71,20 +71,17 @@ bool ScriptEngineV8::IS_THREADSAFE_INVOCATION(const QThread* thread, const QStri return false; } -// engine-aware JS Error copier and factory -V8ScriptValue ScriptEngineV8::makeError(const V8ScriptValue& _other, const QString& type) { +ScriptValue ScriptEngineV8::makeError(const ScriptValue& _other, const QString& type) { if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { - v8::Locker locker(_v8Isolate); - v8::Isolate::Scope isolateScope(_v8Isolate); - v8::HandleScope handleScope(_v8Isolate); - v8::Context::Scope contextScope(getContext()); - return V8ScriptValue(this, v8::Null(_v8Isolate)); + return nullValue(); } v8::Locker locker(_v8Isolate); v8::Isolate::Scope isolateScope(_v8Isolate); v8::HandleScope handleScope(_v8Isolate); v8::Context::Scope contextScope(getContext()); - return V8ScriptValue(this, v8::Null(_v8Isolate)); + return nullValue(); +} + //V8TODO /* auto other = _other; @@ -117,33 +114,8 @@ V8ScriptValue ScriptEngineV8::makeError(const V8ScriptValue& _other, const QStri err.setProperty(it.name(), it.value()); } return err;*/ -} +//} -ScriptValue ScriptEngineV8::makeError(const ScriptValue& _other, const QString& type) { - if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { - return nullValue(); - } - v8::Locker locker(_v8Isolate); - v8::Isolate::Scope isolateScope(_v8Isolate); - v8::HandleScope handleScope(_v8Isolate); - v8::Context::Scope contextScope(getContext()); - return nullValue(); - - //V8TODO - //what does makeError actually do? - /*ScriptValueV8Wrapper* unwrapped = ScriptValueV8Wrapper::unwrap(_other); - V8ScriptValue other; - if (_other.isString()) { - other = QScriptEngine::newObject(); - other.setProperty("message", _other.toString()); - } else if (unwrapped) { - other = unwrapped->toV8Value(); - } else { - other = QScriptEngine::newVariant(_other.toVariant()); - } - V8ScriptValue result = makeError(other, type); - return ScriptValue(new ScriptValueV8Wrapper(this, std::move(result)));*/ -} // check syntax and when there are issues returns an actual "SyntaxError" with the details ScriptValue ScriptEngineV8::checkScriptSyntax(ScriptProgramPointer program) { @@ -200,82 +172,14 @@ ScriptValue ScriptEngineV8::checkScriptSyntax(ScriptProgramPointer program) { return undefinedValue(); }*/ -// this pulls from the best available information to create a detailed snapshot of the current exception -ScriptValue ScriptEngineV8::cloneUncaughtException(const QString& extraDetail) { - if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { - return nullValue(); - } - if (!hasUncaughtException()) { - return nullValue(); - } - v8::Locker locker(_v8Isolate); - v8::Isolate::Scope isolateScope(_v8Isolate); - v8::HandleScope handleScope(_v8Isolate); - v8::Context::Scope contextScope(getContext()); - return nullValue(); - //V8TODO - /* - auto exception = uncaughtException(); - // ensure the error object is engine-local - auto err = makeError(exception); - - // not sure why Qt does't offer uncaughtExceptionFileName -- but the line number - // on its own is often useless/wrong if arbitrarily married to a filename. - // when the error object already has this info, it seems to be the most reliable - auto fileName = exception.property("fileName").toString(); - auto lineNumber = exception.property("lineNumber").toInt32(); - - // the backtrace, on the other hand, seems most reliable taken from uncaughtExceptionBacktrace - auto backtrace = uncaughtExceptionBacktrace(); - if (backtrace.isEmpty()) { - // fallback to the error object - backtrace = exception.property("stack").toString().split(ScriptManager::SCRIPT_BACKTRACE_SEP); - } - // the ad hoc "detail" property can be used now to embed additional clues - auto detail = exception.property("detail").toString(); - if (detail.isEmpty()) { - detail = extraDetail; - } else if (!extraDetail.isEmpty()) { - detail += "(" + extraDetail + ")"; - } - if (lineNumber <= 0) { - lineNumber = uncaughtExceptionLineNumber(); - } - if (fileName.isEmpty()) { - // climb the stack frames looking for something useful to display - for (auto c = QScriptEngine::currentContext(); c && fileName.isEmpty(); c = c->parentContext()) { - V8ScriptContextInfo info{ c }; - if (!info.fileName().isEmpty()) { - // take fileName:lineNumber as a pair - fileName = info.fileName(); - lineNumber = info.lineNumber(); - if (backtrace.isEmpty()) { - backtrace = c->backtrace(); - } - break; - } - } - } - err.setProperty("fileName", fileNlintame); - err.setProperty("lineNumber", lineNumber); - err.setProperty("detail", detail); - err.setProperty("stack", backtrace.join(ScriptManager::SCRIPT_BACKTRACE_SEP)); - -#ifdef DEBUG_JS_EXCEPTIONS - err.setProperty("_fileName", exception.property("fileName").toString()); - err.setProperty("_stack", uncaughtExceptionBacktrace().join(SCRIPT_BACKTRACE_SEP)); - err.setProperty("_lineNumber", uncaughtExceptionLineNumber()); -#endif - return err; - */ -} - bool ScriptEngineV8::raiseException(const V8ScriptValue& exception) { if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { return false; } //V8TODO - _v8Isolate->ThrowException(makeError(exception).get()); + // _v8Isolate->ThrowException(makeError(exception).get()); + + /*if (QScriptEngine::currentContext()) { // we have an active context / JS stack frame so throw the exception per usual QScriptEngine::currentContext()->throwValue(makeError(exception)); @@ -290,22 +194,6 @@ bool ScriptEngineV8::raiseException(const V8ScriptValue& exception) { return false; } -bool ScriptEngineV8::maybeEmitUncaughtException(const QString& debugHint) { - - /* - if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { - return false; - } - if (!isEvaluating() && hasUncaughtException()) { - emit exception(cloneUncaughtException(debugHint)); - clearExceptions(); - return true; - } - - */ - return false; -} - // Lambda /*ScriptValue ScriptEngineV8::newLambdaFunction(std::function operation, const V8ScriptValue& data, @@ -1019,12 +907,11 @@ ScriptValue ScriptEngineV8::evaluateInClosure(const ScriptValue& _closure, } if (hasUncaughtException()) { - auto err = cloneUncaughtException(__FUNCTION__); #ifdef DEBUG_JS_EXCEPTIONS qCWarning(shared) << __FUNCTION__ << "---------- hasCaught:" << err.toString() << result.toString(); err.setProperty("_result", result); #endif - result = err; + result = nullValue(); } else { result = ScriptValue(new ScriptValueV8Wrapper(this, V8ScriptValue(this, v8Result))); } @@ -1073,34 +960,9 @@ ScriptValue ScriptEngineV8::evaluate(const QString& sourceCode, const QString& f v8::ScriptOrigin scriptOrigin(getIsolate(), v8::String::NewFromUtf8(getIsolate(), fileName.toStdString().c_str()).ToLocalChecked()); v8::Local script; if (!v8::Script::Compile(getContext(), v8::String::NewFromUtf8(getIsolate(), sourceCode.toStdString().c_str()).ToLocalChecked(), &scriptOrigin).ToLocal(&script)) { - //V8TODO replace this with external function - int errorColumnNumber = 0; - int errorLineNumber = 0; - QString errorMessage = ""; - QString errorBacktrace = ""; - //v8::String::Utf8Value utf8Value(getIsolate(), tryCatch.Exception()); - v8::String::Utf8Value utf8Value(getIsolate(), tryCatch.Message()->Get()); - errorMessage = QString(*utf8Value); - v8::Local exceptionMessage = tryCatch.Message(); - if (!exceptionMessage.IsEmpty()) { - errorLineNumber = exceptionMessage->GetLineNumber(getContext()).FromJust(); - errorColumnNumber = exceptionMessage->GetStartColumn(getContext()).FromJust(); - v8::Local backtraceV8String; - if (tryCatch.StackTrace(getContext()).ToLocal(&backtraceV8String)) { - if (backtraceV8String->IsString()) { - if (v8::Local::Cast(backtraceV8String)->Length() > 0) { - v8::String::Utf8Value backtraceUtf8Value(getIsolate(), backtraceV8String); - errorBacktrace = *backtraceUtf8Value; - } - } - } - qCDebug(scriptengine) << "Compiling script \"" << fileName << "\" failed on line " << errorLineNumber << " column " << errorColumnNumber << " with message: \"" << errorMessage <<"\" backtrace: " << errorBacktrace; - } - auto err = makeError(newValue(errorMessage)); - raiseException(err); - maybeEmitUncaughtException("compile"); + setUncaughtException(tryCatch, "Error while compiling script"); _evaluatingCounter--; - return err; + return nullValue(); } //qCDebug(scriptengine) << "Script compilation succesful: " << fileName; @@ -1145,6 +1007,12 @@ ScriptValue ScriptEngineV8::evaluate(const QString& sourceCode, const QString& f return ScriptValue(new ScriptValueV8Wrapper(this, std::move(resultValue))); } + +void ScriptEngineV8::setUncaughtEngineException(const QString &reason, const QString& info) { + auto ex = std::make_shared(reason, info); + setUncaughtException(ex); +} + void ScriptEngineV8::setUncaughtException(const v8::TryCatch &tryCatch, const QString& info) { if (!tryCatch.HasCaught()) { qCWarning(scriptengine) << "setUncaughtException called without exception"; @@ -1152,8 +1020,8 @@ void ScriptEngineV8::setUncaughtException(const v8::TryCatch &tryCatch, const QS return; } - ScriptException ex; - ex.additionalInfo = info; + auto ex = std::make_shared(); + ex->additionalInfo = info; v8::Locker locker(_v8Isolate); v8::Isolate::Scope isolateScope(_v8Isolate); @@ -1166,30 +1034,35 @@ void ScriptEngineV8::setUncaughtException(const v8::TryCatch &tryCatch, const QS //v8::String::Utf8Value utf8Value(getIsolate(), tryCatch.Exception()); v8::String::Utf8Value utf8Value(getIsolate(), tryCatch.Message()->Get()); - ex.errorMessage = QString(*utf8Value); + ex->errorMessage = QString(*utf8Value); v8::Local exceptionMessage = tryCatch.Message(); if (!exceptionMessage.IsEmpty()) { - ex.errorLine = exceptionMessage->GetLineNumber(getContext()).FromJust(); - ex.errorColumn = exceptionMessage->GetStartColumn(getContext()).FromJust(); + ex->errorLine = exceptionMessage->GetLineNumber(getContext()).FromJust(); + ex->errorColumn = exceptionMessage->GetStartColumn(getContext()).FromJust(); v8::Local backtraceV8String; if (tryCatch.StackTrace(getContext()).ToLocal(&backtraceV8String)) { if (backtraceV8String->IsString()) { if (v8::Local::Cast(backtraceV8String)->Length() > 0) { v8::String::Utf8Value backtraceUtf8Value(getIsolate(), backtraceV8String); QString errorBacktrace = *backtraceUtf8Value; - ex.backtrace = errorBacktrace.split("\n"); + ex->backtrace = errorBacktrace.split("\n"); } } } } - qCDebug(scriptengine) << "Emitting exception:" << ex; - _uncaughtException = ex; - emit exception(ex); + setUncaughtException(ex); } +void ScriptEngineV8::setUncaughtException(std::shared_ptr uncaughtException) { + qCDebug(scriptengine) << "Emitting exception:" << uncaughtException; + _uncaughtException = uncaughtException; + + auto copy = uncaughtException->clone(); + emit exception(copy); +} QString ScriptEngineV8::formatErrorMessageFromTryCatch(v8::TryCatch &tryCatch) { @@ -1274,18 +1147,14 @@ Q_INVOKABLE ScriptValue ScriptEngineV8::evaluate(const ScriptProgramPointer& pro v8::Context::Scope contextScope(getContext()); ScriptProgramV8Wrapper* unwrapped = ScriptProgramV8Wrapper::unwrap(program); if (!unwrapped) { - errorValue = makeError(newValue("could not unwrap program")); - raiseException(errorValue); - maybeEmitUncaughtException("compile"); + setUncaughtEngineException("Could not unwrap program", "Compile error"); hasFailed = true; } if(!hasFailed) { ScriptSyntaxCheckResultPointer syntaxCheck = unwrapped->checkSyntax(); if (syntaxCheck->state() == ScriptSyntaxCheckResult::Error) { - errorValue = makeError(newValue(syntaxCheck->errorMessage())); - raiseException(errorValue); - maybeEmitUncaughtException("compile"); + setUncaughtEngineException(syntaxCheck->errorMessage(), "Compile error"); hasFailed = true; } } @@ -1307,8 +1176,7 @@ Q_INVOKABLE ScriptValue ScriptEngineV8::evaluate(const ScriptProgramPointer& pro Q_ASSERT(tryCatchRun.HasCaught()); auto runError = tryCatchRun.Message(); errorValue = ScriptValue(new ScriptValueV8Wrapper(this, V8ScriptValue(this, runError->Get()))); - raiseException(errorValue); - maybeEmitUncaughtException("evaluate"); + raiseException(errorValue, "evaluation error"); hasFailed = true; } else { // V8TODO this is just to check if run will always return false for uncaught exception @@ -1529,7 +1397,7 @@ void ScriptEngineV8::abortEvaluation() { } void ScriptEngineV8::clearExceptions() { - _uncaughtException = ScriptException(); + _uncaughtException.reset(); } ScriptContext* ScriptEngineV8::currentContext() const { @@ -1551,7 +1419,7 @@ ScriptContext* ScriptEngineV8::currentContext() const { } bool ScriptEngineV8::hasUncaughtException() const { - return !_uncaughtException.isEmpty(); + return _uncaughtException != nullptr; } bool ScriptEngineV8::isEvaluating() const { @@ -1667,11 +1535,15 @@ void ScriptEngineV8::setThread(QThread* thread) { }*/ -ScriptException ScriptEngineV8::uncaughtException() const { - return _uncaughtException; +std::shared_ptr ScriptEngineV8::uncaughtException() const { + return _uncaughtException->clone(); } -bool ScriptEngineV8::raiseException(const ScriptValue& exception) { +bool ScriptEngineV8::raiseException(const QString& error, const QString &reason) { + return raiseException(ScriptValue(), reason); +} + +bool ScriptEngineV8::raiseException(const ScriptValue& exception, const QString &reason) { //V8TODO //Q_ASSERT(false); // qCritical() << "Script exception occurred: " << exception.toString(); @@ -1681,6 +1553,7 @@ bool ScriptEngineV8::raiseException(const ScriptValue& exception) { // emit //return raiseException(qException); + qCCritical(scriptengine) << "Raise exception for reason" << reason << "NOT IMPLEMENTED!"; return false; } diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.h b/libraries/script-engine/src/v8/ScriptEngineV8.h index b02a73a552..7387df80b4 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8.h +++ b/libraries/script-engine/src/v8/ScriptEngineV8.h @@ -71,7 +71,6 @@ public: // construction public: // ScriptEngine implementation virtual void abortEvaluation() override; virtual void clearExceptions() override; - virtual ScriptValue cloneUncaughtException(const QString& detail = QString()) override; virtual ScriptContext* currentContext() const override; Q_INVOKABLE virtual ScriptValue evaluate(const QString& program, const QString& fileName = QString()) override; Q_INVOKABLE virtual ScriptValue evaluate(const ScriptProgramPointer& program) override; @@ -81,11 +80,6 @@ public: // ScriptEngine implementation virtual bool isEvaluating() const override; //virtual ScriptValue lintScript(const QString& sourceCode, const QString& fileName, const int lineNumber = 1) override; virtual ScriptValue checkScriptSyntax(ScriptProgramPointer program) override; - virtual ScriptValue makeError(const ScriptValue& other, const QString& type = "Error") override; - - - // if there is a pending exception and we are at the top level (non-recursive) stack frame, this emits and resets it - virtual bool maybeEmitUncaughtException(const QString& debugHint = QString()) override; virtual ScriptValue newArray(uint length = 0) override; virtual ScriptValue newArrayBuffer(const QByteArray& message) override; @@ -106,7 +100,11 @@ public: // ScriptEngine implementation virtual ScriptValue newValue(const char* value) override; virtual ScriptValue newVariant(const QVariant& value) override; virtual ScriptValue nullValue() override; - virtual bool raiseException(const ScriptValue& exception) override; + + virtual ScriptValue makeError(const ScriptValue& other, const QString& type = "Error") override; + + virtual bool raiseException(const QString& exception, const QString &reason = QString()) override; + virtual bool raiseException(const ScriptValue& exception, const QString &reason = QString()) override; Q_INVOKABLE virtual void registerEnum(const QString& enumName, QMetaEnum newEnum) override; Q_INVOKABLE virtual void registerFunction(const QString& name, ScriptEngine::FunctionSignature fun, @@ -128,7 +126,7 @@ public: // ScriptEngine implementation virtual void setThread(QThread* thread) override; //Q_INVOKABLE virtual void enterIsolateOnThisThread() override; virtual ScriptValue undefinedValue() override; - virtual ScriptException uncaughtException() const override; + virtual std::shared_ptr uncaughtException() const override; virtual void updateMemoryCost(const qint64& deltaSize) override; virtual void requestCollectGarbage() override { while(!_v8Isolate->IdleNotificationDeadline(getV8Platform()->MonotonicallyIncreasingTime() + GARBAGE_COLLECTION_TIME_LIMIT_S)) {}; } virtual void compileTest() override; @@ -142,7 +140,7 @@ public: // ScriptEngine implementation inline bool IS_THREADSAFE_INVOCATION(const QString& method) { return ScriptEngine::IS_THREADSAFE_INVOCATION(method); } protected: // brought over from BaseScriptEngine - V8ScriptValue makeError(const V8ScriptValue& other, const QString& type = "Error"); + // if the currentContext() is valid then throw the passed exception; otherwise, immediately emit it. // note: this is used in cases where C++ code might call into JS API methods directly @@ -223,16 +221,18 @@ signals: * * @param exception Exception that was thrown */ - void exception(const ScriptException &exception); + void exception(std::shared_ptr exception); protected: static QMutex _v8InitMutex; static std::once_flag _v8InitOnceFlag; static v8::Platform* getV8Platform(); + void setUncaughtEngineException(const QString &message, const QString& info = QString()); void setUncaughtException(const v8::TryCatch &tryCatch, const QString& info = QString()); + void setUncaughtException(std::shared_ptr exception); - ScriptException _uncaughtException; + std::shared_ptr _uncaughtException; // V8TODO: clean up isolate when script engine is destroyed? diff --git a/tests/script-engine/src/tests/010_exception.js b/tests/script-engine/src/tests/010_exception.js new file mode 100644 index 0000000000..a280f9a5cc --- /dev/null +++ b/tests/script-engine/src/tests/010_exception.js @@ -0,0 +1 @@ +foo();