From f79ec0f293eb1200a849d0b436d2285a6e23c39d Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Tue, 9 May 2023 19:44:37 +0200 Subject: [PATCH] Work on script engine exceptions, cleanup --- .../entities/src/EntityScriptingInterface.cpp | 3 +- .../src/BaseScriptEngine.h | 102 ------------------ libraries/script-engine/src/ScriptManager.cpp | 2 +- .../script-engine/src/v8/ScriptEngineV8.cpp | 42 ++++---- 4 files changed, 25 insertions(+), 124 deletions(-) delete mode 100644 libraries/script-engine-qtscript/src/BaseScriptEngine.h diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index db2c92b344..2cca95b16f 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -1574,7 +1574,8 @@ bool EntityPropertyMetadataRequest::script(EntityItemID entityID, const ScriptVa if (!request->isStarted()) { request->deleteLater(); auto engine = handler.engine(); - callScopedHandlerObject(handler, engine->makeError(engine->newValue("Entities Scripting Provider unavailable"), "InternalError"), ScriptValue()); + // It was originally "InternalError", but there's no such error data type in V8 + callScopedHandlerObject(handler, engine->makeError(engine->newValue("Entities Scripting Provider unavailable"), "Error"), ScriptValue()); return false; } return true; diff --git a/libraries/script-engine-qtscript/src/BaseScriptEngine.h b/libraries/script-engine-qtscript/src/BaseScriptEngine.h deleted file mode 100644 index adb7f82c87..0000000000 --- a/libraries/script-engine-qtscript/src/BaseScriptEngine.h +++ /dev/null @@ -1,102 +0,0 @@ -// -// BaseScriptEngine.h -// libraries/script-engine/src -// -// Created by Timothy Dedischew on 02/01/17. -// Copyright 2017 High Fidelity, Inc. -// Copyright 2023 Overte e.V. -// -// Distributed under the Apache License, Version 2.0. -// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html -// SPDX-License-Identifier: Apache-2.0 -// - -#ifndef hifi_BaseScriptEngine_h -#define hifi_BaseScriptEngine_h - -/*#include -#include -#include -#include - -class ScriptEngineQtScript; -using ScriptEngineQtScriptPointer = QSharedPointer; - -// common base class for extending QScriptEngine itself -class BaseScriptEngine : public QScriptEngine, public QEnableSharedFromThis { - Q_OBJECT -public: - // threadsafe "unbound" version of QScriptEngine::nullValue() - static const QScriptValue unboundNullValue() { return QScriptValue(0, QScriptValue::NullValue); } - - BaseScriptEngine() {} - - *@jsdoc - * @function Script.lintScript - * @param {string} sourceCode - Source code. - * @param {string} fileName - File name. - * @param {number} [lineNumber=1] - Line number. - * @returns {object} Object. - * @deprecated This function is deprecated and will be removed. - * - Q_INVOKABLE QScriptValue lintScript(const QString& sourceCode, const QString& fileName, const int lineNumber = 1); - - *@jsdoc - * @function Script.makeError - * @param {object} [other] - Other. - * @param {string} [type="Error"] - Error. - * @returns {object} Object. - * @deprecated This function is deprecated and will be removed. - * - Q_INVOKABLE QScriptValue makeError(const QScriptValue& other = QScriptValue(), const QString& type = "Error"); - - QScriptValue cloneUncaughtException(const QString& detail = QString()); - QScriptValue evaluateInClosure(const QScriptValue& locals, const QScriptProgram& program); - - // if there is a pending exception and we are at the top level (non-recursive) stack frame, this emits and resets it - bool maybeEmitUncaughtException(const QString& debugHint = QString()); - - // 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 - bool raiseException(const QScriptValue& exception); - - // helper to detect and log warnings when other code invokes QScriptEngine/BaseScriptEngine in thread-unsafe ways - static bool IS_THREADSAFE_INVOCATION(const QThread *thread, const QString& method); -signals: - *@jsdoc - * @function Script.signalHandlerException - * @param {object} exception - Exception. - * @returns {Signal} - * @deprecated This signal is deprecated and will be removed. - * - // Script.signalHandlerException is exposed by QScriptEngine. - -protected: - // like `newFunction`, but allows mapping inline C++ lambdas with captures as callable QScriptValues - // even though the context/engine parameters are redundant in most cases, the function signature matches `newFunction` - // anyway so that newLambdaFunction can be used to rapidly prototype / test utility APIs and then if becoming - // permanent more easily promoted into regular static newFunction scenarios. - QScriptValue newLambdaFunction(std::function operation, const QScriptValue& data = QScriptValue(), const QScriptEngine::ValueOwnership& ownership = QScriptEngine::AutoOwnership); - -#ifdef DEBUG_JS - static void _debugDump(const QString& header, const QScriptValue& object, const QString& footer = QString()); -#endif -}; - -// Lambda helps create callable QScriptValues out of std::functions: -// (just meant for use from within the script engine itself) -class Lambda : public QObject { - Q_OBJECT -public: - Lambda(QScriptEngine *engine, std::function operation, QScriptValue data); - ~Lambda(); - public slots: - QScriptValue call(); - QString toString() const; -private: - QScriptEngine* engine; - std::function operation; - QScriptValue data; -};*/ - -#endif // hifi_BaseScriptEngine_h diff --git a/libraries/script-engine/src/ScriptManager.cpp b/libraries/script-engine/src/ScriptManager.cpp index 69f20d0370..454c8fc7ef 100644 --- a/libraries/script-engine/src/ScriptManager.cpp +++ b/libraries/script-engine/src/ScriptManager.cpp @@ -2029,7 +2029,7 @@ void ScriptManager::entityScriptContentAvailable(const EntityItemID& entityID, c } // SANITY/PERFORMANCE CHECK USING SANDBOX - // V8TODO: can be skipped for now but needs to be implemented before release + // V8TODO: can be skipped for now should we implement it before merging V8 branch? /*const int SANDBOX_TIMEOUT = 0.25 * MSECS_PER_SECOND; ScriptEnginePointer sandbox = newScriptEngine(); diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.cpp b/libraries/script-engine/src/v8/ScriptEngineV8.cpp index 63af8f8ef6..f9744999c4 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8.cpp +++ b/libraries/script-engine/src/v8/ScriptEngineV8.cpp @@ -1212,40 +1212,42 @@ bool ScriptEngineV8::raiseException(const QString& error, const QString &reason) bool ScriptEngineV8::raiseException(const ScriptValue& exception, const QString &reason) { //V8TODO Im not sure how to finish these -// qCCritical(scriptengine_v8) << "Script exception occurred: " << exception.toString(); -// ScriptValueV8Wrapper* unwrapped = ScriptValueV8Wrapper::unwrap(exception); -// V8ScriptValue qException = unwrapped ? unwrapped->toV8Value() : QScriptEngine::newVariant(exception.toVariant()); - - // emit - //return raiseException(qException); - -// qCCritical(scriptengine_v8) << "Raise exception for reason" << reason << "NOT IMPLEMENTED!"; -// return false; + qCCritical(scriptengine_v8) << "Script exception occurred: " << exception.toString(); + ScriptValueV8Wrapper* unwrapped = ScriptValueV8Wrapper::unwrap(exception); + ScriptValue qException = unwrapped ? exception : newVariant(exception.toVariant()); return raiseException(ScriptValueV8Wrapper::fullUnwrap(this, exception)); } - - bool ScriptEngineV8::raiseException(const V8ScriptValue& exception) { if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { return false; } + v8::Locker locker(_v8Isolate); + v8::Isolate::Scope isolateScope(_v8Isolate); + v8::HandleScope handleScope(_v8Isolate); + v8::Context::Scope contextScope(getContext()); - _v8Isolate->ThrowException(exception.constGet()); - - /*if (QScriptEngine::currentContext()) { + auto trace = v8::StackTrace::CurrentStackTrace(_v8Isolate, 2); + if (trace->GetFrameCount() > 0) { // we have an active context / JS stack frame so throw the exception per usual - QScriptEngine::currentContext()->throwValue(makeError(exception)); + //currentContext()->throwValue(makeError(exception)); + auto thrown = makeError(ScriptValue(new ScriptValueV8Wrapper(this, exception))); + auto thrownV8 = ScriptValueV8Wrapper::fullUnwrap(this, thrown); + _v8Isolate->ThrowException(thrownV8.get()); return true; - } else if (_scriptManager) { + } else if (_manager) { // we are within a pure C++ stack frame (ie: being called directly by other C++ code) // in this case no context information is available so just emit the exception for reporting - V8ScriptValue thrown = makeError(exception); - emit _scriptManager->unhandledException(ScriptValue(new ScriptValueV8Wrapper(this, std::move(thrown)))); - }*/ - //emit _scriptManager->unhandledException(ScriptValue(new ScriptValueV8Wrapper(this, std::move(thrown)))); + ScriptValue thrown = makeError(ScriptValue(new ScriptValueV8Wrapper(this, exception))); + auto scriptRuntimeException = std::make_shared(); + ScriptValue message = thrown.property("stack"); //This contains more details along with the error message + scriptRuntimeException->errorMessage = message.toString(); + scriptRuntimeException->thrownValue = thrown; + emit _manager->unhandledException(scriptRuntimeException); + } + return false; }