From 0d454eb6e837ee9c63c8a00e59e037c489534b04 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Fri, 16 Dec 2022 09:20:25 +0100 Subject: [PATCH] V8 fixes, including making debug console work --- interface/src/ui/JSConsole.cpp | 17 ++-- interface/src/ui/JSConsole.h | 2 +- libraries/script-engine/src/ScriptManager.cpp | 6 +- .../src/v8/ScriptContextV8Wrapper.cpp | 2 +- .../script-engine/src/v8/ScriptEngineV8.cpp | 90 ++++++++++++------- .../src/v8/ScriptValueV8Wrapper.cpp | 6 +- 6 files changed, 77 insertions(+), 46 deletions(-) diff --git a/interface/src/ui/JSConsole.cpp b/interface/src/ui/JSConsole.cpp index 7d4fadab8e..1becd8b2e9 100644 --- a/interface/src/ui/JSConsole.cpp +++ b/interface/src/ui/JSConsole.cpp @@ -351,12 +351,12 @@ void JSConsole::executeCommand(const QString& command) { std::weak_ptr weakScriptManager = _scriptManager; auto consoleFileName = _consoleFileName; - QFuture future = QtConcurrent::run([weakScriptManager, consoleFileName, command]() -> ScriptValue { - ScriptValue result; + QFuture future = QtConcurrent::run([weakScriptManager, consoleFileName, command]() -> QVariant { + QVariant result; auto scriptManager = weakScriptManager.lock(); if (scriptManager) { BLOCKING_INVOKE_METHOD(scriptManager.get(), [&scriptManager, &consoleFileName, &command, &result]() -> void { - result = scriptManager->evaluate(command, consoleFileName); + result = scriptManager->evaluate(command, consoleFileName).toVariant(); }); } return result; @@ -365,7 +365,7 @@ void JSConsole::executeCommand(const QString& command) { } void JSConsole::commandFinished() { - ScriptValue result = _executeWatcher.result(); + QVariant result = _executeWatcher.result(); _ui->promptTextEdit->setDisabled(false); @@ -374,9 +374,12 @@ void JSConsole::commandFinished() { _ui->promptTextEdit->setFocus(); } - bool error = (_scriptManager->engine()->hasUncaughtException() || result.isError()); - QString gutter = error ? GUTTER_ERROR : GUTTER_PREVIOUS_COMMAND; - QString resultColor = error ? RESULT_ERROR_STYLE : RESULT_SUCCESS_STYLE; + // V8TODO: + //bool error = (_scriptManager->engine()->hasUncaughtException() || result.isError()); + //QString gutter = error ? GUTTER_ERROR : GUTTER_PREVIOUS_COMMAND; + //QString resultColor = error ? RESULT_ERROR_STYLE : RESULT_SUCCESS_STYLE; + QString gutter = GUTTER_PREVIOUS_COMMAND; + QString resultColor = RESULT_SUCCESS_STYLE; QString resultStr = "" + result.toString().toHtmlEscaped() + ""; appendMessage(gutter, resultStr); diff --git a/interface/src/ui/JSConsole.h b/interface/src/ui/JSConsole.h index 4bfa4cb107..58ba8a3b0f 100644 --- a/interface/src/ui/JSConsole.h +++ b/interface/src/ui/JSConsole.h @@ -72,7 +72,7 @@ private: QStandardItemModel* getAutoCompleteModel(const QString& memberOf = nullptr); - QFutureWatcher _executeWatcher; + QFutureWatcher _executeWatcher; Ui::Console* _ui; int _currentCommandInHistory; QString _savedHistoryFilename; diff --git a/libraries/script-engine/src/ScriptManager.cpp b/libraries/script-engine/src/ScriptManager.cpp index dfb38f0375..c658b7c89f 100644 --- a/libraries/script-engine/src/ScriptManager.cpp +++ b/libraries/script-engine/src/ScriptManager.cpp @@ -1968,7 +1968,9 @@ void ScriptManager::entityScriptContentAvailable(const EntityItemID& entityID, c } // SANITY/PERFORMANCE CHECK USING SANDBOX - const int SANDBOX_TIMEOUT = 0.25 * MSECS_PER_SECOND; + // V8TODO: can be skipped for now but needs to be implemented before release + + /*const int SANDBOX_TIMEOUT = 0.25 * MSECS_PER_SECOND; ScriptEnginePointer sandbox = newScriptEngine(); sandbox->setProcessEventsInterval(SANDBOX_TIMEOUT); ScriptValue testConstructor, exception; @@ -2104,7 +2106,7 @@ void ScriptManager::entityScriptContentAvailable(const EntityItemID& entityID, c setError("Could not find constructor (" + testConstructorType + ")", EntityScriptStatus::ERROR_RUNNING_SCRIPT); emit unhandledException(err); return; // done processing script - } + }*/ // (this feeds into refreshFileScript) int64_t lastModified = 0; diff --git a/libraries/script-engine/src/v8/ScriptContextV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptContextV8Wrapper.cpp index 950193166d..10691e56bb 100644 --- a/libraries/script-engine/src/v8/ScriptContextV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptContextV8Wrapper.cpp @@ -119,7 +119,7 @@ ScriptFunctionContextPointer ScriptContextV8Wrapper::functionContext() const { ScriptContextPointer ScriptContextV8Wrapper::parentContext() const { //V8TODO - Q_ASSERT(false); + //Q_ASSERT(false); //V8ScriptContext* result = _context->parentContext(); //return result ? std::make_shared(_engine, result) : ScriptContextPointer(); return ScriptContextPointer(); diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.cpp b/libraries/script-engine/src/v8/ScriptEngineV8.cpp index 6dc619871c..f174da3823 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8.cpp +++ b/libraries/script-engine/src/v8/ScriptEngineV8.cpp @@ -981,28 +981,42 @@ Q_INVOKABLE ScriptValue ScriptEngineV8::evaluate(const ScriptProgramPointer& pro } Q_ASSERT(!isEvaluating()); _isEvaluating = true; - v8::HandleScope handleScope(_v8Isolate); - v8::Context::Scope contextScope(_v8Context.Get(_v8Isolate)); - ScriptProgramV8Wrapper* unwrapped = ScriptProgramV8Wrapper::unwrap(program); - if (!unwrapped) { - auto err = makeError(newValue("could not unwrap program")); - raiseException(err); - maybeEmitUncaughtException("compile"); - _isEvaluating = false; - return err; - } - ScriptSyntaxCheckResultPointer syntaxCheck = unwrapped->checkSyntax(); - if (syntaxCheck->state() == ScriptSyntaxCheckResult::Error) { - auto err = makeError(newValue(syntaxCheck->errorMessage())); - raiseException(err); - maybeEmitUncaughtException("compile"); - _isEvaluating = false; - return err; + bool is_isolate_exit_needed = false; + if(!_v8Isolate->IsCurrent() && !_v8Locker) { + // V8TODO: Theoretically only script thread should access this, so it should be safe + _v8Locker.reset(new v8::Locker(_v8Isolate)); + _v8Isolate->Enter(); + is_isolate_exit_needed = true; } + ScriptValue errorValue; + ScriptValue resultValue; + bool hasFailed = false; + { + v8::HandleScope handleScope(_v8Isolate); + v8::Context::Scope contextScope(_v8Context.Get(_v8Isolate)); + ScriptProgramV8Wrapper* unwrapped = ScriptProgramV8Wrapper::unwrap(program); + if (!unwrapped) { + errorValue = makeError(newValue("could not unwrap program")); + raiseException(errorValue); + maybeEmitUncaughtException("compile"); + hasFailed = true; + } - const V8ScriptProgram& v8Program = unwrapped->toV8Value(); - // V8TODO - /*if (qProgram.isNull()) { + if(!hasFailed) { + ScriptSyntaxCheckResultPointer syntaxCheck = unwrapped->checkSyntax(); + if (syntaxCheck->state() == ScriptSyntaxCheckResult::Error) { + errorValue = makeError(newValue(syntaxCheck->errorMessage())); + raiseException(errorValue); + maybeEmitUncaughtException("compile"); + hasFailed = true; + } + } + + v8::Local result; + if(!hasFailed) { + const V8ScriptProgram& v8Program = unwrapped->toV8Value(); + // V8TODO + /*if (qProgram.isNull()) { // can this happen? auto err = makeError(newValue("requested program is empty")); raiseException(err); @@ -1010,20 +1024,31 @@ Q_INVOKABLE ScriptValue ScriptEngineV8::evaluate(const ScriptProgramPointer& pro return err; }*/ - v8::Local result; - v8::TryCatch tryCatchRun(getIsolate()); - if (!v8Program.constGet()->Run(getContext()).ToLocal(&result)) { - Q_ASSERT(tryCatchRun.HasCaught()); - auto runError = tryCatchRun.Message(); - ScriptValue errorValue(new ScriptValueV8Wrapper(this, V8ScriptValue(_v8Isolate, runError->Get()))); - raiseException(errorValue); - maybeEmitUncaughtException("evaluate"); - _isEvaluating = false; - return errorValue; + v8::TryCatch tryCatchRun(getIsolate()); + if (!v8Program.constGet()->Run(getContext()).ToLocal(&result)) { + Q_ASSERT(tryCatchRun.HasCaught()); + auto runError = tryCatchRun.Message(); + errorValue = ScriptValue(new ScriptValueV8Wrapper(this, V8ScriptValue(_v8Isolate, runError->Get()))); + raiseException(errorValue); + maybeEmitUncaughtException("evaluate"); + hasFailed = true; + } + } + if(!hasFailed) { + V8ScriptValue resultValueV8(_v8Isolate, result); + resultValue = ScriptValue(new ScriptValueV8Wrapper(this, std::move(resultValueV8))); + } } - V8ScriptValue resultValue(_v8Isolate, result); _isEvaluating = false; - return ScriptValue(new ScriptValueV8Wrapper(this, std::move(resultValue))); + if (is_isolate_exit_needed) { + _v8Isolate->Exit(); + _v8Locker.reset(nullptr); + } + if (hasFailed) { + return errorValue; + } else { + return resultValue; + } } @@ -1094,6 +1119,7 @@ ScriptProgramPointer ScriptEngineV8::newProgram(const QString& sourceCode, const //V8TODO: is it used between isolates? //V8TODO: should it be compiled on creation? //V8ScriptProgram result(sourceCode, fileName); + v8::HandleScope handleScope(_v8Isolate); v8::Context::Scope contextScope(_v8Context.Get(_v8Isolate)); return std::make_shared(this, sourceCode, fileName); diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp index f59f5b4657..c4a2600540 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp @@ -429,9 +429,9 @@ bool ScriptValueV8Wrapper::isBool() const { bool ScriptValueV8Wrapper::isError() const { auto isolate = _engine->getIsolate(); - Q_ASSERT(isolate->IsCurrent()); - v8::HandleScope handleScope(isolate); - v8::Context::Scope contextScope(_engine->getContext()); + // Q_ASSERT(isolate->IsCurrent()); + // v8::HandleScope handleScope(isolate); + // v8::Context::Scope contextScope(_engine->getContext()); //V8TODO return false; //return _value.constGet()->IsError();