From cca63ea360a2b8430bb529ad94c3c1d24ea54f49 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Fri, 5 Apr 2024 09:46:16 -0700 Subject: [PATCH 01/10] Update UrlLib to latest to pick up error handling fixes --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d08b872a..544706c4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -30,7 +30,7 @@ FetchContent_Declare(llhttp URL "https://github.com/nodejs/llhttp/archive/refs/tags/release/v8.1.0.tar.gz") FetchContent_Declare(UrlLib GIT_REPOSITORY https://github.com/BabylonJS/UrlLib.git - GIT_TAG 59917f32f6ddfa26af07dd981842c51ce02dafcd) + GIT_TAG c7c162be129e170a12bb8766c727b59c42853826) # -------------------------------------------------- FetchContent_MakeAvailable(CMakeExtensions) From 7a0dca83d175172c11a66c4df8abc13414a6530d Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Fri, 5 Apr 2024 16:45:06 -0700 Subject: [PATCH 02/10] A bunch of miscellaneous fixes - Move Node-API extensions from Babylon Native into JsRuntimeHost. - Add helper method to Node-API extensions to get error string from either callstack or message if callstack is not available. Use this method where appropriate (e.g., DefaultUnhandledExceptionHandler) to avoid crash when callstack is not available. - Add EnableDebugger flag to AppRuntime to allow user configuration. - Add Node-API defines required for JsRuntimeHost directly into the headers instead in CMake to avoid having all consumers add defines to the compile options. - Fix an issue in Napi::Error::New to allow Napi::Error to propogate properly. - Don't throw in Chakra's napi_add_finalizer to avoid compiler warning. - Fix an issue in XMLHttpRequest where some errors are being dropped. --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 5 ++ Core/AppRuntime/Source/AppRuntime_Android.cpp | 2 +- Core/AppRuntime/Source/AppRuntime_Chakra.cpp | 30 ++++--- .../Source/AppRuntime_JavaScriptCore.cpp | 3 + Core/AppRuntime/Source/AppRuntime_UWP.cpp | 2 +- Core/AppRuntime/Source/AppRuntime_Unix.cpp | 2 +- Core/AppRuntime/Source/AppRuntime_V8.cpp | 18 ++-- Core/AppRuntime/Source/AppRuntime_Win32.cpp | 2 +- Core/AppRuntime/Source/AppRuntime_iOS.mm | 2 +- Core/AppRuntime/Source/AppRuntime_macOS.mm | 2 +- Core/CMakeLists.txt | 2 + Core/JsRuntime/CMakeLists.txt | 1 + Core/Node-API-Extensions/CMakeLists.txt | 2 + .../include/napi/pointer.h | 87 +++++++++++++++++++ .../include/napi/utilities.h | 20 +++++ Core/Node-API-JSI/Include/napi/napi-inl.h | 2 + Core/Node-API/CMakeLists.txt | 7 -- .../Include/Shared/napi/js_native_api.h | 3 + .../Include/Shared/napi/js_native_api_types.h | 3 + Core/Node-API/Include/Shared/napi/napi-inl.h | 2 + Core/Node-API/Include/Shared/napi/napi.h | 17 ++-- Core/Node-API/Source/js_native_api_chakra.cc | 3 +- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 14 ++- Tests/UnitTests/Shared/Shared.cpp | 2 +- 24 files changed, 184 insertions(+), 49 deletions(-) create mode 100644 Core/Node-API-Extensions/CMakeLists.txt create mode 100644 Core/Node-API-Extensions/include/napi/pointer.h create mode 100644 Core/Node-API-Extensions/include/napi/utilities.h diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 8e474288..13291ff6 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -4,6 +4,8 @@ #include +#include + #include #include #include @@ -21,6 +23,9 @@ namespace Babylon // Optional handler for unhandled exceptions. std::function UnhandledExceptionHandler{DefaultUnhandledExceptionHandler}; + // Defines whether to enable the debugger. Defaults to false. + bool EnableDebugger{false}; + // Waits for the debugger to be attached before the execution of any script. Only implemented for V8. bool WaitForDebugger{false}; }; diff --git a/Core/AppRuntime/Source/AppRuntime_Android.cpp b/Core/AppRuntime/Source/AppRuntime_Android.cpp index 9ef69bcd..a7b5678c 100644 --- a/Core/AppRuntime/Source/AppRuntime_Android.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Android.cpp @@ -5,7 +5,7 @@ namespace Babylon { void AppRuntime::DefaultUnhandledExceptionHandler(const Napi::Error& error) { - __android_log_print(ANDROID_LOG_ERROR, "BabylonNative", "[Uncaught Error] %s", error.Get("stack").As().Utf8Value().data()); + __android_log_print(ANDROID_LOG_ERROR, "BabylonNative", "[Uncaught Error] %s", Napi::GetErrorString(error).data()); } void AppRuntime::RunPlatformTier() diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index e643b5d2..2329ad30 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -6,6 +6,7 @@ #include #include +#include namespace Babylon { @@ -15,7 +16,9 @@ namespace Babylon { if (errorCode != JsErrorCode::JsNoError) { - throw std::exception(); + std::ostringstream ss; + ss << "Chakra function failed with error code (" << static_cast(errorCode) << ")"; + throw std::runtime_error{ss.str()}; } } } @@ -35,21 +38,21 @@ namespace Babylon JsContextRef context; ThrowIfFailed(JsCreateContext(jsRuntime, &context)); ThrowIfFailed(JsSetCurrentContext(context)); - ThrowIfFailed(JsSetPromiseContinuationCallback([](JsValueRef task, void* callbackState) { - ThrowIfFailed(JsAddRef(task, nullptr)); - auto* dispatch = reinterpret_cast(callbackState); - dispatch->operator()([task]() { - JsValueRef undefined; - ThrowIfFailed(JsGetUndefinedValue(&undefined)); - ThrowIfFailed(JsCallFunction(task, &undefined, 1, nullptr)); - ThrowIfFailed(JsRelease(task, nullptr)); - }); - }, + ThrowIfFailed(JsSetPromiseContinuationCallback( + [](JsValueRef task, void* callbackState) { + ThrowIfFailed(JsAddRef(task, nullptr)); + auto* dispatch = reinterpret_cast(callbackState); + dispatch->operator()([task]() { + JsValueRef undefined; + ThrowIfFailed(JsGetUndefinedValue(&undefined)); + ThrowIfFailed(JsCallFunction(task, &undefined, 1, nullptr)); + ThrowIfFailed(JsRelease(task, nullptr)); + }); + }, &dispatchFunction)); ThrowIfFailed(JsProjectWinRTNamespace(L"Windows")); -#if defined(_DEBUG) - // Put Chakra in debug mode. + if (m_options.EnableDebugger) { auto result = JsStartDebugging(); if (result != JsErrorCode::JsNoError) @@ -57,7 +60,6 @@ namespace Babylon OutputDebugStringW(L"Failed to initialize JavaScript debugging support.\n"); } } -#endif Napi::Env env = Napi::Attach(); diff --git a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp index 86de4a50..e3cf6d91 100644 --- a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp @@ -6,6 +6,9 @@ namespace Babylon void AppRuntime::RunEnvironmentTier(const char*) { auto globalContext = JSGlobalContextCreateInGroup(nullptr, nullptr); + + JSGlobalContextSetInspectable(globalContext, m_options.EnableDebugger); + Napi::Env env = Napi::Attach(globalContext); Run(env); diff --git a/Core/AppRuntime/Source/AppRuntime_UWP.cpp b/Core/AppRuntime/Source/AppRuntime_UWP.cpp index 9ebae1fb..46fdb488 100644 --- a/Core/AppRuntime/Source/AppRuntime_UWP.cpp +++ b/Core/AppRuntime/Source/AppRuntime_UWP.cpp @@ -7,7 +7,7 @@ namespace Babylon void AppRuntime::DefaultUnhandledExceptionHandler(const Napi::Error& error) { std::ostringstream ss{}; - ss << "[Uncaught Error] " << error.Get("stack").As().Utf8Value() << std::endl; + ss << "[Uncaught Error] " << Napi::GetErrorString(error) << std::endl; OutputDebugStringA(ss.str().data()); } diff --git a/Core/AppRuntime/Source/AppRuntime_Unix.cpp b/Core/AppRuntime/Source/AppRuntime_Unix.cpp index 410e15ba..a90777c9 100644 --- a/Core/AppRuntime/Source/AppRuntime_Unix.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Unix.cpp @@ -5,7 +5,7 @@ namespace Babylon { void AppRuntime::DefaultUnhandledExceptionHandler(const Napi::Error& error) { - std::cerr << "[Uncaught Error] " << error.Get("stack").As().Utf8Value().data() << std::endl; + std::cerr << "[Uncaught Error] " << Napi::GetErrorString(error) << std::endl; } void AppRuntime::RunPlatformTier() diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index a2d0825f..6e62e568 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -90,19 +90,25 @@ namespace Babylon Napi::Env env = Napi::Attach(context); #ifdef ENABLE_V8_INSPECTOR - V8InspectorAgent agent{Module::Instance().Platform(), isolate, context, "JsRuntimeHost"}; - agent.Start(5643, "JsRuntimeHost"); - - if (m_options.WaitForDebugger) + if (m_options.EnableDebugger) { - agent.WaitForDebugger(); + V8InspectorAgent agent{Module::Instance().Platform(), isolate, context, "JsRuntimeHost"}; + agent.Start(5643, "JsRuntimeHost"); + + if (m_options.WaitForDebugger) + { + agent.WaitForDebugger(); + } } #endif Run(env); #ifdef ENABLE_V8_INSPECTOR - agent.Stop(); + if (m_options.EnableDebugger) + { + agent.Stop(); + } #endif Napi::Detach(env); diff --git a/Core/AppRuntime/Source/AppRuntime_Win32.cpp b/Core/AppRuntime/Source/AppRuntime_Win32.cpp index 81991a78..108503d8 100644 --- a/Core/AppRuntime/Source/AppRuntime_Win32.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Win32.cpp @@ -10,7 +10,7 @@ namespace Babylon void AppRuntime::DefaultUnhandledExceptionHandler(const Napi::Error& error) { std::ostringstream ss{}; - ss << "[Uncaught Error] " << error.Get("stack").As().Utf8Value() << std::endl; + ss << "[Uncaught Error] " << Napi::GetErrorString(error) << std::endl; OutputDebugStringA(ss.str().data()); } diff --git a/Core/AppRuntime/Source/AppRuntime_iOS.mm b/Core/AppRuntime/Source/AppRuntime_iOS.mm index 026c7a25..7b529115 100644 --- a/Core/AppRuntime/Source/AppRuntime_iOS.mm +++ b/Core/AppRuntime/Source/AppRuntime_iOS.mm @@ -5,7 +5,7 @@ { void AppRuntime::DefaultUnhandledExceptionHandler(const Napi::Error& error) { - NSLog(@"[Uncaught Error] %s", error.Get("stack").As().Utf8Value().data()); + NSLog(@"[Uncaught Error] %s", Napi::GetErrorString(error).data()); } void AppRuntime::RunPlatformTier() diff --git a/Core/AppRuntime/Source/AppRuntime_macOS.mm b/Core/AppRuntime/Source/AppRuntime_macOS.mm index 026c7a25..7b529115 100644 --- a/Core/AppRuntime/Source/AppRuntime_macOS.mm +++ b/Core/AppRuntime/Source/AppRuntime_macOS.mm @@ -5,7 +5,7 @@ { void AppRuntime::DefaultUnhandledExceptionHandler(const Napi::Error& error) { - NSLog(@"[Uncaught Error] %s", error.Get("stack").As().Utf8Value().data()); + NSLog(@"[Uncaught Error] %s", Napi::GetErrorString(error).data()); } void AppRuntime::RunPlatformTier() diff --git a/Core/CMakeLists.txt b/Core/CMakeLists.txt index eac2f222..e1caf8db 100644 --- a/Core/CMakeLists.txt +++ b/Core/CMakeLists.txt @@ -4,6 +4,8 @@ else() add_subdirectory(Node-API) endif() +add_subdirectory(Node-API-Extensions) + add_subdirectory(JsRuntime) if(JSRUNTIMEHOST_CORE_APPRUNTIME) diff --git a/Core/JsRuntime/CMakeLists.txt b/Core/JsRuntime/CMakeLists.txt index 98f85888..87e06089 100644 --- a/Core/JsRuntime/CMakeLists.txt +++ b/Core/JsRuntime/CMakeLists.txt @@ -11,6 +11,7 @@ target_include_directories(JsRuntime PUBLIC "Include") target_link_libraries(JsRuntime PUBLIC napi + PUBLIC napi-extensions PRIVATE arcana) set_property(TARGET JsRuntime PROPERTY FOLDER Core) diff --git a/Core/Node-API-Extensions/CMakeLists.txt b/Core/Node-API-Extensions/CMakeLists.txt new file mode 100644 index 00000000..e6b7c83e --- /dev/null +++ b/Core/Node-API-Extensions/CMakeLists.txt @@ -0,0 +1,2 @@ +add_library(napi-extensions INTERFACE) +target_include_directories(napi-extensions INTERFACE "include") diff --git a/Core/Node-API-Extensions/include/napi/pointer.h b/Core/Node-API-Extensions/include/napi/pointer.h new file mode 100644 index 00000000..bc16614a --- /dev/null +++ b/Core/Node-API-Extensions/include/napi/pointer.h @@ -0,0 +1,87 @@ +#pragma once + +#include + +#include +#include + +namespace Napi +{ + template + struct PointerTraits + { + using RepresentationT = uint32_t; + static_assert(sizeof(PointerT) % sizeof(RepresentationT) == 0); + static inline constexpr size_t ByteSize{sizeof(PointerT)}; + static inline constexpr size_t ArraySize{sizeof(PointerT) / sizeof(RepresentationT)}; + }; + + template + auto NapiPointerDeleter(PointerT pointer) + { + return [pointer]() + { + delete pointer; + }; + } + + class FunctionPointer + { + public: + template + static Napi::Value Create(Napi::Env env, ReturnT (ClassT::*functionPtr)(ArgsT...)) + { + using PointerT = decltype(functionPtr); + auto arrayBuffer = Napi::ArrayBuffer::New(env, PointerTraits::ByteSize); + std::memcpy(arrayBuffer.Data(), &functionPtr, sizeof(PointerT)); + return Napi::Uint32Array::New(env, PointerTraits::ArraySize, arrayBuffer, 0).template As(); + } + }; + + template + class Pointer + { + public: + static Napi::Value Create(Napi::Env env, const T* pointer) + { + using PointerT = T*; + auto arrayBuffer = Napi::ArrayBuffer::New(env, PointerTraits::ByteSize); + std::memcpy(arrayBuffer.Data(), &pointer, sizeof(PointerT)); + return Napi::Uint32Array::New(env, PointerTraits::ArraySize, arrayBuffer, 0); + } + + template + static Napi::Value Create(Napi::Env env, const T* pointer, CallableT&& finalizationCallback) + { + using PointerT = T*; + using DataT = std::array::ArraySize>; + auto finalizer = [callback = std::forward(finalizationCallback)](Napi::Env, void* ptr) + { + callback(); + delete reinterpret_cast(ptr); + }; + auto arrayBuffer = Napi::ArrayBuffer::New(env, new DataT, PointerTraits::ByteSize, std::move(finalizer)); + std::memcpy(arrayBuffer.Data(), &pointer, sizeof(PointerT)); + return Napi::Uint32Array::New(env, PointerTraits::ArraySize, arrayBuffer, 0); + } + + template + Pointer(EnvT&& env, ValueT&& value) + : m_pointer{*reinterpret_cast(Napi::Value(std::forward(env), std::forward(value)).As().Data())} + { + } + + T* Get() const + { + return m_pointer; + } + + T& operator*() const + { + return *m_pointer; + } + + private: + T* m_pointer; + }; +} diff --git a/Core/Node-API-Extensions/include/napi/utilities.h b/Core/Node-API-Extensions/include/napi/utilities.h new file mode 100644 index 00000000..72eac8f2 --- /dev/null +++ b/Core/Node-API-Extensions/include/napi/utilities.h @@ -0,0 +1,20 @@ +#pragma once + +#include +#include + +namespace Napi +{ + // Gets the `stack` or `message` property from the error object. + // The `message` property will be returned if the `stack` property is not defined. + static inline std::string GetErrorString(const Napi::Error& error) + { + Napi::Value value = error.Get("stack"); + if (value.IsUndefined()) + { + value = error.Get("message"); + } + + return value.ToString().Utf8Value(); + } +} diff --git a/Core/Node-API-JSI/Include/napi/napi-inl.h b/Core/Node-API-JSI/Include/napi/napi-inl.h index 9848f3b5..6f47a10c 100644 --- a/Core/Node-API-JSI/Include/napi/napi-inl.h +++ b/Core/Node-API-JSI/Include/napi/napi-inl.h @@ -1523,6 +1523,8 @@ inline Error Error::New(napi_env env, const std::exception& exception) { inline Error Error::New(napi_env env, const std::exception_ptr& exception_ptr) { try { std::rethrow_exception(exception_ptr); + } catch (const Napi::Error& error) { + return {env, error.Value()}; } catch (const std::exception& exception) { return Error::New(env, exception); } diff --git a/Core/Node-API/CMakeLists.txt b/Core/Node-API/CMakeLists.txt index 36730c7f..a43d58b7 100644 --- a/Core/Node-API/CMakeLists.txt +++ b/Core/Node-API/CMakeLists.txt @@ -162,13 +162,6 @@ endif() add_library(napi ${SOURCES}) -target_compile_definitions(napi - PUBLIC NODE_ADDON_API_DISABLE_DEPRECATED - PUBLIC NODE_ADDON_API_DISABLE_NODE_SPECIFIC - PUBLIC NAPI_VERSION=5 - PUBLIC NAPI_HAS_THREADS=0 - ${NAPI_DEFINITIONS}) - target_include_directories(napi ${INCLUDE_DIRECTORIES}) target_link_libraries(napi ${LINK_LIBRARIES}) diff --git a/Core/Node-API/Include/Shared/napi/js_native_api.h b/Core/Node-API/Include/Shared/napi/js_native_api.h index b5b386a1..a05aa651 100644 --- a/Core/Node-API/Include/Shared/napi/js_native_api.h +++ b/Core/Node-API/Include/Shared/napi/js_native_api.h @@ -1,6 +1,9 @@ #ifndef SRC_JS_NATIVE_API_H_ #define SRC_JS_NATIVE_API_H_ +// [BABYLON-NATIVE-ADDITION] +#define NAPI_VERSION 5 + // This file needs to be compatible with C compilers. #include // NOLINT(modernize-deprecated-headers) #include // NOLINT(modernize-deprecated-headers) diff --git a/Core/Node-API/Include/Shared/napi/js_native_api_types.h b/Core/Node-API/Include/Shared/napi/js_native_api_types.h index 14f02825..4a591777 100644 --- a/Core/Node-API/Include/Shared/napi/js_native_api_types.h +++ b/Core/Node-API/Include/Shared/napi/js_native_api_types.h @@ -1,6 +1,9 @@ #ifndef SRC_JS_NATIVE_API_TYPES_H_ #define SRC_JS_NATIVE_API_TYPES_H_ +// [BABYLON-NATIVE-ADDITION] +#define NAPI_VERSION 5 + // This file needs to be compatible with C compilers. // This is a public include file, and these includes have essentially // became part of it's API. diff --git a/Core/Node-API/Include/Shared/napi/napi-inl.h b/Core/Node-API/Include/Shared/napi/napi-inl.h index ea5cde8f..f4b3df07 100644 --- a/Core/Node-API/Include/Shared/napi/napi-inl.h +++ b/Core/Node-API/Include/Shared/napi/napi-inl.h @@ -2945,6 +2945,8 @@ inline Error Error::New(napi_env env, const std::exception& exception) { inline Error Error::New(napi_env env, const std::exception_ptr& exception_ptr) { try { std::rethrow_exception(exception_ptr); + } catch (const Napi::Error& error) { + return {env, error.Value()}; } catch (const std::exception& exception) { return Error::New(env, exception); } diff --git a/Core/Node-API/Include/Shared/napi/napi.h b/Core/Node-API/Include/Shared/napi/napi.h index 748439a4..f347091d 100644 --- a/Core/Node-API/Include/Shared/napi/napi.h +++ b/Core/Node-API/Include/Shared/napi/napi.h @@ -1,6 +1,12 @@ #ifndef SRC_NAPI_H_ #define SRC_NAPI_H_ +// [BABYLON-NATIVE-ADDITION] +#define NODE_ADDON_API_DISABLE_DEPRECATED +#define NODE_ADDON_API_DISABLE_NODE_SPECIFIC +#define NAPI_VERSION 5 +#define NAPI_HAS_THREADS 0 + #ifndef NAPI_HAS_THREADS #if !defined(__wasm__) || (defined(__EMSCRIPTEN_PTHREADS__) || \ (defined(__wasi__) && defined(_REENTRANT))) @@ -316,8 +322,7 @@ class Env { bool IsExceptionPending() const; Error GetAndClearPendingException() const; - // [BABYLON-NATIVE-ADDITION] - // url added and forwarded to napi_run_script call + // [BABYLON-NATIVE-ADDITION]: url added and forwarded to napi_run_script call MaybeOrValue RunScript(const char* utf8script, const char* url = "") const; MaybeOrValue RunScript(const std::string& utf8script, const char* url = "") const; MaybeOrValue RunScript(String script, const char* url = "") const; @@ -1180,12 +1185,10 @@ class ArrayBuffer : public Object { ArrayBuffer(napi_env env, napi_value value); ///< Wraps a Node-API value primitive. - // [BABYLON-NATIVE-ADDITION] - // these methods are made const for Babylon Native + // [BABYLON-NATIVE-ADDITION]: these methods are made const void* Data() const; ///< Gets a pointer to the data buffer. size_t ByteLength() const; ///< Gets the length of the array buffer in bytes. - // [BABYLON-NATIVE-ADDITION] private: mutable void* _data; mutable size_t _length; @@ -1515,6 +1518,7 @@ class Promise : public Object { Promise(napi_env env, napi_value value); }; + // [BABYLON-NATIVE-ADDITION] #ifndef NODE_ADDON_API_DISABLE_NODE_SPECIFIC template @@ -1685,7 +1689,8 @@ class FunctionReference : public Reference { MaybeOrValue Call(napi_value recv, size_t argc, const napi_value* args) const; - // [BABYLON-NATIVE-ADDITION] + +// [BABYLON-NATIVE-ADDITION] #ifndef NODE_ADDON_API_DISABLE_NODE_SPECIFIC MaybeOrValue MakeCallback( napi_value recv, diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 8824140d..8fde5a59 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -2407,7 +2407,8 @@ napi_status napi_add_finalizer(napi_env env, napi_finalize finalize_cb, void* finalize_hint, napi_ref* result) { - throw std::runtime_error("not impl"); + // TODO: not implemented + return napi_generic_failure; } napi_status napi_adjust_external_memory(napi_env env, diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 2d12b0aa..a03fb9aa 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -1,7 +1,6 @@ #include "XMLHttpRequest.h" #include #include -#include namespace Babylon::Polyfills::Internal { @@ -255,19 +254,18 @@ namespace Babylon::Polyfills::Internal } } - m_request.SendAsync().then(m_runtimeScheduler, arcana::cancellation::none(), [env{info.Env()}, this](arcana::expected result) { - if (result.has_error()) - { - Napi::Error::New(env, result.error()).ThrowAsJavaScriptException(); - return; - } - + m_request.SendAsync().then(m_runtimeScheduler, arcana::cancellation::none(), [this]() { SetReadyState(ReadyState::Done); RaiseEvent(EventType::LoadEnd); // Assume the XMLHttpRequest will only be used for a single request and clear the event handlers. // Single use seems to be the standard pattern, and we need to release our strong refs to event handlers. m_eventHandlerRefs.clear(); + }).then(arcana::inline_scheduler, arcana::cancellation::none(), [env = info.Env()](arcana::expected result) { + if (result.has_error()) + { + Napi::Error::New(env, result.error()).ThrowAsJavaScriptException(); + } }); } diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 3ebde751..f140f381 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -39,7 +39,7 @@ TEST(JavaScript, All) Babylon::AppRuntime::Options options{}; options.UnhandledExceptionHandler = [&exitCodePromise](const Napi::Error& error) { - std::cerr << "[Uncaught Error] " << error.Get("stack").As().Utf8Value() << std::endl; + std::cerr << "[Uncaught Error] " << Napi::GetErrorString(error) << std::endl; std::cerr.flush(); exitCodePromise.set_value(-1); From 4be1f9a7ec4660aa9d1f4cd2fba5864fcc7ef6c1 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Fri, 5 Apr 2024 17:42:11 -0700 Subject: [PATCH 03/10] Add ifndef guards --- Core/Node-API/Include/Shared/napi/js_native_api.h | 2 ++ Core/Node-API/Include/Shared/napi/js_native_api_types.h | 2 ++ Core/Node-API/Include/Shared/napi/napi.h | 8 ++++++++ 3 files changed, 12 insertions(+) diff --git a/Core/Node-API/Include/Shared/napi/js_native_api.h b/Core/Node-API/Include/Shared/napi/js_native_api.h index a05aa651..961b30f2 100644 --- a/Core/Node-API/Include/Shared/napi/js_native_api.h +++ b/Core/Node-API/Include/Shared/napi/js_native_api.h @@ -2,7 +2,9 @@ #define SRC_JS_NATIVE_API_H_ // [BABYLON-NATIVE-ADDITION] +#ifndef NAPI_VERSION #define NAPI_VERSION 5 +#endif // This file needs to be compatible with C compilers. #include // NOLINT(modernize-deprecated-headers) diff --git a/Core/Node-API/Include/Shared/napi/js_native_api_types.h b/Core/Node-API/Include/Shared/napi/js_native_api_types.h index 4a591777..bea78ecd 100644 --- a/Core/Node-API/Include/Shared/napi/js_native_api_types.h +++ b/Core/Node-API/Include/Shared/napi/js_native_api_types.h @@ -2,7 +2,9 @@ #define SRC_JS_NATIVE_API_TYPES_H_ // [BABYLON-NATIVE-ADDITION] +#ifndef NAPI_VERSION #define NAPI_VERSION 5 +#endif // This file needs to be compatible with C compilers. // This is a public include file, and these includes have essentially diff --git a/Core/Node-API/Include/Shared/napi/napi.h b/Core/Node-API/Include/Shared/napi/napi.h index f347091d..247dadcb 100644 --- a/Core/Node-API/Include/Shared/napi/napi.h +++ b/Core/Node-API/Include/Shared/napi/napi.h @@ -2,10 +2,18 @@ #define SRC_NAPI_H_ // [BABYLON-NATIVE-ADDITION] +#ifndef NODE_ADDON_API_DISABLE_DEPRECATED #define NODE_ADDON_API_DISABLE_DEPRECATED +#endif +#ifndef NODE_ADDON_API_DISABLE_NODE_SPECIFIC #define NODE_ADDON_API_DISABLE_NODE_SPECIFIC +#endif +#ifndef NAPI_VERSION #define NAPI_VERSION 5 +#endif +#ifndef NAPI_HAS_THREADS #define NAPI_HAS_THREADS 0 +#endif #ifndef NAPI_HAS_THREADS #if !defined(__wasm__) || (defined(__EMSCRIPTEN_PTHREADS__) || \ From 0c6a5bc708a037bb552bab70b39318c3a9105134 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 16 Apr 2024 09:00:18 -0700 Subject: [PATCH 04/10] Fix build issue --- Core/Node-API-JSI/Include/napi/napi-inl.h | 2 +- Core/Node-API/Include/Shared/napi/napi-inl.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Core/Node-API-JSI/Include/napi/napi-inl.h b/Core/Node-API-JSI/Include/napi/napi-inl.h index 6f47a10c..cc70a235 100644 --- a/Core/Node-API-JSI/Include/napi/napi-inl.h +++ b/Core/Node-API-JSI/Include/napi/napi-inl.h @@ -1524,7 +1524,7 @@ inline Error Error::New(napi_env env, const std::exception_ptr& exception_ptr) { try { std::rethrow_exception(exception_ptr); } catch (const Napi::Error& error) { - return {env, error.Value()}; + return error; } catch (const std::exception& exception) { return Error::New(env, exception); } diff --git a/Core/Node-API/Include/Shared/napi/napi-inl.h b/Core/Node-API/Include/Shared/napi/napi-inl.h index 0647d3d4..438a23d0 100644 --- a/Core/Node-API/Include/Shared/napi/napi-inl.h +++ b/Core/Node-API/Include/Shared/napi/napi-inl.h @@ -2946,7 +2946,7 @@ inline Error Error::New(napi_env env, const std::exception_ptr& exception_ptr) { try { std::rethrow_exception(exception_ptr); } catch (const Napi::Error& error) { - return {env, error.Value()}; + return error; } catch (const std::exception& exception) { return Error::New(env, exception); } From 1b12f870f80c377ae0a5afd73d6572ded9566796 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 16 Apr 2024 09:21:02 -0700 Subject: [PATCH 05/10] More build fixes --- Core/AppRuntime/Source/AppRuntime_V8.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index 6e62e568..87f7799a 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -17,6 +17,8 @@ #include #endif +#include + namespace Babylon { namespace @@ -90,14 +92,15 @@ namespace Babylon Napi::Env env = Napi::Attach(context); #ifdef ENABLE_V8_INSPECTOR + std::optional agent; if (m_options.EnableDebugger) { - V8InspectorAgent agent{Module::Instance().Platform(), isolate, context, "JsRuntimeHost"}; - agent.Start(5643, "JsRuntimeHost"); + agent.emplace(Module::Instance().Platform(), isolate, context, "JsRuntimeHost"); + agent->Start(5643, "JsRuntimeHost"); if (m_options.WaitForDebugger) { - agent.WaitForDebugger(); + agent->WaitForDebugger(); } } #endif @@ -105,9 +108,9 @@ namespace Babylon Run(env); #ifdef ENABLE_V8_INSPECTOR - if (m_options.EnableDebugger) + if (agent.has_value()) { - agent.Stop(); + agent->Stop(); } #endif From 6ecfd7d7e6c5b41c73a75851615381513d544b5b Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 16 Apr 2024 15:10:21 -0700 Subject: [PATCH 06/10] Stop using `JSGlobalContextSetInspectable` since it's not supported in all cases --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 2 +- Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 605489d0..7aaeb398 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -23,7 +23,7 @@ namespace Babylon // Optional handler for unhandled exceptions. std::function UnhandledExceptionHandler{DefaultUnhandledExceptionHandler}; - // Defines whether to enable the debugger. Defaults to false. + // Defines whether to enable the debugger. Only implemented for V8 and Chakra. bool EnableDebugger{false}; // Waits for the debugger to be attached before the execution of any script. Only implemented for V8. diff --git a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp index e3cf6d91..4ddcf866 100644 --- a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp @@ -7,7 +7,8 @@ namespace Babylon { auto globalContext = JSGlobalContextCreateInGroup(nullptr, nullptr); - JSGlobalContextSetInspectable(globalContext, m_options.EnableDebugger); + // REVIEW: Ideally, we should call this, but it's not always available in all situations. + //JSGlobalContextSetInspectable(globalContext, m_options.EnableDebugger); Napi::Env env = Napi::Attach(globalContext); From d1d26be9088c00212c88c3f87f59866922fe5898 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 16 Apr 2024 15:55:56 -0700 Subject: [PATCH 07/10] V8_COMPRESS_POINTERS and V8_ENABLE_SANDBOX --- Core/AppRuntime/Source/AppRuntime_V8.cpp | 10 ---------- Core/AppRuntime/V8Inspector/Include/V8Inc.h | 13 +++++++++++++ Core/Node-API/CMakeLists.txt | 9 --------- Core/Node-API/Include/Engine/V8/napi/env.h | 15 ++++++++++++++- Core/Node-API/Source/js_native_api_v8_internals.h | 13 +++++++++++++ 5 files changed, 40 insertions(+), 20 deletions(-) diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index 87f7799a..1297fdbb 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -1,16 +1,6 @@ #include "AppRuntime.h" #include -#if defined(_MSC_VER) -#pragma warning(disable : 4100 4267 4127) -#endif -#if defined(__clang__) -#pragma clang diagnostic ignored "-Wunused-parameter" -#endif -#if defined(__GNUC__) -#pragma GCC diagnostic ignored "-Wunused-parameter" -#endif -#include #include #ifdef ENABLE_V8_INSPECTOR diff --git a/Core/AppRuntime/V8Inspector/Include/V8Inc.h b/Core/AppRuntime/V8Inspector/Include/V8Inc.h index 868e106b..941200dc 100644 --- a/Core/AppRuntime/V8Inspector/Include/V8Inc.h +++ b/Core/AppRuntime/V8Inspector/Include/V8Inc.h @@ -1,5 +1,18 @@ #pragma once +// Enable V8 Pointer Compression +// https://v8.dev/blog/pointer-compression +// https://stackoverflow.com/q/62921373 +#ifndef V8_COMPRESS_POINTERS +#define V8_COMPRESS_POINTERS 1 +#endif + +// Enable V8 Sandbox +// https://v8.dev/blog/sandbox +#ifndef V8_ENABLE_SANDBOX +#define V8_ENABLE_SANDBOX 1 +#endif + #ifdef _MSC_VER #pragma warning(push) #pragma warning(disable: 4100 4267 4127) diff --git a/Core/Node-API/CMakeLists.txt b/Core/Node-API/CMakeLists.txt index a43d58b7..3f88fccb 100644 --- a/Core/Node-API/CMakeLists.txt +++ b/Core/Node-API/CMakeLists.txt @@ -97,8 +97,6 @@ if(NAPI_BUILD_ABI) set(LINK_LIBRARIES ${LINK_LIBRARIES} PUBLIC "${V8_ANDROID_DIR}/jni/${ANDROID_ABI}/libv8android.so") - - set(NAPI_DEFINITIONS PUBLIC V8_COMPRESS_POINTERS) elseif(WIN32) set_cpu_platform_arch() set(V8_VERSION "11.9.169.4") @@ -143,13 +141,6 @@ if(NAPI_BUILD_ABI) PRIVATE v8::v8_libbase PRIVATE v8::v8_libplatform PRIVATE v8::third_party_zlib) - - if(CPU_ARCH STREQUAL "x64") - # Enable V8 Pointer Compression - # https://v8.dev/blog/pointer-compression - # https://stackoverflow.com/q/62921373 - set(NAPI_DEFINITIONS PUBLIC V8_COMPRESS_POINTERS V8_ENABLE_SANDBOX) - endif() else() message(FATAL_ERROR "Unsupported JavaScript engine: ${NAPI_JAVASCRIPT_ENGINE}") endif() diff --git a/Core/Node-API/Include/Engine/V8/napi/env.h b/Core/Node-API/Include/Engine/V8/napi/env.h index e5e84d82..561db47e 100644 --- a/Core/Node-API/Include/Engine/V8/napi/env.h +++ b/Core/Node-API/Include/Engine/V8/napi/env.h @@ -2,6 +2,19 @@ #include +// Enable V8 Pointer Compression +// https://v8.dev/blog/pointer-compression +// https://stackoverflow.com/q/62921373 +#ifndef V8_COMPRESS_POINTERS +#define V8_COMPRESS_POINTERS 1 +#endif + +// Enable V8 Sandbox +// https://v8.dev/blog/sandbox +#ifndef V8_ENABLE_SANDBOX +#define V8_ENABLE_SANDBOX 1 +#endif + #ifdef __clang__ #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wunused-parameter" @@ -14,7 +27,7 @@ #ifdef _MSC_VER #pragma warning(push) -#pragma warning(disable: 4100) // unreferenced formal parameter +#pragma warning(disable: 4100) // unreferenced formal parameter #pragma warning(disable: 4127) // Suppress warning in v8-internal.h, Line 317 inside of V8_COMPRESS_POINTERS conditional #endif diff --git a/Core/Node-API/Source/js_native_api_v8_internals.h b/Core/Node-API/Source/js_native_api_v8_internals.h index e22cad92..c99bdaf6 100644 --- a/Core/Node-API/Source/js_native_api_v8_internals.h +++ b/Core/Node-API/Source/js_native_api_v8_internals.h @@ -13,6 +13,19 @@ // are bridged to remove references to the `node` namespace. `node_version.h`, // included below, defines `NAPI_VERSION`. +// [BABYLON-NATIVE-ADDITION]: Enable V8 Pointer Compression +// https://v8.dev/blog/pointer-compression +// https://stackoverflow.com/q/62921373 +#ifndef V8_COMPRESS_POINTERS +#define V8_COMPRESS_POINTERS 1 +#endif + +// [BABYLON-NATIVE-ADDITION]: Enable V8 Sandbox +// https://v8.dev/blog/sandbox +#ifndef V8_ENABLE_SANDBOX +#define V8_ENABLE_SANDBOX 1 +#endif + #include #include From 9a1791e9363b6175949caf014844badbe74f223d Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 16 Apr 2024 16:36:30 -0700 Subject: [PATCH 08/10] v8 fixes --- Core/Node-API/Source/js_native_api_v8.cc | 57 +++++++----------------- 1 file changed, 17 insertions(+), 40 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_v8.cc b/Core/Node-API/Source/js_native_api_v8.cc index 488e1d74..f5e2896c 100644 --- a/Core/Node-API/Source/js_native_api_v8.cc +++ b/Core/Node-API/Source/js_native_api_v8.cc @@ -2944,20 +2944,6 @@ napi_status NAPI_CDECL napi_create_arraybuffer(napi_env env, return GET_RETURN_STATUS(env); } -struct backingStorDeleterCallbackWrapper -{ - napi_env env; - napi_finalize finalize_cb; - void* finalize_hint; -}; - -void backingStorDeleterCallback(void* data, size_t length, void* deleter_data) -{ - auto deleterData = (backingStorDeleterCallbackWrapper*)deleter_data; - deleterData->finalize_cb(deleterData->env, data, deleterData->finalize_hint); - delete deleterData; -} - napi_status NAPI_CDECL napi_create_external_arraybuffer(napi_env env, void* external_data, @@ -2970,32 +2956,23 @@ napi_create_external_arraybuffer(napi_env env, v8::Isolate* isolate = env->isolate; - std::unique_ptr backingStore; -#if defined(V8_ENABLE_SANDBOX) - v8::Local buffer = v8::ArrayBuffer::New(isolate, byte_length); - memcpy(buffer->Data(), external_data, byte_length); - if (finalize_cb != nullptr) { - // Create a self-deleting weak reference that invokes the finalizer - // callback. - v8impl::Reference::New(env, - buffer, - 0, - v8impl::Ownership::kUserland, - finalize_cb, - external_data, - finalize_hint); - } -#else - if (finalize_cb != nullptr) { - backingStore = v8::ArrayBuffer::NewBackingStore(external_data, byte_length, backingStorDeleterCallback, new backingStorDeleterCallbackWrapper{ env,finalize_cb, finalize_hint }); - } - else { - backingStore = v8::ArrayBuffer::NewBackingStore(isolate, byte_length); - memcpy(backingStore->Data(), external_data, byte_length); - } - v8::Local buffer = v8::ArrayBuffer::New(isolate, std::move(backingStore)); -#endif - * result = v8impl::JsValueFromV8LocalValue(buffer); + struct FinalizeData + { + napi_env env; + napi_finalize finalize_cb; + void* finalize_hint; + + static void Finalize(void* data, size_t length, void* deleter_data) { + auto finalize_data = reinterpret_cast(deleter_data); + finalize_data->finalize_cb(finalize_data->env, data, finalize_data->finalize_hint); + delete finalize_data; + } + }; + + auto backingStore = v8::ArrayBuffer::NewBackingStore(external_data, byte_length, FinalizeData::Finalize, new FinalizeData{env, finalize_cb, finalize_hint}); + + auto buffer = v8::ArrayBuffer::New(isolate, std::move(backingStore)); + *result = v8impl::JsValueFromV8LocalValue(buffer); return GET_RETURN_STATUS(env); } From 3379a019807b3a7773dffa87d6c83ff28100d361 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Thu, 18 Apr 2024 15:02:09 -0700 Subject: [PATCH 09/10] More v8 fixes --- Core/Node-API/Source/js_native_api_v8.cc | 30 +++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_v8.cc b/Core/Node-API/Source/js_native_api_v8.cc index f5e2896c..92ce4221 100644 --- a/Core/Node-API/Source/js_native_api_v8.cc +++ b/Core/Node-API/Source/js_native_api_v8.cc @@ -2956,6 +2956,24 @@ napi_create_external_arraybuffer(napi_env env, v8::Isolate* isolate = env->isolate; +#ifdef V8_ENABLE_SANDBOX + // TODO: We should error out like what happens with node.js. For now, we will copy the buffer instead. + auto buffer = v8::ArrayBuffer::New(isolate, byte_length); + std::memcpy(buffer->Data(), external_data, byte_length); + + if (finalize_cb != nullptr) { + // Create a self-deleting weak reference that invokes the finalizer + // callback. + v8impl::Reference::New(env, + buffer, + 0, + v8impl::Ownership::kUserland, + finalize_cb, + external_data, + finalize_hint); + } +#else + // TODO: This code is untested struct FinalizeData { napi_env env; @@ -2963,16 +2981,22 @@ napi_create_external_arraybuffer(napi_env env, void* finalize_hint; static void Finalize(void* data, size_t length, void* deleter_data) { + // TODO: Is this on the right thread? auto finalize_data = reinterpret_cast(deleter_data); - finalize_data->finalize_cb(finalize_data->env, data, finalize_data->finalize_hint); + + if (finalize_data->finalize_cb != nullptr) { + finalize_data->finalize_cb(finalize_data->env, data, finalize_data->finalize_hint); + } + delete finalize_data; } }; - auto backingStore = v8::ArrayBuffer::NewBackingStore(external_data, byte_length, FinalizeData::Finalize, new FinalizeData{env, finalize_cb, finalize_hint}); + auto buffer = v8::ArrayBuffer::New(isolate, v8::ArrayBuffer::NewBackingStore(external_data, byte_length, FinalizeData::Finalize, new FinalizeData{env, finalize_cb, finalize_hint})); +#endif - auto buffer = v8::ArrayBuffer::New(isolate, std::move(backingStore)); *result = v8impl::JsValueFromV8LocalValue(buffer); + return GET_RETURN_STATUS(env); } From 718fffd6976183bc3007ac405323c0df3a9d28b0 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Thu, 18 Apr 2024 15:40:11 -0700 Subject: [PATCH 10/10] Fix build --- Core/Node-API/Source/js_native_api_v8.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/Node-API/Source/js_native_api_v8.cc b/Core/Node-API/Source/js_native_api_v8.cc index 92ce4221..9b895764 100644 --- a/Core/Node-API/Source/js_native_api_v8.cc +++ b/Core/Node-API/Source/js_native_api_v8.cc @@ -2959,7 +2959,7 @@ napi_create_external_arraybuffer(napi_env env, #ifdef V8_ENABLE_SANDBOX // TODO: We should error out like what happens with node.js. For now, we will copy the buffer instead. auto buffer = v8::ArrayBuffer::New(isolate, byte_length); - std::memcpy(buffer->Data(), external_data, byte_length); + std::memcpy(buffer->GetBackingStore()->Data(), external_data, byte_length); if (finalize_cb != nullptr) { // Create a self-deleting weak reference that invokes the finalizer