Skip to content

refactor: move embedded runtime JS to real .js files (js2c) - #1989

Open
edusperoni wants to merge 1 commit into
mainfrom
feat/js-builtins
Open

refactor: move embedded runtime JS to real .js files (js2c)#1989
edusperoni wants to merge 1 commit into
mainfrom
feat/js-builtins

Conversation

@edusperoni

@edusperoni edusperoni commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

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 .js files under test-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 later unsigned char fix for source bytes ≥ 0x80) converts js/*.js into a generated RuntimeBuiltins.{h,cpp} table — deterministic output, gitignored generated dir.
  • Codegen runs from CMake via add_custom_command (node is already a build requirement — the static-binding-generator shells out to it). The RUNTIME_BUILTIN_JS list is explicit; --check-dir fails the build loudly if it drifts from the directory contents. Incremental: a no-op rebuild skips codegen, touching a .js reruns it.
  • Extraction fidelity was machine-verified: every extracted file was proven AST-identical to the original embedded literal by byte-comparing esbuild-minified output of both (whitespace+syntax minify only).

Extracted builtins: weak-ref, message-loop-timer, smart-stringify, require-factory, json-helper, events, error-events, blob-url.

Loader

  • Builtins follow Node's module wrapper: each file is compiled as a function body via ScriptCompiler::CompileFunction with the fixed parameters exports, module and binding — natives arrive as properties of a bag object built by the C++ call site (Node's internalBinding idiom), and results come back through module.exports.
  • BuiltinLoader::RunBuiltin compiles with a proper internal/<name>.js script origin (runtime frames are identifiable in stack traces) and a process-wide bytecode cache: the first compile in the process uses kEagerCompile + CreateCodeCacheForFunction, later isolates consume via kConsumeCodeCache (with rejected-cache fallback). The cache is mutex-guarded — worker runtimes initialize on their own threads.
  • ESLint (eslint.config.mjs) declares exports, module, binding and the reachable native globals; no-undef is the typo net. Conventions are documented in test-app/runtime/src/main/cpp/js/README.md.

Behavior notes

  • Stack traces from runtime internals now show internal/<name>.js origins instead of anonymous frames.
  • JSONObjectHelper recompiled its JS→org.json serializer 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/__messageLoopTimerStop are no longer installed on the global object — nothing outside MessageLoopTimer referenced them; the builtin receives the pair through binding.

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

    • Added Event and EventTarget APIs, including listener management, propagation controls, and cancellation.
    • Added error and promise-rejection events with global error reporting.
    • Added Blob URL creation, revocation, data access, and synchronized URL search parameters.
    • Added JSON serialization, circular-reference-safe stringification, and enhanced WeakRef compatibility.
    • Improved WebAssembly timer handling and module loading behavior.
  • Documentation

    • Added guidance for runtime built-ins and supported usage patterns.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds runtime JavaScript builtins, a Node.js-to-C++ generator, V8 code caching, and a BuiltinLoader. Runtime initialization paths now load embedded builtins instead of compiling inline JavaScript. JSON serialization uses isolate-scoped cached functions.

Changes

Runtime builtin pipeline

Layer / File(s) Summary
Builtin source implementations
test-app/runtime/src/main/cpp/js/*, eslint.config.mjs, package.json
Adds runtime builtins for events, error events, Blob URLs, JSON serialization, module loading, timers, stringification, and WeakRef behavior. Adds ESLint configuration and the lint script.
Builtin generation and build wiring
tools/js2c.mjs, test-app/runtime/CMakeLists.txt, .gitignore
Adds validated JavaScript-to-C++ generation, directory drift checks, atomic output replacement, and CMake integration for generated sources.
Builtin compilation and execution
test-app/runtime/src/main/cpp/BuiltinLoader.h, test-app/runtime/src/main/cpp/BuiltinLoader.cpp
Adds CommonJS-style builtin execution with exports, module, and binding parameters. Adds process-wide V8 code caching.
Runtime bootstrap integration
test-app/runtime/src/main/cpp/{Events,ErrorEvents,MessageLoopTimer,ModuleInternal,Runtime,V8GlobalHelpers,WeakRef}.*
Replaces inline JavaScript bootstrap compilation with BuiltinLoader calls for runtime builtins.
Isolate-scoped JSON serializer cache
test-app/runtime/src/main/cpp/JSONObjectHelper.*, test-app/runtime/src/main/cpp/IsolateDisposer.cpp
Caches the JSON serializer per isolate and removes its persistent handle during isolate disposal.

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
Loading

Possibly related PRs

Suggested reviewers: nathanwalker

Poem

A rabbit packs scripts in a C++ den,
V8 finds cached code again.
Events hop, timers run,
JSON rests when isolates are done.
Builtins bloom from source to pen.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: moving embedded runtime JavaScript into version-controlled files and adding the js2c generation flow.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Base automatically changed from feat/v8-14 to main August 3, 2026 15:27
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (4)
test-app/runtime/src/main/cpp/BuiltinLoader.cpp (2)

32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer context->GetIsolate() over Isolate::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 win

Guard the code-cache compile attempt with a TryCatch.

A rejected cache does not fail the compile; V8 recompiles internally and sets rejected. A failed CompileFunction therefore signals a real error and schedules an exception on the isolate. The code then falls through and calls CompileFunction a second time while that exception is still pending. Wrap the first attempt in a v8::TryCatch so 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

assert removes the failure check in release builds.

NDEBUG builds drop the assert. A failed builtin load then continues silently, and the message-loop timer never installs its setTimeout/setInterval behavior. Events::Init and ErrorEvents::Init throw NativeScriptException on 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 value

Consider deriving the isolate from context and reporting builtin-compile failures.

GetSerializeFunc takes Local<Context> context but obtains the isolate via Isolate::GetCurrent() instead of context->GetIsolate(). This works today because RegisterFromFunction always enters an Isolate::Scope before calling this method, but it is not self-evident from the signature and creates an implicit dependency on caller-established TLS state.

Separately, BuiltinLoader::RunBuiltin is called without a TryCatch. If json-helper.js fails to compile or execute, the function safely returns nullptr, but any underlying JS exception is not captured or logged here, unlike ConvertCallbackStatic, which wraps its call in a TryCatch for diagnostics. Confirm whether an outer TryCatch in 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

📥 Commits

Reviewing files that changed from the base of the PR and between c08a91b and 05dc54b.

📒 Files selected for processing (28)
  • .gitignore
  • eslint.config.mjs
  • package.json
  • test-app/runtime/CMakeLists.txt
  • test-app/runtime/src/main/cpp/BuiltinLoader.cpp
  • test-app/runtime/src/main/cpp/BuiltinLoader.h
  • test-app/runtime/src/main/cpp/ErrorEvents.cpp
  • test-app/runtime/src/main/cpp/ErrorEvents.h
  • test-app/runtime/src/main/cpp/Events.cpp
  • test-app/runtime/src/main/cpp/IsolateDisposer.cpp
  • test-app/runtime/src/main/cpp/JSONObjectHelper.cpp
  • test-app/runtime/src/main/cpp/JSONObjectHelper.h
  • test-app/runtime/src/main/cpp/MessageLoopTimer.cpp
  • test-app/runtime/src/main/cpp/MessageLoopTimer.h
  • test-app/runtime/src/main/cpp/ModuleInternal.cpp
  • test-app/runtime/src/main/cpp/Runtime.cpp
  • test-app/runtime/src/main/cpp/V8GlobalHelpers.cpp
  • test-app/runtime/src/main/cpp/WeakRef.cpp
  • test-app/runtime/src/main/cpp/js/README.md
  • test-app/runtime/src/main/cpp/js/blob-url.js
  • test-app/runtime/src/main/cpp/js/error-events.js
  • test-app/runtime/src/main/cpp/js/events.js
  • test-app/runtime/src/main/cpp/js/json-helper.js
  • test-app/runtime/src/main/cpp/js/message-loop-timer.js
  • test-app/runtime/src/main/cpp/js/require-factory.js
  • test-app/runtime/src/main/cpp/js/smart-stringify.js
  • test-app/runtime/src/main/cpp/js/weak-ref.js
  • tools/js2c.mjs
💤 Files with no reviewable changes (1)
  • test-app/runtime/src/main/cpp/MessageLoopTimer.h

Comment on lines +84 to +91
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}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment on lines +84 to +94
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
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.txt

Repository: 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.txt

Repository: 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:


🌐 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:


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.

Comment on lines +54 to +58
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 both Set calls for globalTarget and nativeReportFatal, and throw NativeScriptException when either returns false or nothing.
  • test-app/runtime/src/main/cpp/MessageLoopTimer.cpp#L28-L30: check both Set calls for messageLoopTimerStart and messageLoopTimerStop, 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.

Suggested change
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.

Comment on lines +27 to +59
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;
},
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 || true

Repository: 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 -400

Repository: 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.

Comment on lines +9 to +35
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;
});
};
}
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 || true

Repository: 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))
PY

Repository: 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())
PY

Repository: 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 -n

Repository: 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:


🏁 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)
PY

Repository: 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:


🌐 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:


🏁 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.h

Repository: 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.

Comment on lines +4 to +13
#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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.cpp

Repository: 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:


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.

Comment on lines +89 to +91
auto success = BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result);

assert(!result.IsEmpty() && result->IsFunction());
assert(success && result->IsFunction());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 on ToLocal and result->IsFunction(), and throw before line 93 uses result.As<Function>(). This site is the most severe: in release builds it dereferences an empty Local.
  • test-app/runtime/src/main/cpp/MessageLoopTimer.cpp#L32-L33: replace the assert with an IsEmpty() check that throws NativeScriptException.
  • test-app/runtime/src/main/cpp/WeakRef.cpp#L17-L18: replace the assert with an IsEmpty() check that throws NativeScriptException.
🛡️ 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.

Suggested change
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-L33
  • test-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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant