refactor: move embedded runtime JS to real .js files (js2c) - #1989
refactor: move embedded runtime JS to real .js files (js2c)#1989edusperoni wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR adds runtime JavaScript builtins, a Node.js-to-C++ generator, V8 code caching, and a ChangesRuntime builtin pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant BuiltinLoader
participant V8
Runtime->>BuiltinLoader: Load builtin with binding
BuiltinLoader->>V8: Compile or reuse cached wrapper
V8-->>BuiltinLoader: Execute CommonJS wrapper
BuiltinLoader-->>Runtime: Return module.exports
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The runtime's internal JavaScript lived as C++ string literals across eight files, unlintable and invisible to tooling. It now lives in real .js files under test-app/runtime/src/main/cpp/js, embedded into a generated C++ table by tools/js2c.mjs at build time and executed through a new BuiltinLoader. Each file is compiled with v8::ScriptCompiler::CompileFunction as a function body with the fixed parameters `exports`, `module` and `binding` (Node's module wrapper plus its internalBinding idiom): natives arrive as properties of a binding bag built at the C++ call site, results come back through module.exports, and the script origin is internal/<name>.js so runtime frames stay identifiable in stack traces. Compilation goes through a process-wide bytecode cache guarded by a mutex, since worker runtimes initialize on their own threads. Extracted: weak-ref, message-loop-timer, smart-stringify, require-factory, json-helper, events, error-events and blob-url. Each extraction was verified AST-identical to the original literal by byte-comparing esbuild-minified output of both. tools/js2c.mjs is taken from the iOS runtime's feat/ns-util branch, which includes the later `unsigned char` fix for source bytes >= 0x80 (a narrowing error in a plain char array). Its --filelist drift check is adapted to --check-dir, comparing the explicit RUNTIME_BUILTIN_JS list in CMakeLists.txt against the directory contents so a new builtin cannot be silently skipped on incremental builds. Two behavioural notes: - JSONObjectHelper recompiled its JS->org.json serializer on every MetadataNode `from` registration. It is now compiled once per isolate and released via the isolate-dispose hook. - __messageLoopTimerStart/__messageLoopTimerStop are no longer installed on the global object. Nothing outside MessageLoopTimer referenced them, and the timer's start/stop pair now reaches its builtin through the binding bag. Mirrors NativeScript/ios#411.
f32561f to
05dc54b
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
test-app/runtime/src/main/cpp/BuiltinLoader.cpp (2)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
context->GetIsolate()overIsolate::GetCurrent().Both functions already receive the
Local<Context>. Deriving the isolate from the context removes the dependency on thread-local state and keeps the isolate and the context consistent.Also applies to: 91-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/BuiltinLoader.cpp` around lines 32 - 35, Update CompileBuiltin and the other indicated context-based code path to obtain the isolate from the provided Local<Context> via context->GetIsolate() instead of Isolate::GetCurrent(), keeping the isolate associated with the context and removing reliance on thread-local state.
54-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the code-cache compile attempt with a
TryCatch.A rejected cache does not fail the compile; V8 recompiles internally and sets
rejected. A failedCompileFunctiontherefore signals a real error and schedules an exception on the isolate. The code then falls through and callsCompileFunctiona second time while that exception is still pending. Wrap the first attempt in av8::TryCatchso the fallback compile starts from a clean state.🛡️ Proposed fix
Local<v8::Function> fn; if (!blob.empty()) { // The Source owns and deletes the CachedData object; BufferNotOwned // keeps the underlying bytes (our copy) out of its hands. auto* cachedData = new ScriptCompiler::CachedData( blob.data(), static_cast<int>(blob.size()), ScriptCompiler::CachedData::BufferNotOwned); ScriptCompiler::Source source(sourceText, origin, cachedData); - if (ScriptCompiler::CompileFunction(context, &source, kParamCount, params, 0, nullptr, - ScriptCompiler::kConsumeCodeCache) - .ToLocal(&fn) && - !cachedData->rejected) { - return fn; + { + TryCatch tc(isolate); + if (ScriptCompiler::CompileFunction(context, &source, kParamCount, params, 0, nullptr, + ScriptCompiler::kConsumeCodeCache) + .ToLocal(&fn) && + !cachedData->rejected) { + return fn; + } + tc.Reset(); } // Rejected cache (e.g. produced under different flags): fall through🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/BuiltinLoader.cpp` around lines 54 - 77, Wrap the cached-code CompileFunction attempt in the blob-handling branch with a v8::TryCatch, covering the call and rejected-cache check before falling through. If the attempt fails, clear or otherwise handle the caught exception so the subsequent eager CompileFunction starts with a clean isolate state, while preserving the existing successful-cache return and rejected-cache fallback behavior.test-app/runtime/src/main/cpp/MessageLoopTimer.cpp (1)
32-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
assertremoves the failure check in release builds.
NDEBUGbuilds drop theassert. A failed builtin load then continues silently, and the message-loop timer never installs itssetTimeout/setIntervalbehavior.Events::InitandErrorEvents::InitthrowNativeScriptExceptionon the same failure. Align this path with that behavior.♻️ Proposed change
- success = !BuiltinLoader::RunBuiltin(context, BuiltinId::kMessageLoopTimer, binding).IsEmpty(); - assert(success); + if (BuiltinLoader::RunBuiltin(context, BuiltinId::kMessageLoopTimer, binding).IsEmpty()) { + throw NativeScriptException("MessageLoopTimer::Init: the message-loop-timer builtin failed to run"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/MessageLoopTimer.cpp` around lines 32 - 33, Update the builtin-loading failure path in MessageLoopTimer initialization around BuiltinLoader::RunBuiltin so it throws NativeScriptException when the result is empty instead of relying on assert(success). Preserve successful initialization while ensuring failures are enforced in release builds, consistent with Events::Init and ErrorEvents::Init.test-app/runtime/src/main/cpp/JSONObjectHelper.cpp (1)
84-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the isolate from
contextand reporting builtin-compile failures.
GetSerializeFunctakesLocal<Context> contextbut obtains the isolate viaIsolate::GetCurrent()instead ofcontext->GetIsolate(). This works today becauseRegisterFromFunctionalways enters anIsolate::Scopebefore calling this method, but it is not self-evident from the signature and creates an implicit dependency on caller-established TLS state.Separately,
BuiltinLoader::RunBuiltinis called without aTryCatch. Ifjson-helper.jsfails to compile or execute, the function safely returnsnullptr, but any underlying JS exception is not captured or logged here, unlikeConvertCallbackStatic, which wraps its call in aTryCatchfor diagnostics. Confirm whether an outerTryCatchin the call chain already handles this, and consider adding a local one for clearer error reporting.♻️ Optional refactor using `context->GetIsolate()`
Persistent<Function>* JSONObjectHelper::GetSerializeFunc(Local<Context> context) { - Isolate* isolate = v8::Isolate::GetCurrent(); + Isolate* isolate = context->GetIsolate();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/JSONObjectHelper.cpp` around lines 84 - 102, Update JSONObjectHelper::GetSerializeFunc to derive the isolate from context via context->GetIsolate() rather than Isolate::GetCurrent(). Also add local TryCatch handling around BuiltinLoader::RunBuiltin and report any compile or execution exception before returning nullptr, matching the diagnostic behavior used by ConvertCallbackStatic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-app/runtime/CMakeLists.txt`:
- Around line 84-91: Update the CMake configuration around the custom command
using RUNTIME_BUILTIN_JS so additions or removals of builtin JavaScript files
trigger reconfiguration or validation before incremental builds. Prefer a
configure-time file(GLOB ... CONFIGURE_DEPENDS) check that compares the
discovered files with RUNTIME_BUILTIN_JS, while preserving the existing
generation command and dependency behavior.
- Around line 84-94: Update the generated RuntimeBuiltins outputs around
add_custom_command so both RuntimeBuiltins.h and RuntimeBuiltins.cpp are
produced by a named custom target, then attach that target to NativeScript with
add_dependencies(). Preserve the existing generation command and ensure
BuiltinLoader.cpp cannot compile until the generated header target completes.
In `@test-app/runtime/src/main/cpp/ErrorEvents.cpp`:
- Around line 54-58: Check the Maybe<bool> results from both binding->Set calls
in test-app/runtime/src/main/cpp/ErrorEvents.cpp lines 54-58 for globalTarget
and nativeReportFatal, and throw NativeScriptException when either result is
empty or false. Apply the same checked-result handling to both Set calls in
test-app/runtime/src/main/cpp/MessageLoopTimer.cpp lines 28-30 for
messageLoopTimerStart and messageLoopTimerStop, failing initialization when
either result is empty or false.
In `@test-app/runtime/src/main/cpp/js/blob-url.js`:
- Around line 27-59: Update the URL search mutation paths associated with the
searchParams getter so _searchParams is cleared whenever the URL’s search value
changes, including direct search and href assignments. Preserve the existing
write-back behavior while ensuring subsequent searchParams access rebuilds from
the current query component.
In `@test-app/runtime/src/main/cpp/js/message-loop-timer.js`:
- Around line 9-35: Update the wrapped WebAssembly methods in the Proxy get
handler to catch synchronous exceptions from origMethod.apply after
messageLoopTimerStart(), call messageLoopTimerStop(), and rethrow; preserve the
existing timer cleanup for both resolved and rejected asynchronous results.
In `@test-app/runtime/src/main/cpp/JSONObjectHelper.cpp`:
- Around line 4-13: Protect the process-wide isolateToSerializeFunc map with a
std::mutex, including the existing read/insert logic in
JSONObjectHelper::GetSerializeFunc() and erase logic in
JSONObjectHelper::onDisposeIsolate(). Add the mutex header and lock around every
map access, ensuring lookups and disposal cannot race.
In `@test-app/runtime/src/main/cpp/ModuleInternal.cpp`:
- Around line 89-91: Replace the assert-only builtin-load checks with explicit
NativeScriptException failure handling at all three sites: in
test-app/runtime/src/main/cpp/ModuleInternal.cpp lines 89-91, validate both
RunBuiltin().ToLocal(&result) and result->IsFunction() before
result.As<Function>() is used; in
test-app/runtime/src/main/cpp/MessageLoopTimer.cpp lines 32-33 and
test-app/runtime/src/main/cpp/WeakRef.cpp lines 17-18, check the resulting local
for emptiness and throw NativeScriptException on failure. Preserve successful
bootstrap behavior.
In `@test-app/runtime/src/main/cpp/Runtime.cpp`:
- Line 847: Handle the MaybeLocal result returned by BuiltinLoader::RunBuiltin
for BuiltinId::kBlobUrl, matching the error-checking pattern used by the other
builtin loads. Ensure compilation or execution failure is detected and reported
before bootstrap continues to Events::Init, ErrorEvents::Init, or m_module.Init,
rather than leaving a pending isolate exception.
---
Nitpick comments:
In `@test-app/runtime/src/main/cpp/BuiltinLoader.cpp`:
- Around line 32-35: Update CompileBuiltin and the other indicated context-based
code path to obtain the isolate from the provided Local<Context> via
context->GetIsolate() instead of Isolate::GetCurrent(), keeping the isolate
associated with the context and removing reliance on thread-local state.
- Around line 54-77: Wrap the cached-code CompileFunction attempt in the
blob-handling branch with a v8::TryCatch, covering the call and rejected-cache
check before falling through. If the attempt fails, clear or otherwise handle
the caught exception so the subsequent eager CompileFunction starts with a clean
isolate state, while preserving the existing successful-cache return and
rejected-cache fallback behavior.
In `@test-app/runtime/src/main/cpp/JSONObjectHelper.cpp`:
- Around line 84-102: Update JSONObjectHelper::GetSerializeFunc to derive the
isolate from context via context->GetIsolate() rather than
Isolate::GetCurrent(). Also add local TryCatch handling around
BuiltinLoader::RunBuiltin and report any compile or execution exception before
returning nullptr, matching the diagnostic behavior used by
ConvertCallbackStatic.
In `@test-app/runtime/src/main/cpp/MessageLoopTimer.cpp`:
- Around line 32-33: Update the builtin-loading failure path in MessageLoopTimer
initialization around BuiltinLoader::RunBuiltin so it throws
NativeScriptException when the result is empty instead of relying on
assert(success). Preserve successful initialization while ensuring failures are
enforced in release builds, consistent with Events::Init and ErrorEvents::Init.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f265420-0ec4-474f-8b88-cddbb714ae46
📒 Files selected for processing (28)
.gitignoreeslint.config.mjspackage.jsontest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/BuiltinLoader.cpptest-app/runtime/src/main/cpp/BuiltinLoader.htest-app/runtime/src/main/cpp/ErrorEvents.cpptest-app/runtime/src/main/cpp/ErrorEvents.htest-app/runtime/src/main/cpp/Events.cpptest-app/runtime/src/main/cpp/IsolateDisposer.cpptest-app/runtime/src/main/cpp/JSONObjectHelper.cpptest-app/runtime/src/main/cpp/JSONObjectHelper.htest-app/runtime/src/main/cpp/MessageLoopTimer.cpptest-app/runtime/src/main/cpp/MessageLoopTimer.htest-app/runtime/src/main/cpp/ModuleInternal.cpptest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/V8GlobalHelpers.cpptest-app/runtime/src/main/cpp/WeakRef.cpptest-app/runtime/src/main/cpp/js/README.mdtest-app/runtime/src/main/cpp/js/blob-url.jstest-app/runtime/src/main/cpp/js/error-events.jstest-app/runtime/src/main/cpp/js/events.jstest-app/runtime/src/main/cpp/js/json-helper.jstest-app/runtime/src/main/cpp/js/message-loop-timer.jstest-app/runtime/src/main/cpp/js/require-factory.jstest-app/runtime/src/main/cpp/js/smart-stringify.jstest-app/runtime/src/main/cpp/js/weak-ref.jstools/js2c.mjs
💤 Files with no reviewable changes (1)
- test-app/runtime/src/main/cpp/MessageLoopTimer.h
| add_custom_command( | ||
| OUTPUT ${RUNTIME_BUILTINS_GENERATED_DIR}/RuntimeBuiltins.h | ||
| ${RUNTIME_BUILTINS_GENERATED_DIR}/RuntimeBuiltins.cpp | ||
| COMMAND ${NODE_EXECUTABLE} ${RUNTIME_BUILTINS_JS2C} | ||
| --out-dir ${RUNTIME_BUILTINS_GENERATED_DIR} | ||
| --check-dir ${RUNTIME_BUILTIN_JS_DIR} | ||
| ${RUNTIME_BUILTIN_JS} | ||
| DEPENDS ${RUNTIME_BUILTIN_JS} ${RUNTIME_BUILTINS_JS2C} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect directory drift before incremental builds.
--check-dir runs only after CMake schedules this custom command. Adding an unlisted .js file does not change an OUTPUT or a listed DEPENDS entry, so an incremental build can skip the command and silently omit the builtin.
Use a configure-time file(GLOB ... CONFIGURE_DEPENDS) check against RUNTIME_BUILTIN_JS, or add an always-run validation target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/CMakeLists.txt` around lines 84 - 91, Update the CMake
configuration around the custom command using RUNTIME_BUILTIN_JS so additions or
removals of builtin JavaScript files trigger reconfiguration or validation
before incremental builds. Prefer a configure-time file(GLOB ...
CONFIGURE_DEPENDS) check that compares the discovered files with
RUNTIME_BUILTIN_JS, while preserving the existing generation command and
dependency behavior.
| add_custom_command( | ||
| OUTPUT ${RUNTIME_BUILTINS_GENERATED_DIR}/RuntimeBuiltins.h | ||
| ${RUNTIME_BUILTINS_GENERATED_DIR}/RuntimeBuiltins.cpp | ||
| COMMAND ${NODE_EXECUTABLE} ${RUNTIME_BUILTINS_JS2C} | ||
| --out-dir ${RUNTIME_BUILTINS_GENERATED_DIR} | ||
| --check-dir ${RUNTIME_BUILTIN_JS_DIR} | ||
| ${RUNTIME_BUILTIN_JS} | ||
| DEPENDS ${RUNTIME_BUILTIN_JS} ${RUNTIME_BUILTINS_JS2C} | ||
| COMMENT "Generating RuntimeBuiltins from src/main/cpp/js" | ||
| VERBATIM | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'RuntimeBuiltins\.h|GetBuiltinSource' \
test-app/runtime/src/main/cpp/BuiltinLoader.cpp
rg -n -C 8 'add_custom_command|add_custom_target|add_dependencies|RuntimeBuiltins' \
test-app/runtime/CMakeLists.txtRepository: NativeScript/android
Length of output: 2688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CMakeLists relevant sections =="
sed -n '76,110p;200,212p' test-app/runtime/CMakeLists.txt
echo
echo "== BuiltinLoader includes and GetBuiltinSource body =="
sed -n '1,70p;90,130p' test-app/runtime/src/main/cpp/BuiltinLoader.cpp
echo
echo "== Library directive with source order context =="
sed -n '170,208p' test-app/runtime/CMakeLists.txtRepository: NativeScript/android
Length of output: 7860
🌐 Web query:
CMake add_custom_command OUTPUT multiple files attaches rule only to first output
💡 Result:
In CMake, the add_custom_command function technically supports multiple files listed under the OUTPUT keyword. However, historical and practical usage patterns indicate that relying on multiple outputs within a single add_custom_command can lead to inconsistent behavior in how build tools (like Make or Ninja) track dependencies [1]. While the documentation notes that you can list multiple outputs to indicate a command generates several files [2], build systems often struggle to track which specific output was generated or whether the rule needs to run if only one of the outputs is missing. In older versions of CMake, this caused specific issues where the build system treated the first output differently or failed to correctly track dependencies for subsequent outputs [1]. To ensure robust and reliable build behavior, the recommended approach is as follows: 1. Use add_custom_command for each unique output file individually, if the commands to generate them are different [2]. 2. If a single command generates multiple files simultaneously, define the primary output in the add_custom_command and use the BYPRODUCTS option (available in modern CMake) to list the additional files produced by that command. This informs the build system that these files are generated by the same rule [3]. 3. If you need to ensure that multiple outputs are generated together and are required by different, independent targets, wrap the generation command in an add_custom_target. This forces the generation to happen as a single atomic unit before any dependent targets attempt to use the files, preventing race conditions or incomplete builds [4][5][6]. By using add_custom_target to drive the generation, you decouple the execution of the command from the individual file dependencies, ensuring that all necessary outputs are created before any part of the build process relies on them [4][7].
Citations:
- 1: https://stackoverflow.com/questions/24416133/cmake-add-custom-command-issue-with-multiple-output-files
- 2: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Custom%20Commands.html
- 3: https://cmake.org/cmake/help/latest/command/add%5Fcustom%5Ftarget.html
- 4: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 5: https://cmake.org/cmake/help/v4.0/command/add_custom_command.html
- 6: https://cmake.org/cmake/help/v3.26/command/add_custom_command.html
- 7: https://cmake.org/cmake/help/v4.3/command/add_custom_command.html
🌐 Web query:
CMake add_custom_command add_library generated header multiple source files dependency header included in other source
💡 Result:
To manage a generated header file that is used by multiple source files or targets in CMake, you must ensure that the generation command runs before any consuming files are compiled [1][2]. The recommended approach depends on whether the generated header is internal to a single target or shared across multiple independent targets. ### Scenario 1: Internal to a Single Target If the generated header is used only by source files within the same library or executable, simply adding the generated file to the target's source list is often sufficient [3][2]. CMake's dependency scanner will typically recognize that the source files include the generated header and trigger the custom command automatically [4][2]. 1. Use add_custom_command with an OUTPUT parameter pointing to the full path of the generated header (usually in CMAKE_CURRENT_BINARY_DIR) [1][5]. 2. Add the generated file directly to the add_library or add_executable call (or use target_sources) [3][6]. 3. Ensure the directory containing the header is added to your target's include directories using target_include_directories [4][7]. ### Scenario 2: Shared Across Multiple Independent Targets If multiple independent targets need the same generated header, you must avoid race conditions where multiple targets attempt to run the same command in parallel [1]. 1. Wrap the generation command in an add_custom_target to act as a central "build step" [1][3]. cmake add_custom_command( OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/generated.h COMMAND my_generator -o ${CMAKE_CURRENT_BINARY_DIR}/generated.h DEPENDS input.in ) add_custom_target(generate_headers DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/generated.h) 2. Create an INTERFACE library to propagate the dependency and include directories to other targets [3][7]. cmake add_library(generated_header_lib INTERFACE) target_include_directories(generated_header_lib INTERFACE ${CMAKE_CURRENT_BINARY_DIR}) add_dependencies(generated_header_lib generate_headers) 3. Link this interface library to any target that needs the header [8][7]: cmake target_link_libraries(my_app PRIVATE generated_header_lib) By linking to the interface library, you transitively force the dependent targets to wait for the generation target (generate_headers) to complete before they begin their own build [3][9]. Using FILE_SET HEADERS (introduced in CMake 3.23) is the modern, preferred way to manage generated headers within these interface libraries [3][6].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/add_custom_command.html
- 2: https://cmake.org/cmake/help/latest/prop_sf/OBJECT_DEPENDS.html
- 3: https://cmake.org/cmake/help/latest/guide/tutorial/Custom%20Commands%20and%20Generated%20Files.html
- 4: https://stackoverflow.com/questions/13426934/cmake-dynamically-generated-header-dependencies
- 5: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Custom%20Commands.html
- 6: https://discourse.cmake.org/t/how-do-i-get-generated-files-to-be-built-first/13370
- 7: https://stackoverflow.com/questions/47322754/library-depend-on-a-header-file
- 8: https://discourse.cmake.org/t/add-custom-command-and-generated-header-dependencies/7393
- 9: https://cmake.org/cmake/help/latest/command/add%5Fdependencies.html
Make RuntimeBuiltins.h a target-level build dependency.
The current command attaches only to RuntimeBuiltins.cpp, while BuiltinLoader.cpp includes the generated header directly. A parallel build can compile that source before RuntimeBuiltins.h exists, causing NativeScript to fail on clean builds. Add a custom target for both generated files and attach it to NativeScript with add_dependencies().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/CMakeLists.txt` around lines 84 - 94, Update the generated
RuntimeBuiltins outputs around add_custom_command so both RuntimeBuiltins.h and
RuntimeBuiltins.cpp are produced by a named custom target, then attach that
target to NativeScript with add_dependencies(). Preserve the existing generation
command and ensure BuiltinLoader.cpp cannot compile until the generated header
target completes.
| Local<Object> binding = Object::New(isolate); | ||
| binding->Set(context, ArgConverter::ConvertToV8String(isolate, "globalTarget"), | ||
| runtime->GlobalEventTarget().Get(isolate)); | ||
| binding->Set(context, ArgConverter::ConvertToV8String(isolate, "nativeReportFatal"), | ||
| nativeReportFatal); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Discarded Maybe<bool> results when populating the binding bag. Both sites build the binding object with v8::Object::Set and ignore the returned Maybe<bool>, which V8 declares V8_WARN_UNUSED_RESULT. A failed Set produces a binding bag missing a native dependency, and the builtin then fails with an unrelated JavaScript error.
test-app/runtime/src/main/cpp/ErrorEvents.cpp#L54-L58: check bothSetcalls forglobalTargetandnativeReportFatal, and throwNativeScriptExceptionwhen either returns false or nothing.test-app/runtime/src/main/cpp/MessageLoopTimer.cpp#L28-L30: check bothSetcalls formessageLoopTimerStartandmessageLoopTimerStop, and fail initialization when either returns false or nothing.
🛡️ Proposed fix for `ErrorEvents.cpp`
Local<Object> binding = Object::New(isolate);
- binding->Set(context, ArgConverter::ConvertToV8String(isolate, "globalTarget"),
- runtime->GlobalEventTarget().Get(isolate));
- binding->Set(context, ArgConverter::ConvertToV8String(isolate, "nativeReportFatal"),
- nativeReportFatal);
+ if (!binding->Set(context, ArgConverter::ConvertToV8String(isolate, "globalTarget"),
+ runtime->GlobalEventTarget().Get(isolate))
+ .FromMaybe(false) ||
+ !binding->Set(context, ArgConverter::ConvertToV8String(isolate, "nativeReportFatal"),
+ nativeReportFatal)
+ .FromMaybe(false)) {
+ throw NativeScriptException("ErrorEvents::Init: failed to populate the binding bag");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Local<Object> binding = Object::New(isolate); | |
| binding->Set(context, ArgConverter::ConvertToV8String(isolate, "globalTarget"), | |
| runtime->GlobalEventTarget().Get(isolate)); | |
| binding->Set(context, ArgConverter::ConvertToV8String(isolate, "nativeReportFatal"), | |
| nativeReportFatal); | |
| Local<Object> binding = Object::New(isolate); | |
| if (!binding->Set(context, ArgConverter::ConvertToV8String(isolate, "globalTarget"), | |
| runtime->GlobalEventTarget().Get(isolate)) | |
| .FromMaybe(false) || | |
| !binding->Set(context, ArgConverter::ConvertToV8String(isolate, "nativeReportFatal"), | |
| nativeReportFatal) | |
| .FromMaybe(false)) { | |
| throw NativeScriptException("ErrorEvents::Init: failed to populate the binding bag"); | |
| } |
📍 Affects 2 files
test-app/runtime/src/main/cpp/ErrorEvents.cpp#L54-L58(this comment)test-app/runtime/src/main/cpp/MessageLoopTimer.cpp#L28-L30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/ErrorEvents.cpp` around lines 54 - 58, Check
the Maybe<bool> results from both binding->Set calls in
test-app/runtime/src/main/cpp/ErrorEvents.cpp lines 54-58 for globalTarget and
nativeReportFatal, and throw NativeScriptException when either result is empty
or false. Apply the same checked-result handling to both Set calls in
test-app/runtime/src/main/cpp/MessageLoopTimer.cpp lines 28-30 for
messageLoopTimerStart and messageLoopTimerStop, failing initialization when
either result is empty or false.
| Object.defineProperty(URL.prototype, 'searchParams', { | ||
| get() { | ||
| if (this._searchParams == null) { | ||
| this._searchParams = new URLSearchParams(this.search); | ||
| Object.defineProperty(this._searchParams, '_url', { | ||
| enumerable: false, | ||
| writable: false, | ||
| value: this, | ||
| }); | ||
| this._searchParams._append = this._searchParams.append; | ||
| this._searchParams.append = function (name, value) { | ||
| this._append(name, value); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._delete = this._searchParams.delete; | ||
| this._searchParams.delete = function (name) { | ||
| this._delete(name); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._set = this._searchParams.set; | ||
| this._searchParams.set = function (name, value) { | ||
| this._set(name, value); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._sort = this._searchParams.sort; | ||
| this._searchParams.sort = function () { | ||
| this._sort(); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| } | ||
| return this._searchParams; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the URL "search" setter implementation and check whether it invalidates _searchParams.
rg -n -C 5 'set search|search\s*=|_searchParams' --type=js -g '!node_modules'Repository: NativeScript/android
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching blob-url.js / URL implementation =="
fd -a 'blob-url\.js$|url|BlobUrl' . | sed 's#^\./##' | head -200
echo
echo "== target file with line numbers =="
cat -n test-app/runtime/src/main/cpp/js/blob-url.js
echo
echo "== focused URL search/url property searches =="
rg -n -C 4 'defineProperty|set (search|href|origin|protocol|password|username)|searchParams|_searchParams|URLSearchParams|set search' test-app/runtime/src/main/cpp test-app/runtime/src/main/cpp/js || trueRepository: NativeScript/android
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== URL implementation files =="
fd -a 'URL.*\.(h|cpp)$|Blob.*\.(h|cpp)$|URLSearchParams.*\.(h|cpp)$' test-app/runtime/src/main/cpp | sed 's#^\./##' | sort
echo
echo "== URLImpl header outline =="
ast-grep outline test-app/runtime/src/main/cpp/URLImpl.h --view compact || true
echo
echo "== URLImpl relevant search/url/params code =="
rg -n -C 6 "class URL|Struct.*URL|set search|URLSearchParams|searchParams|GetSearchParams|set_(search|href)|search" test-app/runtime/src/main/cpp/URLImpl.h test-app/runtime/src/main/cpp/URLImpl.cpp | head -400
echo
echo "== registration of blob-url.js and URL class files =="
rg -n -C 4 "blob-url|BlobURL|URL::Register|URLImpl|JSValueConstructor|defineProperty.*searchParams" test-app/runtime/src/main/cpp test-app/runtime/src/main/cpp/js | head -400Repository: NativeScript/android
Length of output: 34762
Invalidate _searchParams when search changes.
searchParams builds and caches a URLSearchParams once, and the written-back methods only update the original search value. When url.search or url.href is assigned directly, url.searchParams still returns the stale cached instance. Clear _searchParams in the search setter and any path that changes the query component.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/js/blob-url.js` around lines 27 - 59, Update
the URL search mutation paths associated with the searchParams getter so
_searchParams is cleared whenever the URL’s search value changes, including
direct search and href assignments. Preserve the existing write-back behavior
while ensuring subsequent searchParams access rebuilds from the current query
component.
| global.WebAssembly = new Proxy(WebAssembly, { | ||
| get: (target, name) => { | ||
| let origMethod = target[name]; | ||
| let proxyMethods = [ | ||
| "compile", | ||
| "compileStreaming", | ||
| "instantiate", | ||
| "instantiateStreaming" | ||
| ]; | ||
|
|
||
| if (proxyMethods.indexOf(name) < 0) { | ||
| return origMethod; | ||
| } | ||
|
|
||
| return function (...args) { | ||
| messageLoopTimerStart(); | ||
| let result = origMethod.apply(this, args); | ||
| return result.then(x => { | ||
| messageLoopTimerStop(); | ||
| return x; | ||
| }).catch(e => { | ||
| messageLoopTimerStop(); | ||
| throw e; | ||
| }); | ||
| }; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'message-loop-timer\.js$' . || true
echo "== git status/stat =="
git diff --stat || true
echo "== file context =="
if [ -f test-app/runtime/src/main/cpp/js/message-loop-timer.js ]; then
cat -n test-app/runtime/src/main/cpp/js/message-loop-timer.js
fi
echo "== related timer usages =="
rg -n "messageLoopTimer(Start|Stop)|WebAssembly" test-app/runtime/src/main/cpp/js -S || true
echo "== package/runtime hints =="
rg -n "V8|WebAssembly|NativeScript|Node.js|Runtime" package.json test-app -S --glob '!**/build/**' --glob '!**/dist/**' 2>/dev/null | head -200 || trueRepository: NativeScript/android
Length of output: 27779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Standalone probe for the proxy control-flow under synchronous vs asynchronous outcomes.
# It does not depend on repository code, dependencies, or executable files.
events = []
def messageLoopTimerStart():
events.append("start")
def messageLoopTimerStop():
msg = "stop"
events.append(msg)
return msg
proxy_method_names = ["compile", "compileStreaming", "instantiate", "instantiateStreaming"]
def probe(name, impl):
events.clear()
result = None
try:
handler = {
"get": lambda target, n: (impl if n in proxy_method_names else getattr(target, n, None))
}
proxy = type("Target", (), {})
global_object = {"WebAssembly": {}}
proxy["WebAssembly"] = {}
# recreate function proxy as in source
def proxy_getter(target, n):
if n in proxy_method_names:
pass
return target[n]
wrapped = lambda ...args: (messageLoopTimerStart(), (lambda result=None: (func(result)))(impl()))[-1] # placeholder
# explicit model from original:
if name == "sync_throw":
messageLoopTimerStart()
try:
result = impl()
except Exception as e:
events.append("sync_catch")
events.append(str(e))
def simulate_sync_throw(orig):
events = []
messageLoopTimerStart()
try:
result = orig()
except Exception as e:
events.append("sync_catch")
events.append(str(e))
events.append("then_chain_reached", ("result" in dir(result) if hasattr(result, "then") else result))
return events
def orig_sync_invalid():
raise TypeError("WebAssembly.compile: buffer argument required")
events = simulate_sync_invalid = simulate_sync_throw
print(simulate_sync_invalid(lambda: (_ for _ in ()).throw(TypeError("invalid"))) if hasattr((x for x in ()),"throw") else simulate_sync_throw(orig_sync_invalid))
PYRepository: NativeScript/android
Length of output: 355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
events = []
def messageLoopTimerStart():
events.append("start")
def messageLoopTimerStop():
events.append("stop")
def simulate_sync_throw():
events.clear()
messageLoopTimerStart()
try:
raise TypeError("buffer required")
except Exception:
pass
return events
def simulate_async_rejection():
events.clear()
messageLoopTimerStart()
result = Promise.reject(TypeError("async failed"))
# model .then/catch not awaiting here; relevant is synchronous flow outcome
return events
print("sync_throw_events =", simulate_sync_throw())
PYRepository: NativeScript/android
Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Timer C++ implementation =="
ast-grep outline test-app/runtime/src/main/cpp/Timers.cpp --match 'MessageLoopTimer' --view expanded || true
rg -n "MessageLoopTimer|messageLoopTimer(Start|Stop)|PumpMessageLoop|StartMessageLoop|StopMessageLoop" test-app/runtime/src/main/cpp test-app/runtime/src/main/java -S || true
echo "== Relevant Timers.cpp sections =="
sed -n '220,360p' test-app/runtime/src/main/cpp/Timers.cpp | cat -nRepository: NativeScript/android
Length of output: 9044
🌐 Web query:
V8 WebAssembly.compile WebAssembly.instantiate invalid BufferSource throws synchronously before returning promise
💡 Result:
Yes, WebAssembly.compile and WebAssembly.instantiate are specified and implemented to throw a TypeError synchronously if the provided bufferSource argument is not a valid type (such as a typed array or ArrayBuffer) [1][2][3][4][5]. This behavior ensures that invalid input types are caught immediately before the functions initiate their asynchronous compilation or instantiation processes [3]. If the input type is valid but the WebAssembly binary itself is malformed or invalid, the promise returned by these functions will reject asynchronously with a WebAssembly.CompileError, LinkError, or RuntimeError, rather than throwing synchronously [1][2][6]. Summary of behavior for WebAssembly.compile and WebAssembly.instantiate(bufferSource,...): - Invalid BufferSource type: Throws a TypeError synchronously [1][2][3]. - Valid BufferSource type but invalid Wasm binary: Returns a promise that rejects asynchronously [1][2][6]. Note: The synchronous behavior regarding the argument type is distinct from the asynchronous behavior regarding the content of the buffer, which follows the standard pattern of promises in the WebAssembly JavaScript API [3][4][5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/WebAssembly/Reference/JavaScript_interface/compile_static
- 2: https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface/instantiate
- 3: WebAssembly/design@0e68a86
- 4: https://www.w3.org/TR/2024/WD-wasm-js-api-2-20240822/
- 5: https://www.w3.org/TR/2026/CRD-wasm-js-api-2-20260309/
- 6: https://devdoc.net/web/developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/instantiate.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only behavioral probe of the JS control flow for synchronous throws.
# This mirrors the referenced lines from test-app/runtime/src/main/cpp/js/message-loop-timer.js.
events = []
messageLoopCount = 0
def messageLoopTimerStart():
global messageLoopCount
events.append("start")
messageLoopCount += 1
def messageLoopTimerStop():
global messageLoopCount
events.append("stop")
messageLoopCount -= 1
def build_proxy_orig(impl):
wrapped = lambda *args: messageLoopTimerStart() or impl(*args) # keep same order as source
wrapped.then = lambda *cb: cb[0](wrapped)
return wrapped
def simulate_sync_throw():
events.clear()
import json
wrapped = build_proxy_orig(lambda *a: json.JSONDecoder().raw_decode) # always throws synchronously
try:
wrapped().catch(None)
except Exception:
pass
return events, messageLoopCount
events, count = simulate_sync_throw()
print("events =", events)
print("messageLoopCount =", count)
print("stop_was_reached =", "stop" in events)
print("start_and_no_stop =", events == ["start"] and count == 1)
PYRepository: NativeScript/android
Length of output: 248
🌐 Web query:
WebAssembly compile instantiate WebIDL BufferSource coercion throws synchronous TypeError example
💡 Result:
When working with WebAssembly compilation and instantiation APIs—such as WebAssembly.instantiate, WebAssembly.compile, or the WebAssembly.Module constructor—providing an invalid BufferSource (e.g., passing a plain object, string, or number instead of a TypedArray or ArrayBuffer) will trigger a TypeError [1][2][3]. The distinction between synchronous and asynchronous behavior is crucial: 1. Asynchronous APIs (WebAssembly.instantiate and WebAssembly.compile): If the BufferSource argument is invalid, the returned Promise will reject with a TypeError [1][2]. This rejection happens because the WebIDL coercion process for the BufferSource parameter occurs as part of the initial method invocation, and failure to satisfy the required type definition causes the operation to reject synchronously (by returning a rejected promise) [1][4][5]. 2. Synchronous API (WebAssembly.Module constructor): If the provided argument is not a valid BufferSource, the constructor will throw a TypeError synchronously when it is invoked [3][6]. Example of a synchronous TypeError: // Attempting to instantiate with an invalid type (e.g., a string) try { const invalidBuffer = "not a buffer"; new WebAssembly.Module(invalidBuffer); } catch (e) { if (e instanceof TypeError) { console.error("Caught expected TypeError:", e.message); } } In this example, the WebIDL binding layer attempts to coerce the input into a BufferSource. Because a string cannot be coerced into a BufferSource according to the WebIDL specification, it immediately throws a TypeError [3]. Similarly, if you were to pass an invalid type to WebAssembly.instantiate(invalidBuffer), the function would return a promise that is rejected with a TypeError [1].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface/instantiate
- 2: https://developer.cdn.mozilla.net/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/compile
- 3: https://developer.mozilla.org/en-US/docs/WebAssembly/Reference/JavaScript_interface/Module/Module
- 4: https://www.w3.org/TR/2026/CRD-wasm-js-api-2-20260309/
- 5: https://www.w3.org/TR/2024/WD-wasm-js-api-2-20240216/
- 6: https://www.typeerror.org/docs/javascript/global_objects/webassembly/module/module
🌐 Web query:
Node.js V8 WebAssembly.compile invalid TypedArray synchronous TypeError thrown before promise
💡 Result:
In Node.js (and other environments using V8), WebAssembly.compile is designed to handle invalid input types (i.e., when the buffer source is not an ArrayBuffer or a TypedArray) by throwing a synchronous TypeError before returning a promise [1][2][3][4]. Historically, early specifications of the WebAssembly API proposed that the returned promise should reject with a TypeError [4]. However, the behavior was subsequently updated to throw synchronously if the input is not a valid BufferSource [4]. Technical Context: 1. Synchronous Validation: V8 implementation code (wasm-js.cc) performs validation on the first argument before initiating asynchronous compilation [1][2][3]. If the provided argument does not pass the check for an ArrayBuffer or TypedArray, the V8 error thrower is invoked synchronously [1][2][3]. 2. Promise Chain: Because this validation occurs during the setup phase of the WebAssembly.compile call, it triggers an exception before the function can return a Promise object or enter an asynchronous state [4]. 3. Debugging Tip: If you are seeing a TypeError in this context, ensure that the variable being passed to WebAssembly.compile is explicitly an instance of ArrayBuffer or a subclass of TypedArray (e.g., Uint8Array, Int32Array) at the moment of the call [5][6]. If you are passing data from an asynchronous source (like a fetch request or a stream), verify that the data has been fully buffered and correctly typed before invoking the compile method [7][8]. If you are experiencing unexpected behavior, it may be helpful to wrap the call in a try-catch block to handle the synchronous error, rather than relying solely on the promise catch method [9].
Citations:
- 1: https://github.com/nodejs/node/blob/159ae48f/deps/v8/src/wasm/wasm-js.cc
- 2: https://chromium.googlesource.com/v8/v8/+/414c594db396360b293267226cbaf4d1472cfd3e/src/wasm/wasm-js.cc
- 3: https://chromium.googlesource.com/v8/v8/+/d1a2d787cba1b38fabd802767bc3f643e65aff2f/src/wasm/wasm-js.cc
- 4: WebAssembly/design@0e68a86
- 5: https://developer.mozilla.org/en-US/docs/WebAssembly/Reference/JavaScript_interface/compile_static
- 6: https://developer.cdn.mozilla.net/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/compile
- 7: wasm: streaming APIs accept non-Uint8Array response body chunks nodejs/node#64669
- 8: node v18.6.0. - TypeError: The "source" argument must be an instance of Response or an Promise resolving to Response. Received an instance of Response nodejs/node#43925
- 9: https://beta.docs.nodejs.org/errors.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MessageLoopTimer C++ =="
cat -n test-app/runtime/src/main/cpp/MessageLoopTimer.cpp
cat -n test-app/runtime/src/main/cpp/MessageLoopTimer.hRepository: NativeScript/android
Length of output: 4820
Guard against synchronous failures from wrapped WebAssembly calls.
messageLoopTimerStart() runs before origMethod.apply(this, args). If the wrapped WebAssembly.compile, compileStreaming, instantiate, or instantiateStreaming call throws synchronously during argument validation, the promise chain never runs and messageLoopTimerStop() is skipped. The timer then leaves m_isRunning true and can keep the worker thread working unnecessarily. Wrap the call in try/catch, call messageLoopTimerStop() in both synchronous and async error paths, then rethrow the error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/js/message-loop-timer.js` around lines 9 - 35,
Update the wrapped WebAssembly methods in the Proxy get handler to catch
synchronous exceptions from origMethod.apply after messageLoopTimerStart(), call
messageLoopTimerStop(), and rethrow; preserve the existing timer cleanup for
both resolved and rejected asynchronous results.
| #include "BuiltinLoader.h" | ||
| #include "robin_hood.h" | ||
| #include <sstream> | ||
| #include <string> | ||
|
|
||
| using namespace v8; | ||
| using namespace tns; | ||
|
|
||
| static robin_hood::unordered_map<Isolate*, Persistent<Function>*> isolateToSerializeFunc; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find isolate-keyed static maps and check for accompanying mutex usage.
rg -n --type=cpp -B2 -A15 'unordered_map<\s*(v8::)?Isolate\s*\*' test-app/runtime/src/main/cpp | rg -n 'mutex|lock_guard|unordered_map<'Repository: NativeScript/android
Length of output: 3499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JSONObjectHelper relevant sections =="
sed -n '1,130p' test-app/runtime/src/main/cpp/JSONObjectHelper.cpp
echo
echo "== IsolateDisposer relevant sections =="
sed -n '1,130p' test-app/runtime/src/main/cpp/IsolateDisposer.h
sed -n '1,140p' test-app/runtime/src/main/cpp/IsolateDisposer.cpp
echo
echo "== JSONObjectHelper usages including onDisposeIsolate/RegisterFromFunction/GetSerializeFunc =="
rg -n "JSONObjectHelper|RegisterFromFunction|GetSerializeFunc|onDisposeIsolate|isolateToSerializeFunc" test-app/runtime/src/main/cpp
echo
echo "== worker isolate/thread references =="
rg -n --type=cpp -i "worker|workers|new.*thread|thread|IsolateData|CreateWorker|ThreadLocal|CurrentThread|v8::current|dispose.*isolate|onDispose" test-app/runtime/src/main/cpp | sed -n '1,220p'Repository: NativeScript/android
Length of output: 36318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JSONObjectHelper call sites in MetadataNode.cpp =="
rg -n -B5 -A8 "JSONObjectHelper::RegisterFromFunction|RegisterFromFunction\\(" test-app/runtime/src/main/cpp/MetadataNode.cpp
echo
echo "== MetadataNode dispose implementation =="
sed -n '2240,2270p' test-app/runtime/src/main/cpp/MetadataNode.cpp
echo
echo "== RegisterFromFunction call sites =="
rg -n -B8 -A8 "RegisterFromFunction\\(" test-app/runtime/src/main/cpp
echo
echo "== lock_guard/isolate usage in V8GlobalHelpers and ArgConverter =="
sed -n '1,150p' test-app/runtime/src/main/cpp/V8GlobalHelpers.cpp
sed -n '200,255p' test-app/runtime/src/main/cpp/ArgConverter.cppRepository: NativeScript/android
Length of output: 11283
🌐 Web query:
robin_hood unordered_map thread safety concurrent read insert erase documentation
💡 Result:
The popular C++ libraries martinus/robin-hood-hashing [1] and tsl/robin-map [2] are not thread-safe. Like the standard C++ container std::unordered_map [3], these implementations follow the same thread-safety guarantees: 1. Multiple threads can concurrently read (e.g., call const member functions like find) the same container instance [3][2]. 2. Any operation that modifies the container (e.g., insert, erase, or non-const find operations that might trigger a rehash) requires external synchronization (such as a std::mutex or std::shared_mutex) [3][2]. Concurrent read and write operations, or multiple concurrent write operations, on the same container instance without external synchronization result in undefined behavior [3]. While there are research-oriented implementations of "Concurrent Robin Hood Hashing" that utilize techniques like K-CAS (Key-Compare-And-Swap) to support concurrent operations without standard locking [4][5][6], these are distinct from the widely used martinus/robin-hood-hashing and tsl/robin-map libraries. If you require thread-safe concurrent access for production code, you must either use an external synchronization mechanism [3] or a purpose-built concurrent hash map data structure designed for such operations [5][7].
Citations:
- 1: https://github.com/martinus/robin-hood-hashing?tab=readme-ov-file
- 2: https://github.com/Tessil/robin-map/
- 3: https://stackoverflow.com/questions/9685486/unordered-map-thread-safety
- 4: https://drops.dagstuhl.de/storage/00lipics/lipics-vol125-opodis2018/LIPIcs.OPODIS.2018.10/LIPIcs.OPODIS.2018.10.pdf
- 5: https://github.com/asefahmed56/concurrent-robin-map
- 6: https://ar5iv.labs.arxiv.org/html/1809.04339
- 7: https://github.com/DaKellyFella/concurrent-robin-hood-hashing
Guard isolateToSerializeFunc with a mutex.
JSONObjectHelper::GetSerializeFunc() reads and inserts into the process-wide robin_hood::unordered_map, while JSONObjectHelper::onDisposeIsolate() erases from it. Workers run on their own isolate and thread, so a cached-serializer lookup while another isolate is disposed can race and corrupt/destroy the same map. Add and use a std::mutex for all accesses; also include <mutex>.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/JSONObjectHelper.cpp` around lines 4 - 13,
Protect the process-wide isolateToSerializeFunc map with a std::mutex, including
the existing read/insert logic in JSONObjectHelper::GetSerializeFunc() and erase
logic in JSONObjectHelper::onDisposeIsolate(). Add the mutex header and lock
around every map access, ensuring lookups and disposal cannot race.
| auto success = BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result); | ||
|
|
||
| assert(!result.IsEmpty() && result->IsFunction()); | ||
| assert(success && result->IsFunction()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
assert is the only check on RunBuiltin failures at three bootstrap sites. NDEBUG builds remove every one of these asserts, so a failed builtin load continues silently. Events::Init and ErrorEvents::Init already throw NativeScriptException for the same failure; align these three sites with that behavior.
test-app/runtime/src/main/cpp/ModuleInternal.cpp#L89-L91: replace the assert with an explicit check onToLocalandresult->IsFunction(), and throw before line 93 usesresult.As<Function>(). This site is the most severe: in release builds it dereferences an emptyLocal.test-app/runtime/src/main/cpp/MessageLoopTimer.cpp#L32-L33: replace the assert with anIsEmpty()check that throwsNativeScriptException.test-app/runtime/src/main/cpp/WeakRef.cpp#L17-L18: replace the assert with anIsEmpty()check that throwsNativeScriptException.
🛡️ Proposed fix for `ModuleInternal.cpp`
Local<Value> result;
- auto success = BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result);
-
- assert(success && result->IsFunction());
+ if (!BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result) ||
+ !result->IsFunction()) {
+ throw NativeScriptException("ModuleInternal::Init: the require-factory builtin did not return a function");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto success = BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result); | |
| assert(!result.IsEmpty() && result->IsFunction()); | |
| assert(success && result->IsFunction()); | |
| Local<Value> result; | |
| if (!BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result) || | |
| !result->IsFunction()) { | |
| throw NativeScriptException("ModuleInternal::Init: the require-factory builtin did not return a function"); | |
| } |
📍 Affects 3 files
test-app/runtime/src/main/cpp/ModuleInternal.cpp#L89-L91(this comment)test-app/runtime/src/main/cpp/MessageLoopTimer.cpp#L32-L33test-app/runtime/src/main/cpp/WeakRef.cpp#L17-L18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/ModuleInternal.cpp` around lines 89 - 91,
Replace the assert-only builtin-load checks with explicit NativeScriptException
failure handling at all three sites: in
test-app/runtime/src/main/cpp/ModuleInternal.cpp lines 89-91, validate both
RunBuiltin().ToLocal(&result) and result->IsFunction() before
result.As<Function>() is used; in
test-app/runtime/src/main/cpp/MessageLoopTimer.cpp lines 32-33 and
test-app/runtime/src/main/cpp/WeakRef.cpp lines 17-18, check the resulting local
for emptiness and throw NativeScriptException on failure. Preserve successful
bootstrap behavior.
|
|
||
| v8::Local<v8::Value> out; | ||
| script->Run(context).ToLocal(&out); | ||
| BuiltinLoader::RunBuiltin(context, BuiltinId::kBlobUrl); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not discard the kBlobUrl loader result.
This call ignores the returned MaybeLocal. If the builtin fails to compile or throws, a pending exception remains on the isolate and bootstrap continues into Events::Init, ErrorEvents::Init and m_module.Init. Blob URL support is then missing without any diagnostic. Every other call site in this PR checks the result.
🛡️ Proposed fix
- BuiltinLoader::RunBuiltin(context, BuiltinId::kBlobUrl);
+ if (BuiltinLoader::RunBuiltin(context, BuiltinId::kBlobUrl).IsEmpty()) {
+ throw NativeScriptException(
+ "Runtime::PrepareV8Runtime: the blob-url builtin failed to run");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BuiltinLoader::RunBuiltin(context, BuiltinId::kBlobUrl); | |
| if (BuiltinLoader::RunBuiltin(context, BuiltinId::kBlobUrl).IsEmpty()) { | |
| throw NativeScriptException( | |
| "Runtime::PrepareV8Runtime: the blob-url builtin failed to run"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/Runtime.cpp` at line 847, Handle the MaybeLocal
result returned by BuiltinLoader::RunBuiltin for BuiltinId::kBlobUrl, matching
the error-checking pattern used by the other builtin loads. Ensure compilation
or execution failure is detected and reported before bootstrap continues to
Events::Init, ErrorEvents::Init, or m_module.Init, rather than leaving a pending
isolate exception.
Description
Android mirror of NativeScript/ios#411. Based on
main(the V8 14.9 upgrade, #1987, has merged).Moves the runtime JavaScript that was embedded as C++ string literals across eight files into real, version-controlled
.jsfiles undertest-app/runtime/src/main/cpp/js/, compiled into the runtime at build time (Node-style js2c).Extraction & build
tools/js2c.mjs(taken from the iOS runtime, including its laterunsigned charfix for source bytes ≥ 0x80) convertsjs/*.jsinto a generatedRuntimeBuiltins.{h,cpp}table — deterministic output, gitignored generated dir.add_custom_command(node is already a build requirement — the static-binding-generator shells out to it). TheRUNTIME_BUILTIN_JSlist is explicit;--check-dirfails the build loudly if it drifts from the directory contents. Incremental: a no-op rebuild skips codegen, touching a.jsreruns it.Extracted builtins:
weak-ref,message-loop-timer,smart-stringify,require-factory,json-helper,events,error-events,blob-url.Loader
ScriptCompiler::CompileFunctionwith the fixed parametersexports,moduleandbinding— natives arrive as properties of a bag object built by the C++ call site (Node's internalBinding idiom), and results come back throughmodule.exports.BuiltinLoader::RunBuiltincompiles with a properinternal/<name>.jsscript origin (runtime frames are identifiable in stack traces) and a process-wide bytecode cache: the first compile in the process useskEagerCompile+CreateCodeCacheForFunction, later isolates consume viakConsumeCodeCache(with rejected-cache fallback). The cache is mutex-guarded — worker runtimes initialize on their own threads.eslint.config.mjs) declaresexports,module,bindingand the reachable native globals;no-undefis the typo net. Conventions are documented intest-app/runtime/src/main/cpp/js/README.md.Behavior notes
internal/<name>.jsorigins instead of anonymous frames.JSONObjectHelperrecompiled its JS→org.jsonserializer on every registration; it is now compiled once per isolate and released via the isolate-dispose hook (the Android analog of the iOS PR's recompile-per-use perf fixes).__messageLoopTimerStart/__messageLoopTimerStopare no longer installed on the global object — nothing outsideMessageLoopTimerreferenced them; the builtin receives the pair throughbinding.Related Pull Requests
Does your pull request have unit tests?
Covered by the full existing device suite: 605 specs, 0 failures on an emulator (all extracted paths — events, error events, URL/Blob, WeakRef, module require, console stringify, workers — are exercised by existing specs; workers exercise the cross-isolate bytecode cache).
Summary by CodeRabbit
New Features
Documentation