feat: ESM resolver hardening, http loader, dev-mode globals - #1965
Open
NathanWalker wants to merge 7 commits into
Open
feat: ESM resolver hardening, http loader, dev-mode globals#1965NathanWalker wants to merge 7 commits into
NathanWalker wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThe runtime replaces deleted DevFlags and HMR support with an HTTP loader. ESM resolution now supports canonical URLs, import maps, synthetic modules, asynchronous graphs, and stronger error handling. New builtin APIs expose loader and logging controls. Runtime workers and test tooling also receive updates. ChangesHTTP module loading and ESM execution
Runtime integration
Builtin APIs and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🟠 High · up to This PR substantially changes Android ESM loading and development runtime behavior, but unresolved failure paths can hang the JavaScript thread, race on loader configuration, leak or exhaust fetch-thread resources, break nested-class loading, and accept invalid test results. These concrete runtime and verification risks make the current head unsafe to merge until addressed or explicitly accepted. 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 |
NathanWalker
force-pushed
the
feat/hmr-dev-sessions
branch
2 times, most recently
from
June 16, 2026 17:07
fb386eb to
708fecd
Compare
NathanWalker
changed the title
NathanWalker
force-pushed
the
feat/hmr-dev-sessions
branch
from
July 3, 2026 23:54
08e7b8e to
696d4fc
Compare
NathanWalker
marked this pull request as ready for review
July 4, 2026 00:33
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
test-app/app/src/main/assets/app/tests/testNodeBuiltinsAndOptionalModules.mjs (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoose OR assertion weakens the test.
expect(p === "/foo/bar.txt" || p === "foo/bar.txt").toBe(true)accepts two different behaviors, which means a regression that flips the leading-slash handling would go undetected either way. If the exact expected value on Android is known, pin it directly instead of accepting both.🤖 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/app/src/main/assets/app/tests/testNodeBuiltinsAndOptionalModules.mjs` around lines 20 - 21, The assertion in testNodeBuiltinsAndOptionalModules.mjs is too permissive because it accepts both leading-slash and no-leading-slash results from mod.fileURLToPath. Update the test around the fileURLToPath check to assert the exact expected Android value directly, using the same mod.fileURLToPath symbol and expect call, so the test fails if the path handling changes unexpectedly.
🤖 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 `@README.md`:
- Around line 80-92: The runtime cache path description is incomplete: the dex
filename pattern in the README should match DexFactory.getDexFile. Update the
documentation around ClassResolver and DexFactory to state that the generated
dex is written with the thumb suffix (class name plus dex thumb) rather than
just <name>.dex, so the troubleshooting guidance reflects the actual on-disk
path.
In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`:
- Around line 1348-1361: Wrap CallbackHandlers::TerminateAllWorkersCallback in
the same V8 exception handling pattern used by the neighboring worker callbacks
so any exception from WorkerWrapper::TerminateChildren or child->Terminate() is
converted to NativeScriptException instead of escaping across V8. Locate the fix
in TerminateAllWorkersCallback and apply the same try/catch boundary and
rethrow/forwarding behavior already used in the adjacent callback handlers that
call into WorkerWrapper.
In `@test-app/runtime/src/main/cpp/HMRSupport.cpp`:
- Around line 1249-1251: configureRuntime() is leaving stale resolver state
behind because SetImportMapEntries() and SetVolatilePatterns() are only called
when the parsed lists are non-empty. Update the logic in configureRuntime() so
an explicit empty import map or volatile pattern list still invokes the
համապատասխան setter and replaces any previous session values. Keep the existing
parsing helpers like ReadImportMapEntries() and ReadVolatilePatterns(), but
remove the empty-check gate before SetImportMapEntries() and
SetVolatilePatterns() so cleared runtime config truly resets resolver state.
- Around line 1044-1063: The detached prefetch worker in
HMRSupport::KickstartHmrPrefetchUrlsSync can still update g_prefetchCache after
the request has timed out or global HMR state has been cleaned up. Add a
cancellation/liveness check tied to the current prefetch context (for example in
the ctxCopy worker path before writing to the cache) so stale workers exit
without mutating shared state. Apply the same guard to the matching detached
fetch path referenced by the related block, and keep the cache write under
g_prefetchMutex only when the context is still valid.
- Around line 664-668: The per-fetch URL entry trace in HMRSupport’s HTTP-ESM
fetch path is still guarded by the script-loading flag instead of the new
httpFetchUrlLog setting. Update the conditional around the DEBUG_WRITE in the
fetch entry flow to use the httpFetchUrlLog-backed check (for example, the
getter or helper associated with httpFetchUrlLog) so enabling that setting alone
turns on the URL trace. Keep the existing fetch entry logging in the same
location, just swap the gate used by the HTTP fetch diagnostics path.
- Around line 580-586: `g_prefetchCache` is using raw URLs instead of the same
canonical identity used by `MarkUrlsForCacheBust()`, so equivalent URLs can miss
cache hits or leave stale prefetched bodies behind. Update the prefetch cache
read/write/eviction paths in `HMRSupport.cpp` to normalize URLs before using
them as keys, and make the affected prewarm and invalidation flows use the same
canonicalized key consistently. Use the existing `MarkUrlsForCacheBust()` logic
as the reference for canonicalization and apply it wherever `g_prefetchCache` is
accessed.
In `@test-app/runtime/src/main/cpp/MetadataNode.cpp`:
- Around line 1798-1816: The module-name normalization in
MetadataNode::GetModulePath should strip any query string or fragment before
checking for .mjs/.js suffixes, since cache-busted URLs can bypass the current
extension trimming. Update the logic around the normalized/fullPathToFile
handling to remove everything after ? or # first, then keep sanitizing all
non-identifier characters (including ?, =, &, #) before the Util::SplitString
step.
In `@test-app/runtime/src/main/cpp/ModuleInternal.cpp`:
- Around line 292-321: The promise-drain logic in ModuleInternal.cpp currently
exits successfully when evalResult remains kPending after the maxAttempts loop.
Update the HTTP module evaluation path in the promise handling block to detect
the still-pending state after the loop and throw a timeout/pending-evaluation
NativeScriptException instead of falling through. Keep the existing
rejected-path behavior intact and make the new error message clearly identify
the module path and that evaluation never completed.
In `@test-app/runtime/src/main/cpp/Runtime.cpp`:
- Around line 101-113: The fatal signal handler in Runtime.cpp currently
heap-allocates via abi::__cxa_demangle and frees the result, which makes the
crash path depend on the allocator. Update the backtrace formatting logic around
the symbol lookup to avoid any allocation in this handler: keep info.dli_sname
unchanged for logging, remove the demangling/freeing work from this path, and
move demangling to an offline or non-signal-handling context if needed. Use the
existing backtrace loop and __android_log_print call site as the place to
preserve safe, allocator-free logging.
In `@test-app/runtime/src/main/cpp/URLImpl.cpp`:
- Around line 55-86: The URL.searchParams getter caches a URLSearchParams
instance, but the SetSearch path does not refresh that cached object when
url.search is reassigned, so it can become stale. Update the URLImpl URL/search
handling so the existing _searchParams object is synchronized with the new
search string in SetSearch instead of replacing or leaving it unchanged, and
keep the URLSearchParams methods on the cached instance consistent with the
updated URL.
In `@test-app/runtime/src/main/cpp/Version.h`:
- Around line 1-2: The checked-in fallback for the runtime commit SHA in
Version.h is still the placeholder string, so startup logs can show a bogus
value. Update the Version.h literal or make test-app/runtime/build.gradle
replace the exact symbol used by NATIVE_SCRIPT_RUNTIME_COMMIT_SHA so packaged
release builds include the real git SHA instead of the fallback.
---
Nitpick comments:
In
`@test-app/app/src/main/assets/app/tests/testNodeBuiltinsAndOptionalModules.mjs`:
- Around line 20-21: The assertion in testNodeBuiltinsAndOptionalModules.mjs is
too permissive because it accepts both leading-slash and no-leading-slash
results from mod.fileURLToPath. Update the test around the fileURLToPath check
to assert the exact expected Android value directly, using the same
mod.fileURLToPath symbol and expect call, so the test fails if the path handling
changes unexpectedly.
🪄 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
Run ID: f6782789-325f-4eaa-9610-9964979b18d6
📒 Files selected for processing (26)
README.mdtest-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/esm/meta-no-hot.mjstest-app/app/src/main/assets/app/tests/testHttpCanonicalKey.mjstest-app/app/src/main/assets/app/tests/testNodeBuiltinsAndOptionalModules.mjstest-app/app/src/main/assets/app/tests/testNsDevBoundary.mjstest-app/runtime/src/main/cpp/CallbackHandlers.cpptest-app/runtime/src/main/cpp/CallbackHandlers.htest-app/runtime/src/main/cpp/DevFlags.cpptest-app/runtime/src/main/cpp/DevFlags.htest-app/runtime/src/main/cpp/HMRSupport.cpptest-app/runtime/src/main/cpp/HMRSupport.htest-app/runtime/src/main/cpp/MetadataNode.cpptest-app/runtime/src/main/cpp/ModuleInternal.cpptest-app/runtime/src/main/cpp/ModuleInternalCallbacks.cpptest-app/runtime/src/main/cpp/ModuleInternalCallbacks.htest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/URLImpl.cpptest-app/runtime/src/main/cpp/URLImpl.htest-app/runtime/src/main/cpp/Version.htest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/cpp/WorkerWrapper.htest-app/runtime/src/main/java/com/tns/AppConfig.javatest-app/runtime/src/main/java/com/tns/ClassResolver.javatest-app/runtime/src/main/java/com/tns/DexFactory.javatest-app/runtime/src/main/java/com/tns/Runtime.java
Comment on lines
+80
to
+92
| The runtime path is wired through | ||
| [`com.tns.ClassResolver`](test-app/runtime/src/main/java/com/tns/ClassResolver.java) | ||
| → [`com.tns.DexFactory`](test-app/runtime/src/main/java/com/tns/DexFactory.java): | ||
|
|
||
| 1. `ClassResolver.resolveClass` first tries `classStorageService.retrieveClass(name)`. | ||
| In production this hits the SBG-generated dex and we're done. | ||
| 2. On `LookedUpClassNotFound` (typical for HMR), if a `baseClassName` is | ||
| present, `ClassResolver` falls back to `DexFactory.resolveClass(...)` | ||
| which runs the same `ProxyGenerator`/`Dump` pipeline SBG uses — only | ||
| it does it at runtime, writes the dex into the app's per-thumb cache | ||
| under `<dexDir>/<name>.dex`, wraps it in a `.jar`, and loads it via | ||
| `DexClassLoader` (or `BaseDexClassLoader` injection when the | ||
| `injectIntoParentClassLoader` flag is on). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Dex cache file path in doc omits thumb suffix.
Doc says the dex is written to <dexDir>/<name>.dex, but DexFactory.getDexFile actually appends the thumb: dexDir + "/" + classToProxyFile + "-" + dexThumb + ".dex". Since this section doubles as a troubleshooting reference (see "What to look at when this breaks"), the actual on-disk filename pattern is worth stating precisely.
✏️ Suggested wording fix
- which runs the same `ProxyGenerator`/`Dump` pipeline SBG uses — only
- it does it at runtime, writes the dex into the app's per-thumb cache
- under `<dexDir>/<name>.dex`, wraps it in a `.jar`, and loads it via
+ which runs the same `ProxyGenerator`/`Dump` pipeline SBG uses — only
+ it does it at runtime, writes the dex into the app's per-thumb cache
+ under `<dexDir>/<name>-<dexThumb>.dex`, wraps it in a `.jar`, and loads it via
`DexClassLoader` (or `BaseDexClassLoader` injection when the📝 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
| The runtime path is wired through | |
| [`com.tns.ClassResolver`](test-app/runtime/src/main/java/com/tns/ClassResolver.java) | |
| → [`com.tns.DexFactory`](test-app/runtime/src/main/java/com/tns/DexFactory.java): | |
| 1. `ClassResolver.resolveClass` first tries `classStorageService.retrieveClass(name)`. | |
| In production this hits the SBG-generated dex and we're done. | |
| 2. On `LookedUpClassNotFound` (typical for HMR), if a `baseClassName` is | |
| present, `ClassResolver` falls back to `DexFactory.resolveClass(...)` | |
| which runs the same `ProxyGenerator`/`Dump` pipeline SBG uses — only | |
| it does it at runtime, writes the dex into the app's per-thumb cache | |
| under `<dexDir>/<name>.dex`, wraps it in a `.jar`, and loads it via | |
| `DexClassLoader` (or `BaseDexClassLoader` injection when the | |
| `injectIntoParentClassLoader` flag is on). | |
| The runtime path is wired through | |
| [`com.tns.ClassResolver`](test-app/runtime/src/main/java/com/tns/ClassResolver.java) | |
| → [`com.tns.DexFactory`](test-app/runtime/src/main/java/com/tns/DexFactory.java): | |
| 1. `ClassResolver.resolveClass` first tries `classStorageService.retrieveClass(name)`. | |
| In production this hits the SBG-generated dex and we're done. | |
| 2. On `LookedUpClassNotFound` (typical for HMR), if a `baseClassName` is | |
| present, `ClassResolver` falls back to `DexFactory.resolveClass(...)` | |
| which runs the same `ProxyGenerator`/`Dump` pipeline SBG uses — only | |
| it does it at runtime, writes the dex into the app's per-thumb cache | |
| under `<dexDir>/<name>-<dexThumb>.dex`, wraps it in a `.jar`, and loads it via | |
| `DexClassLoader` (or `BaseDexClassLoader` injection when the | |
| `injectIntoParentClassLoader` flag is on). |
🤖 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 `@README.md` around lines 80 - 92, The runtime cache path description is
incomplete: the dex filename pattern in the README should match
DexFactory.getDexFile. Update the documentation around ClassResolver and
DexFactory to state that the generated dex is written with the thumb suffix
(class name plus dex thumb) rather than just <name>.dex, so the troubleshooting
guidance reflects the actual on-disk path.
Comment on lines
+1348
to
+1361
| void | ||
| CallbackHandlers::TerminateAllWorkersCallback(const v8::FunctionCallbackInfo<v8::Value> &args) { | ||
| // `__NS_DEV__.terminateAllWorkers()` — main-isolate-only dev helper. | ||
| // Tears down every worker parented by this isolate through the WorkerWrapper | ||
| // registry. TerminateChildren snapshots the registry under its lock, | ||
| // terminates and clears each worker, and lets each one cascade into its own | ||
| // nested workers, so a worker self-terminating in parallel can't invalidate | ||
| // the walk. Returns the number of direct (top-level) workers torn down so | ||
| // the HMR client can log it. | ||
| auto isolate = args.GetIsolate(); | ||
| HandleScope scope(isolate); | ||
|
|
||
| int terminated = WorkerWrapper::TerminateChildren(isolate); | ||
| args.GetReturnValue().Set(terminated); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap worker termination in the same V8 callback exception boundary.
TerminateChildren() can reach JNI through child->Terminate(). If that throws, this callback currently lets a C++ exception escape across V8; the neighboring worker callbacks convert it back to NativeScriptException.
Proposed fix
void
CallbackHandlers::TerminateAllWorkersCallback(const v8::FunctionCallbackInfo<v8::Value> &args) {
- // `__NS_DEV__.terminateAllWorkers()` — main-isolate-only dev helper.
- // Tears down every worker parented by this isolate through the WorkerWrapper
- // registry. TerminateChildren snapshots the registry under its lock,
- // terminates and clears each worker, and lets each one cascade into its own
- // nested workers, so a worker self-terminating in parallel can't invalidate
- // the walk. Returns the number of direct (top-level) workers torn down so
- // the HMR client can log it.
auto isolate = args.GetIsolate();
- HandleScope scope(isolate);
+ try {
+ HandleScope scope(isolate);
- int terminated = WorkerWrapper::TerminateChildren(isolate);
- args.GetReturnValue().Set(terminated);
+ int terminated = WorkerWrapper::TerminateChildren(isolate);
+ args.GetReturnValue().Set(terminated);
+ } catch (NativeScriptException &ex) {
+ ex.ReThrowToV8();
+ } catch (std::exception &e) {
+ std::stringstream ss;
+ ss << "Error: c++ exception: " << e.what() << std::endl;
+ NativeScriptException nsEx(ss.str());
+ nsEx.ReThrowToV8();
+ } catch (...) {
+ NativeScriptException nsEx(std::string("Error: c++ exception!"));
+ nsEx.ReThrowToV8();
+ }
}📝 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
| void | |
| CallbackHandlers::TerminateAllWorkersCallback(const v8::FunctionCallbackInfo<v8::Value> &args) { | |
| // `__NS_DEV__.terminateAllWorkers()` — main-isolate-only dev helper. | |
| // Tears down every worker parented by this isolate through the WorkerWrapper | |
| // registry. TerminateChildren snapshots the registry under its lock, | |
| // terminates and clears each worker, and lets each one cascade into its own | |
| // nested workers, so a worker self-terminating in parallel can't invalidate | |
| // the walk. Returns the number of direct (top-level) workers torn down so | |
| // the HMR client can log it. | |
| auto isolate = args.GetIsolate(); | |
| HandleScope scope(isolate); | |
| int terminated = WorkerWrapper::TerminateChildren(isolate); | |
| args.GetReturnValue().Set(terminated); | |
| void | |
| CallbackHandlers::TerminateAllWorkersCallback(const v8::FunctionCallbackInfo<v8::Value> &args) { | |
| auto isolate = args.GetIsolate(); | |
| try { | |
| HandleScope scope(isolate); | |
| int terminated = WorkerWrapper::TerminateChildren(isolate); | |
| args.GetReturnValue().Set(terminated); | |
| } catch (NativeScriptException &ex) { | |
| ex.ReThrowToV8(); | |
| } catch (std::exception &e) { | |
| std::stringstream ss; | |
| ss << "Error: c++ exception: " << e.what() << std::endl; | |
| NativeScriptException nsEx(ss.str()); | |
| nsEx.ReThrowToV8(); | |
| } catch (...) { | |
| NativeScriptException nsEx(std::string("Error: c++ exception!")); | |
| nsEx.ReThrowToV8(); | |
| } | |
| } |
🤖 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/CallbackHandlers.cpp` around lines 1348 - 1361,
Wrap CallbackHandlers::TerminateAllWorkersCallback in the same V8 exception
handling pattern used by the neighboring worker callbacks so any exception from
WorkerWrapper::TerminateChildren or child->Terminate() is converted to
NativeScriptException instead of escaping across V8. Locate the fix in
TerminateAllWorkersCallback and apply the same try/catch boundary and
rethrow/forwarding behavior already used in the adjacent callback handlers that
call into WorkerWrapper.
Comment on lines
+1798
to
1816
| auto endsWith = [](const string& s, const string& suffix) { | ||
| return s.size() >= suffix.size() && | ||
| s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0; | ||
| }; | ||
| if (endsWith(normalized, ".mjs")) { | ||
| normalized.resize(normalized.size() - 4); | ||
| } else if (endsWith(normalized, ".js")) { | ||
| normalized.resize(normalized.size() - 3); | ||
| } | ||
|
|
||
| fullPathToFile = normalized; | ||
|
|
||
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), '/', '_'); | ||
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), '.', '_'); | ||
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), '-', '_'); | ||
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), ' ', '_'); | ||
|
|
||
| std::vector<std::string> pathParts; | ||
|
|
||
| Util::SplitString(fullPathToFile, "_", pathParts); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip query/fragment and sanitize all non-identifier characters.
Cache-busted HTTP module URLs like foo.mjs?t=... won’t match the .mjs suffix check and can leave ?, =, &, or # in the generated token.
Proposed fix
+ size_t queryOrFragment = normalized.find_first_of("?#");
+ if (queryOrFragment != string::npos) {
+ normalized.resize(queryOrFragment);
+ }
+
auto endsWith = [](const string& s, const string& suffix) {
return s.size() >= suffix.size() &&
s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0;
};
@@
- std::replace(fullPathToFile.begin(), fullPathToFile.end(), '/', '_');
- std::replace(fullPathToFile.begin(), fullPathToFile.end(), '.', '_');
- std::replace(fullPathToFile.begin(), fullPathToFile.end(), '-', '_');
- std::replace(fullPathToFile.begin(), fullPathToFile.end(), ' ', '_');
+ for (char& ch : fullPathToFile) {
+ const unsigned char c = static_cast<unsigned char>(ch);
+ const bool isIdentifierChar =
+ (c >= 'A' && c <= 'Z') ||
+ (c >= 'a' && c <= 'z') ||
+ (c >= '0' && c <= '9') ||
+ ch == '_';
+ if (!isIdentifierChar) {
+ ch = '_';
+ }
+ }📝 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 endsWith = [](const string& s, const string& suffix) { | |
| return s.size() >= suffix.size() && | |
| s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0; | |
| }; | |
| if (endsWith(normalized, ".mjs")) { | |
| normalized.resize(normalized.size() - 4); | |
| } else if (endsWith(normalized, ".js")) { | |
| normalized.resize(normalized.size() - 3); | |
| } | |
| fullPathToFile = normalized; | |
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), '/', '_'); | |
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), '.', '_'); | |
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), '-', '_'); | |
| std::replace(fullPathToFile.begin(), fullPathToFile.end(), ' ', '_'); | |
| std::vector<std::string> pathParts; | |
| Util::SplitString(fullPathToFile, "_", pathParts); | |
| size_t queryOrFragment = normalized.find_first_of("?#"); | |
| if (queryOrFragment != string::npos) { | |
| normalized.resize(queryOrFragment); | |
| } | |
| auto endsWith = [](const string& s, const string& suffix) { | |
| return s.size() >= suffix.size() && | |
| s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0; | |
| }; | |
| if (endsWith(normalized, ".mjs")) { | |
| normalized.resize(normalized.size() - 4); | |
| } else if (endsWith(normalized, ".js")) { | |
| normalized.resize(normalized.size() - 3); | |
| } | |
| fullPathToFile = normalized; | |
| for (char& ch : fullPathToFile) { | |
| const unsigned char c = static_cast<unsigned char>(ch); | |
| const bool isIdentifierChar = | |
| (c >= 'A' && c <= 'Z') || | |
| (c >= 'a' && c <= 'z') || | |
| (c >= '0' && c <= '9') || | |
| ch == '_'; | |
| if (!isIdentifierChar) { | |
| ch = '_'; | |
| } | |
| } | |
| std::vector<std::string> pathParts; | |
| Util::SplitString(fullPathToFile, "_", pathParts); |
🤖 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/MetadataNode.cpp` around lines 1798 - 1816, The
module-name normalization in MetadataNode::GetModulePath should strip any query
string or fragment before checking for .mjs/.js suffixes, since cache-busted
URLs can bypass the current extension trimming. Update the logic around the
normalized/fullPathToFile handling to remove everything after ? or # first, then
keep sanitizing all non-identifier characters (including ?, =, &, #) before the
Util::SplitString step.
Comment on lines
+101
to
+113
| int status = 0; | ||
| demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); | ||
| if (status == 0 && demangled) { | ||
| symName = demangled; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| __android_log_print(ANDROID_LOG_FATAL, "TNS.Native", | ||
| " #%02d pc 0x%016lx %s (%s+%lu)", idx, | ||
| static_cast<unsigned long>(relPc), libName, symName, | ||
| static_cast<unsigned long>(symOff)); | ||
| if (demangled) free(demangled); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect allocating operations still present in the fatal signal path.
python - <<'PY'
from pathlib import Path
p = Path('test-app/runtime/src/main/cpp/Runtime.cpp')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 85 <= i <= 176 and any(token in line for token in ['__cxa_demangle', 'free(', 'stringstream', 'malloc', 'new ']):
print(f'{i}: {line}')
PYRepository: NativeScript/android
Length of output: 472
🏁 Script executed:
#!/bin/bash
sed -n '90,180p' test-app/runtime/src/main/cpp/Runtime.cpp | cat -nRepository: NativeScript/android
Length of output: 4653
Avoid heap allocation in the fatal signal handler. abi::__cxa_demangle and the matching free() make this path depend on the allocator; if the crash already involves heap corruption, the backtrace can hang or fail before logging completes. Keep info.dli_sname as-is and demangle offline.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 108-108: #error Unsupported bitness
(preprocessorErrorDirective)
🤖 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` around lines 101 - 113, The fatal
signal handler in Runtime.cpp currently heap-allocates via abi::__cxa_demangle
and frees the result, which makes the crash path depend on the allocator. Update
the backtrace formatting logic around the symbol lookup to avoid any allocation
in this handler: keep info.dli_sname unchanged for logging, remove the
demangling/freeing work from this path, and move demangling to an offline or
non-signal-handling context if needed. Use the existing backtrace loop and
__android_log_print call site as the place to preserve safe, allocator-free
logging.
Comment on lines
+55
to
+86
| 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 | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify coverage for URL.search/searchParams cache synchronization.
rg -n -C3 'searchParams|URLSearchParams|URL\\.createObjectURL' test-app/runtime/src/main/cpp test-app/app/src/main/assets/app/testsRepository: NativeScript/android
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the URL polyfill and any existing cache-invalidation hooks.
sed -n '1,140p' test-app/runtime/src/main/cpp/URLImpl.cpp
printf '\n--- search-related definitions ---\n'
rg -n -C2 'searchParams|\.search\b|_searchParamsSearch|defineProperty\(URL\.prototype' test-app/runtime/src/main/cpp/URLImpl.cpp
printf '\n--- tests covering URL.search / searchParams interactions ---\n'
rg -n -C3 'searchParams|\.search\s*=|URL\(' test-app/app/src/main/assets/app/tests/testURLImpl.js test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.jsRepository: NativeScript/android
Length of output: 14841
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the URL.search accessor implementation and any cache-reset logic.
rg -n -C4 'SetSearch|GetSearch|search\b|_searchParams|searchParams' test-app/runtime/src/main/cpp/URLImpl.cpp
printf '\n--- accessor block around search ---\n'
sed -n '140,260p' test-app/runtime/src/main/cpp/URLImpl.cppRepository: NativeScript/android
Length of output: 8160
Keep URL.searchParams in sync when search is reassigned. SetSearch updates the backing URL but never resyncs the cached params object, so url.searchParams can expose stale data after url.search = ...; update the existing params object instead of recreating it.
🤖 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/URLImpl.cpp` around lines 55 - 86, The
URL.searchParams getter caches a URLSearchParams instance, but the SetSearch
path does not refresh that cached object when url.search is reassigned, so it
can become stale. Update the URLImpl URL/search handling so the existing
_searchParams object is synchronized with the new search string in SetSearch
instead of replacing or leaving it unchanged, and keep the URLSearchParams
methods on the cached instance consistent with the updated URL.
Comment on lines
+1
to
+2
| #define NATIVE_SCRIPT_RUNTIME_VERSION "9.1.0-alpha.7" | ||
| #define NATIVE_SCRIPT_RUNTIME_COMMIT_SHA "no commit sha was provided by build.gradle build" No newline at end of file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether build tooling rewrites NATIVE_SCRIPT_RUNTIME_COMMIT_SHA.
rg -n -C3 'NATIVE_SCRIPT_RUNTIME_COMMIT_SHA|Version\\.h|commit sha' .Repository: NativeScript/android
Length of output: 1470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the runtime build wiring around Version.h and the commit SHA placeholder.
printf '\n== build.gradle relevant lines ==\n'
sed -n '1,220p' build.gradle | nl -ba | sed -n '1,140p'
printf '\n== files that mention Version.h generation ==\n'
rg -n -C 2 'Version\.h|arVersion|pVersion|commit sha was provided by build\.gradle build|native.*version' build.gradle test-app/runtime -g '!**/dist/**'Repository: NativeScript/android
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== build.gradle excerpt ==\n'
awk 'NR>=1 && NR<=220 {printf "%4d %s\n", NR, $0}' build.gradle
printf '\n== Version.h generation refs ==\n'
rg -n -C 3 'Version\.h|arVersion|pVersion|commit sha was provided by build\.gradle build|configure.*Version|generate.*Version' build.gradle test-app/runtime -g '!**/dist/**'Repository: NativeScript/android
Length of output: 14555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== exact runtime build replacements ==\n'
sed -n '260,305p' test-app/runtime/build.gradle | awk '{printf "%4d %s\n", NR+259, $0}'
printf '\n== all placeholder symbols in repo ==\n'
rg -n 'RUNTIME_COMMIT_SHA_PLACEHOLDER|0\.0\.0\.0|no commit sha was provided by build\.gradle build' test-app/runtime build.gradleRepository: NativeScript/android
Length of output: 2400
Replace the commit SHA fallback before packaging
test-app/runtime/build.gradle only rewrites RUNTIME_COMMIT_SHA_PLACEHOLDER, so the checked-in value in test-app/runtime/src/main/cpp/Version.h stays as "no commit sha was provided by build.gradle build" and gets logged at startup. Update the literal or the replacement token so release builds carry the actual git SHA.
🤖 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/Version.h` around lines 1 - 2, The checked-in
fallback for the runtime commit SHA in Version.h is still the placeholder
string, so startup logs can show a bogus value. Update the Version.h literal or
make test-app/runtime/build.gradle replace the exact symbol used by
NATIVE_SCRIPT_RUNTIME_COMMIT_SHA so packaged release builds include the real git
SHA instead of the fallback.
NathanWalker
force-pushed
the
feat/hmr-dev-sessions
branch
from
July 16, 2026 20:32
f821c25 to
7c48d11
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/src/main/cpp/HMRSupport.cpp`:
- Around line 697-729: Remove the process-wide keep-alive workaround guarded by
sKeepAliveDisabled, including the System.setProperty("http.keepAlive", "false")
JNI calls. Preserve the existing per-request Connection: close header and retry
path so the workaround remains scoped to loader requests.
- Around line 1015-1044: Update KickstartScheduleUrls so it does not create one
detached thread per URL or call EnterPending before unbounded thread
construction. Build a shared URL queue and start at most maxConcurrent worker
threads that consume it, ensuring thread creation is bounded and construction
failures cannot leave pending state inconsistent.
In `@test-app/runtime/src/main/cpp/ModuleInternalCallbacks.cpp`:
- Around line 751-789: Update RemoveModuleFromRegistry and InvalidateModules to
also clear the corresponding handle in g_vendorModuleCache whenever the
canonical key is an ns-vendor://<id> entry. Keep registry removal and existing
URL eviction behavior unchanged, and ensure both APIs evict the vendor cache
entry so ResolveFromVendorRegistry cannot return the stale module.
- Around line 580-592: The declaration generation in ResolveFromVendorRegistry
must not use export names that are JavaScript reserved words, even when
IsValidJSIdentifier accepts them. Detect reserved keywords and emit a safe local
alias for the declaration, then re-export that alias under the original name;
retain direct declarations for non-reserved valid identifiers.
- Around line 1728-1741: Update the dynamic-import evaluation flow around
blobMod->Evaluate() to await its returned promise before resolving the module
namespace, propagating rejected evaluation promises to the import resolver.
Apply the same promise chaining and fulfillment-only namespace resolution to the
other dynamic-import branches, while preserving the existing synchronous error
handling.
In `@test-app/runtime/src/main/cpp/Runtime.cpp`:
- Around line 225-240: Update the signal-handler setup around sigaltstack and
the sigaction calls to check each return value and log or otherwise surface
registration failures. Ensure failures for the alternate stack and every signal
in this initialization path are reported, while preserving the existing handler
configuration.
🪄 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
Run ID: 503fb44d-5be8-4da5-a9c1-bf6a23e9b351
📒 Files selected for processing (26)
README.mdtest-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/esm/meta-no-hot.mjstest-app/app/src/main/assets/app/tests/testHttpCanonicalKey.mjstest-app/app/src/main/assets/app/tests/testNodeBuiltinsAndOptionalModules.mjstest-app/app/src/main/assets/app/tests/testNsDevBoundary.mjstest-app/runtime/src/main/cpp/CallbackHandlers.cpptest-app/runtime/src/main/cpp/CallbackHandlers.htest-app/runtime/src/main/cpp/DevFlags.cpptest-app/runtime/src/main/cpp/DevFlags.htest-app/runtime/src/main/cpp/HMRSupport.cpptest-app/runtime/src/main/cpp/HMRSupport.htest-app/runtime/src/main/cpp/MetadataNode.cpptest-app/runtime/src/main/cpp/ModuleInternal.cpptest-app/runtime/src/main/cpp/ModuleInternalCallbacks.cpptest-app/runtime/src/main/cpp/ModuleInternalCallbacks.htest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/URLImpl.cpptest-app/runtime/src/main/cpp/URLImpl.htest-app/runtime/src/main/cpp/Version.htest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/cpp/WorkerWrapper.htest-app/runtime/src/main/java/com/tns/AppConfig.javatest-app/runtime/src/main/java/com/tns/ClassResolver.javatest-app/runtime/src/main/java/com/tns/DexFactory.javatest-app/runtime/src/main/java/com/tns/Runtime.java
🚧 Files skipped from review as they are similar to previous changes (22)
- test-app/runtime/src/main/cpp/Version.h
- test-app/app/src/main/assets/app/tests/esm/meta-no-hot.mjs
- test-app/runtime/src/main/cpp/DevFlags.h
- test-app/runtime/src/main/cpp/CallbackHandlers.h
- test-app/app/src/main/assets/app/tests/testHttpCanonicalKey.mjs
- test-app/app/src/main/assets/app/tests/testNodeBuiltinsAndOptionalModules.mjs
- test-app/runtime/src/main/java/com/tns/Runtime.java
- test-app/runtime/src/main/cpp/ModuleInternalCallbacks.h
- test-app/runtime/src/main/cpp/WorkerWrapper.cpp
- test-app/app/src/main/assets/app/mainpage.js
- README.md
- test-app/runtime/src/main/cpp/CallbackHandlers.cpp
- test-app/runtime/src/main/cpp/URLImpl.cpp
- test-app/runtime/src/main/java/com/tns/AppConfig.java
- test-app/runtime/src/main/java/com/tns/DexFactory.java
- test-app/runtime/src/main/cpp/WorkerWrapper.h
- test-app/runtime/src/main/cpp/ModuleInternal.cpp
- test-app/runtime/src/main/cpp/HMRSupport.h
- test-app/runtime/src/main/cpp/MetadataNode.cpp
- test-app/runtime/src/main/cpp/DevFlags.cpp
- test-app/app/src/main/assets/app/tests/testNsDevBoundary.mjs
- test-app/runtime/src/main/cpp/URLImpl.h
Comment on lines
+580
to
+592
| static bool IsValidJSIdentifier(const std::string& name) { | ||
| if (name.empty()) return false; | ||
| char first = name[0]; | ||
| if (!((first >= 'a' && first <= 'z') || (first >= 'A' && first <= 'Z') || | ||
| first == '_' || first == '$')) | ||
| return false; | ||
| for (size_t i = 1; i < name.size(); i++) { | ||
| char c = name[i]; | ||
| if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || | ||
| (c >= '0' && c <= '9') || c == '_' || c == '$')) | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C4 'IsValidJSIdentifier|export const .*__mod' \
test-app/runtime/src/main/cpp test-app/app/src/main/assets/app/testsRepository: NativeScript/android
Length of output: 4223
🏁 Script executed:
sed -n '576,686p' test-app/runtime/src/main/cpp/ModuleInternalCallbacks.cppRepository: NativeScript/android
Length of output: 4605
🏁 Script executed:
rg -n 'reserved|keyword|IsValidJSIdentifier|exportNames' test-app/runtime/src/main/cpp/ModuleInternalCallbacks.cpp test-app/runtime/src/main/cppRepository: NativeScript/android
Length of output: 43980
Avoid using export names as declaration identifiers
In ResolveFromVendorRegistry, names like class or await can pass IsValidJSIdentifier but still make export const <name> = ... invalid syntax. Emit a safe local alias and re-export it under the original name.
🤖 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/ModuleInternalCallbacks.cpp` around lines 580 -
592, The declaration generation in ResolveFromVendorRegistry must not use export
names that are JavaScript reserved words, even when IsValidJSIdentifier accepts
them. Detect reserved keywords and emit a safe local alias for the declaration,
then re-export that alias under the original name; retain direct declarations
for non-reserved valid identifiers.
Comment on lines
+751
to
+789
| void RemoveModuleFromRegistry(const std::string& canonicalKey) { | ||
| const std::string registryKey = CanonicalizeRegistryKey(canonicalKey); | ||
| // Defensive: never wipe a sentinel key. | ||
| if (registryKey == "@" || | ||
| registryKey.find("__invalid_at__.mjs") != std::string::npos) { | ||
| if (IsScriptLoadingLogEnabled()) { | ||
| DEBUG_WRITE("[resolver][guard] ignore remove for sentinel %s", registryKey.c_str()); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| auto it = g_moduleRegistry.find(registryKey); | ||
| if (it != g_moduleRegistry.end()) { | ||
| bool isHttpKey = StartsWith(registryKey, "http://") || StartsWith(registryKey, "https://"); | ||
| if (IsScriptLoadingLogEnabled() && !isHttpKey) { | ||
| DEBUG_WRITE("[resolver] removing stale module %s", registryKey.c_str()); | ||
| } | ||
| it->second.Reset(); | ||
| g_moduleRegistry.erase(it); | ||
| } | ||
| } | ||
|
|
||
| size_t InvalidateModules(const std::vector<std::string>& keys) { | ||
| size_t removed = 0; | ||
| std::vector<std::string> urlsToEvict; | ||
| urlsToEvict.reserve(keys.size()); | ||
| for (const auto& raw : keys) { | ||
| if (raw.empty()) continue; | ||
| const std::string registryKey = CanonicalizeRegistryKey(raw); | ||
| auto it = g_moduleRegistry.find(registryKey); | ||
| if (it != g_moduleRegistry.end()) { | ||
| it->second.Reset(); | ||
| g_moduleRegistry.erase(it); | ||
| ++removed; | ||
| } | ||
| if (StartsWith(registryKey, "http://") || StartsWith(registryKey, "https://")) { | ||
| urlsToEvict.push_back(registryKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Evict the vendor cache alongside the module registry.
Invalidating ns-vendor://<id> removes only g_moduleRegistry; ResolveFromVendorRegistry() then returns the same module from g_vendorModuleCache. Clear the corresponding vendor-cache handle in both removal APIs.
🤖 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/ModuleInternalCallbacks.cpp` around lines 751 -
789, Update RemoveModuleFromRegistry and InvalidateModules to also clear the
corresponding handle in g_vendorModuleCache whenever the canonical key is an
ns-vendor://<id> entry. Keep registry removal and existing URL eviction behavior
unchanged, and ensure both APIs evict the vendor cache entry so
ResolveFromVendorRegistry cannot return the stale module.
Comment on lines
+225
to
+240
| sigaltstack(&altStack, nullptr); | ||
|
|
||
| struct sigaction action; | ||
| action.sa_handler = SIG_handler; | ||
| memset(&action, 0, sizeof(action)); | ||
| sigemptyset(&action.sa_mask); | ||
| // SA_SIGINFO enables the 3-arg handler so we get siginfo_t (fault addr | ||
| // and si_code). SA_ONSTACK lets the handler run on an alternate stack | ||
| // — important so we still produce a useful backtrace if the original | ||
| // crash was a stack overflow. | ||
| action.sa_flags = SA_SIGINFO | SA_ONSTACK; | ||
| action.sa_sigaction = SIG_handler; | ||
| sigaction(SIGABRT, &action, NULL); | ||
| sigaction(SIGSEGV, &action, NULL); | ||
| sigaction(SIGBUS, &action, NULL); | ||
| sigaction(SIGFPE, &action, NULL); | ||
| sigaction(SIGILL, &action, NULL); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='test-app/runtime/src/main/cpp/Runtime.cpp'
echo '--- file size ---'
wc -l "$file"
echo '--- relevant lines ---'
sed -n '190,270p' "$file"Repository: NativeScript/android
Length of output: 3083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='test-app/runtime/src/main/cpp/Runtime.cpp'
echo '--- other sigaltstack/sigaction occurrences ---'
rg -n '\bsigaltstack\b|\bsigaction\b' "$file"
echo '--- nearby helper/error logging patterns ---'
rg -n '__android_log_print|perror|strerror|errno' "$file" | head -n 50Repository: NativeScript/android
Length of output: 1234
Check signal-handler installation failures. sigaltstack() and each sigaction() return value is ignored here, so initialization can silently leave the crash handler inactive. Log or surface an error when registration fails.
🤖 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` around lines 225 - 240, Update the
signal-handler setup around sigaltstack and the sigaction calls to check each
return value and log or otherwise surface registration failures. Ensure failures
for the alternate stack and every signal in this initialization path are
reported, while preserving the existing handler configuration.
This was referenced Jul 28, 2026
Canonicalize module identity into three registry shapes — http(s) URLs, custom schemes (node:, blob:, optional:), and absolute file paths — and key the module registries by v8::Isolate instead of thread_local storage. import() now rejects missing bare specifiers instead of installing placeholders; optional-module placeholders are built without string interpolation, detection is unified in IsLikelyOptionalModule, and module source preserves embedded NUL bytes. Thenables handed to the loader from JS are adopted properly. Blob URLs (blob:nativescript/<uuid>) become first-class module identities via URL.createObjectURL and URL.InternalAccessor. The prewarm/prefetch machinery is replaced by an async module-graph loader; boot hands off to a manual runloop that pumps pending module work when the entry script has not reached the main looper yet (e.g. a top-level-await entry still loading its graph). Load surfaces the failure cause to callers, and relative import() against a filesystem referrer keeps the already-absolute path instead of prefixing the application root twice.
Dev sessions serve the app's module graph over HTTP during development,
with a mechanism-only dev-loader contract: policy stays in JS tooling,
the runtime supplies fetch/registry/invalidations. The loader is
deny-by-default — remote allowlist entries only authorize URLs on a
URL-component boundary ('/', '?', '#' or exact match), refusing
lookalike-host and lookalike-port bypasses; a specific port must be
listed explicitly. Hot-path hash containers use robin_hood maps.
Per-fetch URL logging is opt-in via the httpFetchUrlLog config flag
(volume is one line per fetch), alongside the existing
logScriptLoading-gated diagnostics. The previous HMRSupport/DevFlags
sources are replaced by HttpLoader (JNI HttpURLConnection).
The dev-loader control surface (HttpLoader) is reachable from JS as the ns:module builtin module: NsBuiltinModules routes ns:module through BuildNsModuleBinding — the binding builder decides build-dependent membership — and ns-module.js (compiled in via js2c) shapes and freezes whatever arrives. docs/ns-builtin-modules.md documents the surface.
Worker entry-script load failures now reach worker.onerror instead of failing silently. Messages posted before the worker's entry script has installed onmessage are no longer dropped: ConcurrentQueue::Signal re-arms the drain source without enqueueing (a silent no-op when racing Terminate), and WorkerWrapper retries delivery through a deferred drain, with drainRetryPending_ preventing one stacked retry per attempt.
The ns:module surface, remote-module allowlist boundary matching, and relative ESM dynamic-import cases exercise the async loader and the deny-by-default HTTP gate. The on-device result harvester falls back to run-as when adb root is unavailable (Play Store emulator images), and -Pabis is forwarded so a single-ABI V8 tree can build and test locally.
…into ns:runtime Live log flags (logScriptLoading, httpFetchUrlLog) move onto ns:runtime setConfig/getConfig. Remote-module security stays boot-time nativescript.config only. Android does not expose releasedObjectPolicy.
NathanWalker
force-pushed
the
feat/hmr-dev-sessions
branch
from
August 13, 2026 02:19
7c48d11 to
a20f2bc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
test-app/runtime/src/main/cpp/HttpLoader.cpp (3)
525-532: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRestrict the retry to transport errors.
PerformHttpFetchOnceSyncreturns false for any non-2xx status, so this retry also fires for deterministic responses such as 404 and 403. Each miss then costs an extra request plus a 120 ms sleep on the calling thread, which is the JS thread on the cold-boot path. The header contract states "one retry on transport error" (HttpLoader.hLine 69).Gate the retry on
status == 0, which is the transport-failure signal.♻️ Proposed fix
bool ok = PerformHttpFetchOnceSync(url, out, contentType, status); - if (!ok) { + if (!ok && status == 0) { if (IsScriptLoadingLogEnabled()) { DEBUG_WRITE_FORCE("[http-loader] retrying %s after initial fetch error", url.c_str()); } usleep(120 * 1000); ok = PerformHttpFetchOnceSync(url, out, contentType, status); }The same gate applies to the async path at Lines 781-788.
🤖 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/HttpLoader.cpp` around lines 525 - 532, Restrict the synchronous retry in PerformHttpFetchOnceSync’s caller to transport failures by requiring status == 0 alongside !ok before sleeping and retrying. Apply the same status == 0 gate to the retry condition in the asynchronous path, while preserving existing logging and retry behavior for transport errors.
841-848: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport installation failure instead of aborting.
InstallDevFunctionusesToLocalChecked()and.Check(), so any failure terminates the process.BuildNsModuleBindingis documented to return false when the binding could not be populated (HttpLoader.hLines 174-175), and thecanonicalizeHttpUrlKeybranch below already follows that contract. Make the four core members behave the same way.♻️ Proposed refactor
-void InstallDevFunction(v8::Isolate* isolate, v8::Local<v8::Context> context, +bool InstallDevFunction(v8::Isolate* isolate, v8::Local<v8::Context> context, v8::Local<v8::Object> target, const char* name, v8::FunctionCallback callback) { - v8::Local<v8::FunctionTemplate> fnTpl = v8::FunctionTemplate::New(isolate, callback); - v8::Local<v8::Function> fn = fnTpl->GetFunction(context).ToLocalChecked(); + v8::Local<v8::Function> fn; + if (!v8::FunctionTemplate::New(isolate, callback)->GetFunction(context).ToLocal(&fn)) { + return false; + } fn->SetName(ToV8String(isolate, name)); - target->CreateDataProperty(context, ToV8String(isolate, name), fn).Check(); + return target->CreateDataProperty(context, ToV8String(isolate, name), fn).FromMaybe(false); }Then propagate the result from each call site in
BuildNsModuleBinding.🤖 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/HttpLoader.cpp` around lines 841 - 848, Update InstallDevFunction to report installation failures through a boolean result instead of using ToLocalChecked() and Check(), while preserving successful registration behavior. Change each core-member call in BuildNsModuleBinding to inspect and propagate that result, matching the existing canonicalizeHttpUrlKey failure path and returning false when any installation fails.
775-776: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBound the number of fetch threads.
Each call spawns one detached
std::thread. The phase-1 module-graph walk fetches every import, so a large graph creates one thread per module URL with no upper bound. Thread creation cost and memory pressure grow with graph size, and the origin receives an unbounded burst of parallel connections.Use a small fixed-size worker pool with a work queue instead, and cap the in-flight request count.
🤖 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/HttpLoader.cpp` around lines 775 - 776, Replace the per-call detached thread created around the fetch logic in HttpLoader with a small fixed-size worker pool and synchronized work queue. Route each URL/completion task through the queue, enforce a fixed maximum number of concurrent requests, and preserve completion delivery and existing fetch behavior while preventing one worker thread from being created per module URL.
🤖 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/app/src/main/assets/app/tests/testNsModule.js`:
- Around line 35-44: In test-app/app/src/main/assets/app/tests/testNsModule.js
lines 35-44, save global.__NS_HMR_BOOT_COMPLETE__ before the spec and restore
that saved value during cleanup instead of forcing false. In lines 88-104, move
configureLoader into beforeEach/afterEach so each spec restores the prior loader
configuration and re-installs the boot-time canonicalization vocabulary after
execution.
In `@test-app/app/src/main/assets/app/tests/testRemoteModuleSecurity.js`:
- Around line 146-153: Rename the spec describing
com.tns.Runtime.isRemoteUrlAllowed so its title reflects that it verifies the
helper exists and preserves the debug bypass, not refusal of lookalike-host
prefixes. Keep the assertions unchanged; do not claim boundary matching is
tested unless a separate directly reachable test is added.
In `@test-app/runtests.gradle`:
- Around line 70-77: Remove ignoreExitValue = true from the
android_unit_test_results.xml cleanup task so failures from the run-as removal
command stop the flow instead of allowing stale results to remain; keep the
existing rm -f cleanup behavior and platform-specific command handling
unchanged.
In `@test-app/runtime/src/main/cpp/HttpLoader.cpp`:
- Around line 872-888: Replace the global JSON lookup and manual stringify
invocation in the importMap object branch with v8::JSON::Stringify, preserving
the existing result-to-UTF-8 conversion and jsonStr assignment only when
serialization succeeds. Remove the ToLocalChecked calls and unchecked JSON
object/function casts from this path.
- Around line 230-254: Replace the unsynchronized globals used by
SetCanonicalizationConfig, ResetCanonicalizationConfig, and
CanonicalizeHttpUrlKey with an atomically published immutable shared snapshot,
using the existing project conventions for atomic shared-pointer access. Publish
a new const CanonicalizationConfig on configure and a null snapshot on reset;
have CanonicalizeHttpUrlKey acquire one snapshot at entry, check it for
configuration state, and use that stable snapshot throughout the call instead of
g_canonConfigured or g_canonConfig.
- Around line 700-717: Update the read loop around HttpLoader’s
CallIntMethod(inStream, readMethod, buffer) to check for a pending JNI exception
immediately after each read; record the exception and break before handling n ==
0. Preserve normal EOF and successful reads, while ensuring the recorded
exception is propagated or handled by the surrounding loader flow after cleanup.
- Around line 765-806: Update FetchModuleBodyAsync’s worker thread to detach
from the JVM after invoking completion and completing all JNI-related work. Add
an exception-safe scope guard at the end of the thread lambda so detachment
occurs on normal completion and when an exception exits the lambda, without
changing the existing fetch or callback behavior.
- Around line 808-817: Remove the immediate boot pumping from
MaybePumpJSThreadDuringBoot, or defer its execution until ResolveModuleCallback
and the LoadHttpModuleForUrl/HttpFetchText/InvokeHttpFetch call chain has fully
returned. Ensure neither PerformMicrotaskCheckpoint nor ALooper_pollOnce can
re-enter JavaScript while module instantiation is still active.
In `@test-app/runtime/src/main/cpp/ModuleInternal.cpp`:
- Around line 53-85: Update PromiseRejectionMessage so property reads on the
rejection reason are enclosed in a local v8::TryCatch, covering the
errorObj->Get call and its result handling. Ensure any exception from a proxy or
throwing message getter is caught and does not remain pending on the isolate,
while preserving the existing diagnostic message behavior.
In `@test-app/runtime/src/main/cpp/Runtime.cpp`:
- Around line 342-355: Reduce the synchronous async-module drain deadline in
PumpPendingHttpModuleGraph in test-app/runtime/src/main/cpp/Runtime.cpp (lines
342-355) for the main thread and log when the deadline expires. Also update the
top-level-await handling in test-app/runtime/src/main/cpp/ModuleInternal.cpp
(lines 659-680) to reduce its 30-second main-thread bound or return the pending
promise instead of draining it inline.
In `@test-app/runtime/src/main/cpp/WorkerWrapper.cpp`:
- Around line 158-178: Bound the retry path in WorkerWrapper::DrainPendingTasks
using a looper-scheduled delay instead of spawning and detaching a std::thread
for each retry. Add the proposed kMaxDrainRetryAttempts and looper-thread-only
drainRetryAttempts_ state, increment attempts while onmessage is unavailable,
and reschedule only below the cap; once the cap is reached, fall through to the
existing per-message logging and discard handling. Reset drainRetryAttempts_ to
zero when a valid onmessage handler is found.
In `@test-app/runtime/src/main/java/com/tns/DexFactory.java`:
- Line 197: Update DexFactory.findClass so canonicalName only replaces '/' with
'.', preserving '$' for ordinary nested-class loading before
classLoader.loadClass. Apply underscore normalization only within
generated-proxy lookup, and add regression coverage for both nested-class
loading and proxy-name normalization.
In `@test-app/tools/try_to_find_test_result_file.js`:
- Around line 140-144: Update the result validation in
try_to_find_test_result_file to parse the file with the existing XML parser
before calling process.exit(0), and require a testsuites root so only
verifier-ready artifacts succeed. Replace the startsWith("<?xml") check in both
branches, preserving retry behavior when parsing or root validation fails and
accepting valid XML without an XML declaration.
---
Nitpick comments:
In `@test-app/runtime/src/main/cpp/HttpLoader.cpp`:
- Around line 525-532: Restrict the synchronous retry in
PerformHttpFetchOnceSync’s caller to transport failures by requiring status == 0
alongside !ok before sleeping and retrying. Apply the same status == 0 gate to
the retry condition in the asynchronous path, while preserving existing logging
and retry behavior for transport errors.
- Around line 841-848: Update InstallDevFunction to report installation failures
through a boolean result instead of using ToLocalChecked() and Check(), while
preserving successful registration behavior. Change each core-member call in
BuildNsModuleBinding to inspect and propagate that result, matching the existing
canonicalizeHttpUrlKey failure path and returning false when any installation
fails.
- Around line 775-776: Replace the per-call detached thread created around the
fetch logic in HttpLoader with a small fixed-size worker pool and synchronized
work queue. Route each URL/completion task through the queue, enforce a fixed
maximum number of concurrent requests, and preserve completion delivery and
existing fetch behavior while preventing one worker thread from being created
per module URL.
🪄 Autofix
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: 66cc6baa-801c-4de3-8a03-2b70c9377106
📒 Files selected for processing (32)
build.gradledocs/ns-builtin-modules.mdtest-app/app/src/main/assets/app/tests/testNsModule.jstest-app/app/src/main/assets/app/tests/testNsRuntime.jstest-app/app/src/main/assets/app/tests/testRemoteModuleSecurity.jstest-app/runtests.gradletest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/ConcurrentQueue.cpptest-app/runtime/src/main/cpp/ConcurrentQueue.htest-app/runtime/src/main/cpp/DevFlags.cpptest-app/runtime/src/main/cpp/DevFlags.htest-app/runtime/src/main/cpp/HMRSupport.cpptest-app/runtime/src/main/cpp/HMRSupport.htest-app/runtime/src/main/cpp/HttpLoader.cpptest-app/runtime/src/main/cpp/HttpLoader.htest-app/runtime/src/main/cpp/MetadataNode.cpptest-app/runtime/src/main/cpp/ModuleInternal.cpptest-app/runtime/src/main/cpp/ModuleInternal.htest-app/runtime/src/main/cpp/ModuleInternalCallbacks.cpptest-app/runtime/src/main/cpp/ModuleInternalCallbacks.htest-app/runtime/src/main/cpp/NsBuiltinModules.cpptest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/Runtime.htest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/cpp/WorkerWrapper.htest-app/runtime/src/main/cpp/js/README.mdtest-app/runtime/src/main/cpp/js/ns-module.jstest-app/runtime/src/main/cpp/js/ns-runtime.jstest-app/runtime/src/main/java/com/tns/AppConfig.javatest-app/runtime/src/main/java/com/tns/DexFactory.javatest-app/runtime/src/main/java/com/tns/Runtime.javatest-app/tools/try_to_find_test_result_file.js
💤 Files with no reviewable changes (5)
- test-app/runtime/src/main/cpp/DevFlags.h
- test-app/runtime/src/main/cpp/HMRSupport.h
- test-app/runtime/src/main/cpp/ModuleInternal.h
- test-app/runtime/src/main/cpp/HMRSupport.cpp
- test-app/runtime/src/main/cpp/DevFlags.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- test-app/runtime/src/main/cpp/MetadataNode.cpp
- test-app/runtime/src/main/java/com/tns/Runtime.java
- test-app/runtime/src/main/java/com/tns/AppConfig.java
Comment on lines
+35
to
+44
| it("setDevBootComplete flips the JS-visible boot-complete global", function () { | ||
| var nsModule = require("ns:module"); | ||
| nsModule.setDevBootComplete(true); | ||
| expect(global.__NS_HMR_BOOT_COMPLETE__).toBe(true); | ||
| nsModule.setDevBootComplete(false); | ||
| expect(global.__NS_HMR_BOOT_COMPLETE__).toBe(false); | ||
| nsModule.setDevBootComplete(); | ||
| expect(global.__NS_HMR_BOOT_COMPLETE__).toBe(true); | ||
| nsModule.setDevBootComplete(false); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specs mutate process-wide loader state without restoring it. Both specs change global ns:module state that later specs and the loader read, and neither restores the prior value. The shared root cause is missing save/restore around process-wide configuration.
test-app/app/src/main/assets/app/tests/testNsModule.js#L35-L44: captureglobal.__NS_HMR_BOOT_COMPLETE__before the spec and restore it at the end instead of forcingfalse.test-app/app/src/main/assets/app/tests/testNsModule.js#L88-L104: move theconfigureLoadercall intobeforeEach/afterEachand re-install the boot-time canonicalization vocabulary after the spec.
📍 Affects 1 file
test-app/app/src/main/assets/app/tests/testNsModule.js#L35-L44(this comment)test-app/app/src/main/assets/app/tests/testNsModule.js#L88-L104
🤖 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/app/src/main/assets/app/tests/testNsModule.js` around lines 35 - 44,
In test-app/app/src/main/assets/app/tests/testNsModule.js lines 35-44, save
global.__NS_HMR_BOOT_COMPLETE__ before the spec and restore that saved value
during cleanup instead of forcing false. In lines 88-104, move configureLoader
into beforeEach/afterEach so each spec restores the prior loader configuration
and re-installs the boot-time canonicalization vocabulary after execution.
Comment on lines
+146
to
+153
| it("should refuse lookalike-host prefixes at a URL-component boundary (Java helper)", function() { | ||
| // The Java helper is the production-path twin of the native gate. | ||
| // Debug still short-circuits to true, so this only asserts the | ||
| // helper exists and debug bypass still holds; production matching | ||
| // is covered by the native RemoteUrlMatchesAllowlistEntry logic. | ||
| expect(typeof com.tns.Runtime.isRemoteUrlAllowed).toBe("function"); | ||
| expect(com.tns.Runtime.isRemoteUrlAllowed("https://cdn.example.com.attacker.com/x.js")).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename this spec; it does not verify refusal.
The spec title claims lookalike-host prefixes are refused at a URL-component boundary. The body asserts the opposite result (true) because debug mode short-circuits isRemoteUrlAllowed. The boundary matching in com.tns.Runtime.isRemoteUrlAllowed is never reached. A reader scanning the spec names gains false confidence in a security control.
Rename the spec to state what it checks, for example "exposes isRemoteUrlAllowed and keeps the debug bypass". If remoteUrlMatchesAllowlistEntry is reachable from JS, add a separate spec that exercises the boundary logic directly.
✏️ Proposed rename
- it("should refuse lookalike-host prefixes at a URL-component boundary (Java helper)", function() {
+ it("exposes the Java isRemoteUrlAllowed helper and keeps the debug bypass", 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
| it("should refuse lookalike-host prefixes at a URL-component boundary (Java helper)", function() { | |
| // The Java helper is the production-path twin of the native gate. | |
| // Debug still short-circuits to true, so this only asserts the | |
| // helper exists and debug bypass still holds; production matching | |
| // is covered by the native RemoteUrlMatchesAllowlistEntry logic. | |
| expect(typeof com.tns.Runtime.isRemoteUrlAllowed).toBe("function"); | |
| expect(com.tns.Runtime.isRemoteUrlAllowed("https://cdn.example.com.attacker.com/x.js")).toBe(true); | |
| }); | |
| it("exposes the Java isRemoteUrlAllowed helper and keeps the debug bypass", function() { | |
| // The Java helper is the production-path twin of the native gate. | |
| // Debug still short-circuits to true, so this only asserts the | |
| // helper exists and debug bypass still holds; production matching | |
| // is covered by the native RemoteUrlMatchesAllowlistEntry logic. | |
| expect(typeof com.tns.Runtime.isRemoteUrlAllowed).toBe("function"); | |
| expect(com.tns.Runtime.isRemoteUrlAllowed("https://cdn.example.com.attacker.com/x.js")).toBe(true); | |
| }); |
🤖 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/app/src/main/assets/app/tests/testRemoteModuleSecurity.js` around
lines 146 - 153, Rename the spec describing com.tns.Runtime.isRemoteUrlAllowed
so its title reflects that it verifies the helper exists and preserves the debug
bypass, not refusal of lookalike-host prefixes. Keep the assertions unchanged;
do not claim boundary matching is tested unless a separate directly reachable
test is added.
Comment on lines
+70
to
+77
| ignoreExitValue = true | ||
| doFirst { | ||
| println "Removing previous android_unit_test_results.xml" | ||
|
|
||
| if (isWinOs) { | ||
| commandLine "cmd", "/c", "adb", runOnDeviceOrEmulator, "-e", "shell", "rm", "-rf", "/data/data/com.tns.testapplication/android_unit_test_results.xml" | ||
| commandLine "cmd", "/c", "adb", runOnDeviceOrEmulator, "-e", "shell", "run-as", "com.tns.testapplication", "rm", "-f", "android_unit_test_results.xml" | ||
| } else { | ||
| commandLine "adb", runOnDeviceOrEmulator, "-e", "shell", "rm", "-rf", "/data/data/com.tns.testapplication/android_unit_test_results.xml" | ||
| commandLine "adb", runOnDeviceOrEmulator, "-e", "shell", "run-as", "com.tns.testapplication", "rm", "-f", "android_unit_test_results.xml" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not ignore result-file cleanup failures.
android_unit_test_results.xml is reused across runs. If run-as returns a non-zero status, ignoreExitValue = true leaves the previous result on the device. test-app/tools/try_to_find_test_result_file.js can then detect that old file before the current test writes a result. Downstream verification can consume stale test data.
Remove ignoreExitValue = true, or verify that the file is absent before starting the test. rm -f already handles a missing file.
Proposed fix
task deletePreviousResultXml(type: Exec) {
- ignoreExitValue = true📝 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
| ignoreExitValue = true | |
| doFirst { | |
| println "Removing previous android_unit_test_results.xml" | |
| if (isWinOs) { | |
| commandLine "cmd", "/c", "adb", runOnDeviceOrEmulator, "-e", "shell", "rm", "-rf", "/data/data/com.tns.testapplication/android_unit_test_results.xml" | |
| commandLine "cmd", "/c", "adb", runOnDeviceOrEmulator, "-e", "shell", "run-as", "com.tns.testapplication", "rm", "-f", "android_unit_test_results.xml" | |
| } else { | |
| commandLine "adb", runOnDeviceOrEmulator, "-e", "shell", "rm", "-rf", "/data/data/com.tns.testapplication/android_unit_test_results.xml" | |
| commandLine "adb", runOnDeviceOrEmulator, "-e", "shell", "run-as", "com.tns.testapplication", "rm", "-f", "android_unit_test_results.xml" | |
| doFirst { | |
| println "Removing previous android_unit_test_results.xml" | |
| if (isWinOs) { | |
| commandLine "cmd", "/c", "adb", runOnDeviceOrEmulator, "-e", "shell", "run-as", "com.tns.testapplication", "rm", "-f", "android_unit_test_results.xml" | |
| } else { | |
| commandLine "adb", runOnDeviceOrEmulator, "-e", "shell", "run-as", "com.tns.testapplication", "rm", "-f", "android_unit_test_results.xml" |
🤖 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/runtests.gradle` around lines 70 - 77, Remove ignoreExitValue = true
from the android_unit_test_results.xml cleanup task so failures from the run-as
removal command stop the flow instead of allowing stale results to remain; keep
the existing rm -f cleanup behavior and platform-specific command handling
unchanged.
Comment on lines
+230
to
+254
| struct CanonicalizationConfig { | ||
| std::vector<std::string> stripParams; | ||
| std::vector<std::string> devPathPrefixes; | ||
| std::vector<std::string> preserveQueryPrefixes; | ||
| }; | ||
| static CanonicalizationConfig g_canonConfig; | ||
| static bool g_canonConfigured = false; | ||
|
|
||
| static void SetCanonicalizationConfig(CanonicalizationConfig config) { | ||
| g_canonConfig = std::move(config); | ||
| g_canonConfigured = true; | ||
| if (IsScriptLoadingLogEnabled()) { | ||
| DEBUG_WRITE_FORCE( | ||
| "[ns:module configureLoader] canonicalization set (strip=%lu devPrefixes=%lu " | ||
| "preserve=%lu)", | ||
| (unsigned long)g_canonConfig.stripParams.size(), | ||
| (unsigned long)g_canonConfig.devPathPrefixes.size(), | ||
| (unsigned long)g_canonConfig.preserveQueryPrefixes.size()); | ||
| } | ||
| } | ||
|
|
||
| static void ResetCanonicalizationConfig() { | ||
| g_canonConfig = CanonicalizationConfig{}; | ||
| g_canonConfigured = false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the canonicalization config against concurrent access.
g_canonConfig and g_canonConfigured are plain globals with no synchronization. Writers and readers run on different threads:
SetCanonicalizationConfigruns on a JS thread fromConfigureLoaderCallback(Line 935).ResetCanonicalizationConfigruns on the main isolate fromCleanupHttpLoaderGlobals(Line 833).CanonicalizeHttpUrlKeyreads both globals (Lines 283-330) and is reached from theFetchModuleBodyAsyncbackground thread throughApplyCacheBustNonce→IsUrlMarkedForCacheBust→CanonicalizeHttpUrlKey(Lines 444, 365, 571, 780).
A configureLoader call during an in-flight prefetch therefore reassigns the std::vector<std::string> members while a background thread iterates them. This is a data race and can read freed string buffers. Note that g_bustNextFetchMutex does not help: the writers never take it.
Publish an immutable snapshot instead, so readers hold a stable copy for the duration of the call.
🔒 Proposed fix using an immutable shared snapshot
-static CanonicalizationConfig g_canonConfig;
-static bool g_canonConfigured = false;
+static std::mutex g_canonConfigMutex;
+static std::shared_ptr<const CanonicalizationConfig> g_canonConfig;
+
+static std::shared_ptr<const CanonicalizationConfig> CurrentCanonicalizationConfig() {
+ std::lock_guard<std::mutex> lock(g_canonConfigMutex);
+ return g_canonConfig;
+}
static void SetCanonicalizationConfig(CanonicalizationConfig config) {
- g_canonConfig = std::move(config);
- g_canonConfigured = true;
+ auto snapshot = std::make_shared<const CanonicalizationConfig>(std::move(config));
+ {
+ std::lock_guard<std::mutex> lock(g_canonConfigMutex);
+ g_canonConfig = snapshot;
+ }
if (IsScriptLoadingLogEnabled()) {
DEBUG_WRITE_FORCE(
"[ns:module configureLoader] canonicalization set (strip=%lu devPrefixes=%lu "
"preserve=%lu)",
- (unsigned long)g_canonConfig.stripParams.size(),
- (unsigned long)g_canonConfig.devPathPrefixes.size(),
- (unsigned long)g_canonConfig.preserveQueryPrefixes.size());
+ (unsigned long)snapshot->stripParams.size(),
+ (unsigned long)snapshot->devPathPrefixes.size(),
+ (unsigned long)snapshot->preserveQueryPrefixes.size());
}
}
static void ResetCanonicalizationConfig() {
- g_canonConfig = CanonicalizationConfig{};
- g_canonConfigured = false;
+ std::lock_guard<std::mutex> lock(g_canonConfigMutex);
+ g_canonConfig.reset();
}Then take one snapshot at the top of CanonicalizeHttpUrlKey and use it in place of g_canonConfigured / g_canonConfig:
const std::shared_ptr<const CanonicalizationConfig> canon = CurrentCanonicalizationConfig();
// ... replace `g_canonConfigured` with `canon != nullptr`
// ... replace `g_canonConfig.` with `canon->`🤖 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/HttpLoader.cpp` around lines 230 - 254, Replace
the unsynchronized globals used by SetCanonicalizationConfig,
ResetCanonicalizationConfig, and CanonicalizeHttpUrlKey with an atomically
published immutable shared snapshot, using the existing project conventions for
atomic shared-pointer access. Publish a new const CanonicalizationConfig on
configure and a null snapshot on reset; have CanonicalizeHttpUrlKey acquire one
snapshot at entry, check it for configuration state, and use that stable
snapshot throughout the call instead of g_canonConfigured or g_canonConfig.
Comment on lines
+700
to
+717
| jbyteArray buffer = env.NewByteArray(8192); | ||
| while (true) { | ||
| jint n = env.CallIntMethod(inStream, readMethod, buffer); | ||
| if (n < 0) break; | ||
| if (n == 0) continue; | ||
| env.CallVoidMethod(baos, baosWrite, buffer, 0, n); | ||
| } | ||
|
|
||
| env.CallVoidMethod(inStream, closeIS); | ||
| jbyteArray bytes = static_cast<jbyteArray>(env.CallObjectMethod(baos, baosToByteArray)); | ||
| env.CallVoidMethod(baos, baosClose); | ||
|
|
||
| if (!bytes) return false; | ||
| jsize len = env.GetArrayLength(bytes); | ||
| out.resize(static_cast<size_t>(len)); | ||
| if (len > 0) { | ||
| env.GetByteArrayRegion(bytes, 0, len, reinterpret_cast<jbyte*>(&out[0])); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check for a pending JNI exception inside the read loop.
env.CallIntMethod(inStream, readMethod, buffer) returns 0 when InputStream.read throws, for example on a mid-body IOException or socket timeout. The loop treats 0 as "no data yet" and calls read again with an exception still pending. Every following call returns 0 the same way, so the loop never terminates and the calling thread hangs. On the synchronous path that thread is the JS thread.
Break out of the loop when an exception is pending, and record it.
🐛 Proposed fix
jbyteArray buffer = env.NewByteArray(8192);
+ bool readFailed = false;
while (true) {
jint n = env.CallIntMethod(inStream, readMethod, buffer);
+ {
+ std::string excClass, excMsg;
+ if (DrainPendingJniException(env, excClass, excMsg)) {
+ RecordLastHttpFetchError("read-body", excClass, excMsg);
+ if (IsScriptLoadingLogEnabled()) {
+ DEBUG_WRITE_FORCE(
+ "[http-esm][fetch][exception] stage=read-body url=%s class=%s msg=%s",
+ url.c_str(), excClass.c_str(), excMsg.c_str());
+ }
+ readFailed = true;
+ break;
+ }
+ }
if (n < 0) break;
if (n == 0) continue;
env.CallVoidMethod(baos, baosWrite, buffer, 0, n);
}
env.CallVoidMethod(inStream, closeIS);
+ if (readFailed) {
+ return false;
+ }🤖 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/HttpLoader.cpp` around lines 700 - 717, Update
the read loop around HttpLoader’s CallIntMethod(inStream, readMethod, buffer) to
check for a pending JNI exception immediately after each read; record the
exception and break before handling n == 0. Preserve normal EOF and successful
reads, while ensuring the recorded exception is propagated or handled by the
surrounding loader flow after cleanup.
Comment on lines
+53
to
85
| static std::string PromiseRejectionMessage(Isolate* isolate, Local<Promise> promise, | ||
| const std::string& path) { | ||
| std::string errorMessage = "Module evaluation promise rejected: " + path; | ||
| Local<Value> reason = promise->Result(); | ||
| if (reason.IsEmpty()) { | ||
| return errorMessage; | ||
| } | ||
| if (reason->IsObject()) { | ||
| Local<Context> context = isolate->GetCurrentContext(); | ||
| Local<Object> errorObj = reason.As<Object>(); | ||
| Local<Value> messageVal; | ||
| if (errorObj->Get(context, ArgConverter::ConvertToV8String(isolate, "message")) | ||
| .ToLocal(&messageVal) && | ||
| messageVal->IsString()) { | ||
| v8::String::Utf8Value messageUtf8(isolate, messageVal); | ||
| if (*messageUtf8) { | ||
| errorMessage.append(" — "); | ||
| errorMessage.append(*messageUtf8); | ||
| } | ||
| } | ||
| } else { | ||
| Local<Context> context = isolate->GetCurrentContext(); | ||
| auto maybeReasonStr = reason->ToString(context); | ||
| if (!maybeReasonStr.IsEmpty()) { | ||
| v8::String::Utf8Value reasonUtf8(isolate, maybeReasonStr.ToLocalChecked()); | ||
| if (*reasonUtf8) { | ||
| errorMessage.append(" — "); | ||
| errorMessage.append(*reasonUtf8); | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| return errorMessage; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the property read with a TryCatch.
Line 64 calls errorObj->Get(...). The rejection reason can be a proxy or an object with a throwing message getter. Then Get schedules an exception that stays pending after this helper returns, and the caller reports a misleading error. Wrap the reads in a local TryCatch so the diagnostic helper never changes the isolate exception state.
🛡️ Proposed fix
static std::string PromiseRejectionMessage(Isolate* isolate, Local<Promise> promise,
const std::string& path) {
std::string errorMessage = "Module evaluation promise rejected: " + path;
Local<Value> reason = promise->Result();
if (reason.IsEmpty()) {
return errorMessage;
}
+ TryCatch tc(isolate);
if (reason->IsObject()) {📝 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
| static std::string PromiseRejectionMessage(Isolate* isolate, Local<Promise> promise, | |
| const std::string& path) { | |
| std::string errorMessage = "Module evaluation promise rejected: " + path; | |
| Local<Value> reason = promise->Result(); | |
| if (reason.IsEmpty()) { | |
| return errorMessage; | |
| } | |
| if (reason->IsObject()) { | |
| Local<Context> context = isolate->GetCurrentContext(); | |
| Local<Object> errorObj = reason.As<Object>(); | |
| Local<Value> messageVal; | |
| if (errorObj->Get(context, ArgConverter::ConvertToV8String(isolate, "message")) | |
| .ToLocal(&messageVal) && | |
| messageVal->IsString()) { | |
| v8::String::Utf8Value messageUtf8(isolate, messageVal); | |
| if (*messageUtf8) { | |
| errorMessage.append(" — "); | |
| errorMessage.append(*messageUtf8); | |
| } | |
| } | |
| } else { | |
| Local<Context> context = isolate->GetCurrentContext(); | |
| auto maybeReasonStr = reason->ToString(context); | |
| if (!maybeReasonStr.IsEmpty()) { | |
| v8::String::Utf8Value reasonUtf8(isolate, maybeReasonStr.ToLocalChecked()); | |
| if (*reasonUtf8) { | |
| errorMessage.append(" — "); | |
| errorMessage.append(*reasonUtf8); | |
| } | |
| } | |
| } | |
| return false; | |
| return errorMessage; | |
| } | |
| static std::string PromiseRejectionMessage(Isolate* isolate, Local<Promise> promise, | |
| const std::string& path) { | |
| std::string errorMessage = "Module evaluation promise rejected: " + path; | |
| Local<Value> reason = promise->Result(); | |
| if (reason.IsEmpty()) { | |
| return errorMessage; | |
| } | |
| TryCatch tc(isolate); | |
| if (reason->IsObject()) { | |
| Local<Context> context = isolate->GetCurrentContext(); | |
| Local<Object> errorObj = reason.As<Object>(); | |
| Local<Value> messageVal; | |
| if (errorObj->Get(context, ArgConverter::ConvertToV8String(isolate, "message")) | |
| .ToLocal(&messageVal) && | |
| messageVal->IsString()) { | |
| v8::String::Utf8Value messageUtf8(isolate, messageVal); | |
| if (*messageUtf8) { | |
| errorMessage.append(" — "); | |
| errorMessage.append(*messageUtf8); | |
| } | |
| } | |
| } else { | |
| Local<Context> context = isolate->GetCurrentContext(); | |
| auto maybeReasonStr = reason->ToString(context); | |
| if (!maybeReasonStr.IsEmpty()) { | |
| v8::String::Utf8Value reasonUtf8(isolate, maybeReasonStr.ToLocalChecked()); | |
| if (*reasonUtf8) { | |
| errorMessage.append(" — "); | |
| errorMessage.append(*reasonUtf8); | |
| } | |
| } | |
| } | |
| return errorMessage; | |
| } |
🤖 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 53 - 85,
Update PromiseRejectionMessage so property reads on the rejection reason are
enclosed in a local v8::TryCatch, covering the errorObj->Get call and its result
handling. Ensure any exception from a proxy or throwing message getter is caught
and does not remain pending on the isolate, while preserving the existing
diagnostic message behavior.
Comment on lines
+342
to
+355
| static void PumpPendingHttpModuleGraph(v8::Isolate* isolate) { | ||
| if (!tns::HasPendingAsyncModuleGraphWork()) { | ||
| return; | ||
| } | ||
| const auto start = std::chrono::steady_clock::now(); | ||
| while (tns::HasPendingAsyncModuleGraphWork()) { | ||
| isolate->PerformMicrotaskCheckpoint(); | ||
| ALooper_pollOnce(10, nullptr, nullptr, nullptr); | ||
| isolate->PerformMicrotaskCheckpoint(); | ||
| if (std::chrono::duration<double>(std::chrono::steady_clock::now() - start).count() > 60.0) { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Long synchronous busy-waits on the JS thread in Runtime.cpp and ModuleInternal.cpp. Both sites resolve asynchronous module work by spinning on the calling thread with ALooper_pollOnce plus microtask checkpoints until a multi-second deadline expires, instead of returning control to the host run loop. On the main thread each site produces an ANR window when the dev server is slow or unreachable. The shared root cause is one design decision: synchronous drain of asynchronous loader work.
test-app/runtime/src/main/cpp/Runtime.cpp#L342-L355: reduce the 60 second bound for the main thread, and log when the deadline expires so the stall is diagnosable.test-app/runtime/src/main/cpp/ModuleInternal.cpp#L659-L680: reduce the 30 second top-level-await bound for the main thread, or return the pending promise to the caller instead of draining it inline.
📍 Affects 2 files
test-app/runtime/src/main/cpp/Runtime.cpp#L342-L355(this comment)test-app/runtime/src/main/cpp/ModuleInternal.cpp#L659-L680
🤖 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` around lines 342 - 355, Reduce the
synchronous async-module drain deadline in PumpPendingHttpModuleGraph in
test-app/runtime/src/main/cpp/Runtime.cpp (lines 342-355) for the main thread
and log when the deadline expires. Also update the top-level-await handling in
test-app/runtime/src/main/cpp/ModuleInternal.cpp (lines 659-680) to reduce its
30-second main-thread bound or return the pending promise instead of draining it
inline.
Comment on lines
+158
to
+178
| if (!isTerminating_ && !isClosing_ && !queue_.IsEmpty()) { | ||
| Local<Value> onMessageValue; | ||
| bool gotHandler = | ||
| globalObject->Get(context, ArgConverter::ConvertToV8String(isolate, "onmessage")) | ||
| .ToLocal(&onMessageValue); | ||
| if (!gotHandler || !onMessageValue->IsFunction()) { | ||
| bool expected = false; | ||
| if (drainRetryPending_.compare_exchange_strong(expected, true)) { | ||
| const int workerId = workerId_; | ||
| std::thread([workerId]() { | ||
| usleep(50 * 1000); | ||
| auto wrapper = WorkerWrapper::GetById(workerId); | ||
| if (wrapper != nullptr) { | ||
| wrapper->drainRetryPending_ = false; | ||
| wrapper->SignalMessageDrain(); | ||
| } | ||
| }).detach(); | ||
| } | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the drain retry, and avoid one thread per retry.
The retry is self-perpetuating. SignalMessageDrain wakes the looper, DrainPendingTasks runs again, and if onmessage is still absent it schedules another retry. For a worker script that never installs onmessage, this repeats every 50 ms for the whole life of the worker. Two consequences follow:
- The worker thread wakes 20 times per second forever, and each wake creates and detaches a new
std::threadwhose only work isusleep. - Queued messages are never drained and never dropped, so
messagesQueue_grows without bound while the sender keeps posting.
The previous behavior dropped the message and logged. The new behavior needs an upper bound.
Cap the number of retries, then fall through to the existing per-message handling that logs and discards. Reuse the runtime's looper for the delay instead of a new thread.
♻️ Proposed fix (bounded retries)
if (!gotHandler || !onMessageValue->IsFunction()) {
bool expected = false;
- if (drainRetryPending_.compare_exchange_strong(expected, true)) {
+ if (drainRetryAttempts_ < kMaxDrainRetryAttempts &&
+ drainRetryPending_.compare_exchange_strong(expected, true)) {
+ ++drainRetryAttempts_;
const int workerId = workerId_;
std::thread([workerId]() {
usleep(50 * 1000);
auto wrapper = WorkerWrapper::GetById(workerId);
if (wrapper != nullptr) {
wrapper->drainRetryPending_ = false;
wrapper->SignalMessageDrain();
}
}).detach();
+ return;
}
- return;
+ if (drainRetryAttempts_ < kMaxDrainRetryAttempts) {
+ return;
+ }
+ // Retry budget exhausted: fall through so the per-message loop
+ // logs the missing handler and drops the messages.
}Add to WorkerWrapper.h:
static constexpr int kMaxDrainRetryAttempts = 40; // ~2s at 50ms
int drainRetryAttempts_ = 0; // looper thread onlyReset drainRetryAttempts_ to 0 once a handler is found.
🤖 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/WorkerWrapper.cpp` around lines 158 - 178,
Bound the retry path in WorkerWrapper::DrainPendingTasks using a
looper-scheduled delay instead of spawning and detaching a std::thread for each
retry. Add the proposed kMaxDrainRetryAttempts and looper-thread-only
drainRetryAttempts_ state, increment attempts while onmessage is unavailable,
and reschedule only below the cap; once the cap is reached, fall through to the
existing per-message logging and discard handling. Reset drainRetryAttempts_ to
zero when a valid onmessage handler is found.
|
|
||
| public Class<?> findClass(String className) throws ClassNotFoundException { | ||
| String canonicalName = className.replace('/', '.'); | ||
| String canonicalName = className.replace('/', '.').replace('$', '_'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\bfindClass\s*\(' \
test-app/runtime/src/main/java test-app/runtime/src/main/cpp test-app/app/src/main/assets || true
rg -n -C 2 '\$[A-Za-z_][A-Za-z0-9_$]*' \
test-app/runtime/src/main/java test-app/app/src/main/assets || trueRepository: NativeScript/android
Length of output: 4151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- DexFactory outline ---'
ast-grep outline test-app/runtime/src/main/java/com/tns/DexFactory.java --view expanded
echo '--- DexFactory relevant implementation ---'
sed -n '1,245p' test-app/runtime/src/main/java/com/tns/DexFactory.java
echo '--- Runtime callers and class-resolution paths ---'
rg -n -C 8 'findClass|resolveClass|loadClass|canonicalName|replace\(.*\\$' \
test-app/runtime/src/main/java/com/tns test-app/runtime/src/main/cpp
echo '--- Repository changes for the target file ---'
git diff -- test-app/runtime/src/main/java/com/tns/DexFactory.java
git status --shortRepository: NativeScript/android
Length of output: 36765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- All findClass references ---'
rg -n -C 5 'findClass|FindClass|RuntimeCallable' . \
-g '!node_modules' -g '!build' -g '!dist'
echo '--- Class-name transformations and generated-name usage ---'
rg -n -C 5 'replace\("\$"|replace\(.*\\\$|com\.tns\.gen|injectedDexClasses|storeClass|retrieveClass|loadClass' \
test-app/runtime test-app/app/src/main/assets \
-g '!build' -g '!dist'
echo '--- Candidate regression tests and test build files ---'
rg -n -C 4 'DexFactory|ClassResolver|nested|Nested|Inner|findClass|resolveClass' \
test-app -g '*Test*' -g '*.java' -g '*.js' -g '*.gradle' -g '*.xml' \
-g '!build' -g '!dist' || true
echo '--- Recent target-file history summary ---'
git log --oneline -8 -- test-app/runtime/src/main/java/com/tns/DexFactory.javaRepository: NativeScript/android
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Direct findClass references only ---'
rg -n -C 6 '\bfindClass\b' \
test-app/runtime/src/main/java test-app/runtime/src/main/cpp \
test-app/app/src/main/assets \
-g '*.java' -g '*.cpp' -g '*.h' -g '*.js' -g '*.ts' \
-g '!**/runtime-binding-generator/**' || true
echo '--- Runtime class lookup entry points ---'
rg -n -C 8 'class\.forName|forName|loadClass|findClass|GetMethodID.*findClass|CALL.*findClass|FindClass' \
test-app/runtime/src/main/cpp test-app/runtime/src/main/java/com/tns \
-g '*.cpp' -g '*.h' -g '*.java' | head -n 500
echo '--- Existing nested-class coverage ---'
sed -n '1,100p' test-app/app/src/main/java/com/tns/tests/OuterClass.java
sed -n '720,780p' test-app/app/src/main/assets/app/tests/tests.js
sed -n '1,180p' test-app/app/src/main/assets/app/tests/testClassForNameDiscovery.js
echo '--- Deterministic name-transformation probe ---'
python3 - <<'PY'
from pathlib import Path
source = Path("test-app/runtime/src/main/java/com/tns/DexFactory.java").read_text()
needle = "String canonicalName = className.replace('/', '.').replace('$', '_');"
assert needle in source, "target transformation is not present"
def find_class_name(name):
return name.replace("/", ".").replace("$", "_")
ordinary = find_class_name("com.example.Outer$Inner")
jni = find_class_name("com/example/Outer$Inner")
proxy = find_class_name("com.tns.gen.com.example.Outer$Inner")
print("ordinary:", ordinary)
print("jni:", jni)
print("proxy:", proxy)
assert ordinary != "com.example.Outer$Inner"
assert jni != "com.example.Outer$Inner"
assert proxy == "com.tns.gen.com.example.Outer_Inner"
resolve_source = source.split("public Class<?> resolveClass", 1)[1].split("public Class<?> findClass", 1)[0]
assert 'className.replace("$", "_")' in resolve_source
print("resolveClass uses underscore proxy naming; findClass also rewrites ordinary nested names")
PY
echo '--- Recent target-file history summary ---'
git log --oneline -8 -- test-app/runtime/src/main/java/com/tns/DexFactory.javaRepository: NativeScript/android
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- CallbackHandlers::FindClass ---'
rg -n -C 20 'CallbackHandlers::FindClass|FindClass\(Isolate' \
test-app/runtime/src/main/cpp/CallbackHandlers.cpp \
test-app/runtime/src/main/cpp/CallbackHandlers.h
echo '--- Runtime cached-class and findClass methods ---'
sed -n '1425,1480p' test-app/runtime/src/main/java/com/tns/Runtime.java
sed -n '1,90p' test-app/runtime/src/main/cpp/JEnv.cpp
sed -n '720,755p' test-app/runtime/src/main/cpp/JEnv.cpp
echo '--- Metadata names for nested classes ---'
rg -n -C 8 'm_name|GetJniClassName|ConvertFromJniToCanonicalName|ClassAccessorGetterCallback|GetTypeMetadata' \
test-app/runtime/src/main/cpp/MetadataNode.cpp \
test-app/runtime/src/main/cpp/Util.cpp \
test-app/runtime/src/main/cpp/CallbackHandlers.cpp \
-g '*.cpp' -g '*.h' | head -n 400
echo '--- Exact class-for-name discovery test ---'
cat test-app/app/src/main/assets/app/tests/testClassForNameDiscovery.jsRepository: NativeScript/android
Length of output: 48052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Exact runtime findClass references ---'
git grep -n -E 'findClass|getCachedClass|RuntimeCallable' -- \
':!test-app/runtime-binding-generator/**' \
':!test-app/runtime/src/main/java/com/tns/Runtime.java' \
':!test-app/runtime/src/main/java/com/tns/DexFactory.java' \
| grep -E 'findClass|getCachedClass' || true
echo '--- RuntimeCallable registration and invocation ---'
git grep -n -C 5 'RuntimeCallable' -- \
test-app/runtime/src/main/java test-app/runtime/src/main/cpp \
| head -n 500
echo '--- Runtime initialization and callable exposure ---'
rg -n -C 10 'RuntimeCallable|runtimeCallable|callable|GetMethodID.*Runtime|Invoke.*Runtime|Runtime::' \
test-app/runtime/src/main/java/com/tns/Runtime.java \
test-app/runtime/src/main/cpp \
-g '*.java' -g '*.cpp' -g '*.h' | head -n 800
echo '--- Target-file commits and target-line introduction ---'
git log --oneline -12 --follow -- test-app/runtime/src/main/java/com/tns/DexFactory.java
git blame -L 190,210 -- test-app/runtime/src/main/java/com/tns/DexFactory.javaRepository: NativeScript/android
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Native method identifiers for Runtime.findClass ---'
rg -n -C 8 'FIND_CLASS|findClass|find_class|FindClass.*Runtime|Runtime.*FindClass' \
test-app/runtime/src/main/cpp \
-g '*.cpp' -g '*.h' || true
echo '--- Runtime invocation tables and method-name dispatch ---'
rg -n -C 8 'GetMethodID|GetStaticMethodID|Call.*Method|methodName|method.*name|RuntimeCallable' \
test-app/runtime/src/main/cpp/Runtime* \
test-app/runtime/src/main/cpp/CallbackHandlers* \
test-app/runtime/src/main/cpp/JEnv* \
-g '*.cpp' -g '*.h' | head -n 1000
echo '--- All source references to the exact Java method name ---'
git grep -n -F '"findClass"' -- \
':!test-app/runtime-binding-generator/**' || true
git grep -n -F 'findClass(' -- \
':!test-app/runtime-binding-generator/**' || true
echo '--- Target-file history output only ---'
git log --oneline -12 --follow -- test-app/runtime/src/main/java/com/tns/DexFactory.java
echo '--- Blame for target method ---'
git blame -L 196,207 -- test-app/runtime/src/main/java/com/tns/DexFactory.javaRepository: NativeScript/android
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Target-file history ---'
git log --oneline -12 --follow -- test-app/runtime/src/main/java/com/tns/DexFactory.java
echo '--- Target-method blame ---'
git blame -L 196,207 -- test-app/runtime/src/main/java/com/tns/DexFactory.java
echo '--- RuntimeCallable references outside runtime Java ---'
git grep -n 'RuntimeCallable' -- \
':!test-app/runtime/src/main/java/**' \
':!test-app/runtime-binding-generator/**' || true
echo '--- Runtime-call bridge source names ---'
git ls-files | rg -i 'runtime.*(call|method)|call.*runtime|metadata.*runtime|runtime.*metadata'Repository: NativeScript/android
Length of output: 6042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Introducing commit summary ---'
git show --stat --oneline --no-renames 20aa9322
echo '--- Introducing commit changes for DexFactory.java ---'
git show --format=fuller --no-ext-diff --unified=20 \
20aa9322 -- test-app/runtime/src/main/java/com/tns/DexFactory.java
echo '--- Parent and current implementations of findClass ---'
git show 20aa9322^:test-app/runtime/src/main/java/com/tns/DexFactory.java \
| sed -n '188,212p'
sed -n '190,212p' test-app/runtime/src/main/java/com/tns/DexFactory.javaRepository: NativeScript/android
Length of output: 5912
Preserve $ in DexFactory.findClass.
Convert / to . but keep $ unchanged before classLoader.loadClass. The current conversion maps com/tns/tests/OuterClass$InnerClass to com.tns.tests.OuterClass_InnerClass, which breaks ordinary nested-class loading. Scope underscore normalization to generated-proxy lookup and add regression coverage for both cases.
🤖 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/java/com/tns/DexFactory.java` at line 197, Update
DexFactory.findClass so canonicalName only replaces '/' with '.', preserving '$'
for ordinary nested-class loading before classLoader.loadClass. Apply underscore
normalization only within generated-proxy lookup, and add regression coverage
for both nested-class loading and proxy-name normalization.
Comment on lines
+140
to
+144
| const text = fs.readFileSync("android_unit_test_results.xml", "utf8"); | ||
| if (text.trimStart().startsWith("<?xml")) { | ||
| console.log("Tests results file found!"); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Parse the result before returning success.
Both branches treat startsWith("<?xml") as complete validation. A pull or run-as cat can observe the file while boot.js is still writing it. The function then exits successfully on a truncated document. test-app/tools/check_if_tests_passed.js later receives a non-parseable result.
Reuse the existing XML parser and exit only after parsing succeeds. Require the testsuites root if this function promises a verifier-ready artifact. The current check also rejects valid XML without an XML declaration.
Also applies to: 156-159
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require("child_process")
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
🤖 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/tools/try_to_find_test_result_file.js` around lines 140 - 144,
Update the result validation in try_to_find_test_result_file to parse the file
with the existing XML parser before calling process.exit(0), and require a
testsuites root so only verifier-ready artifacts succeed. Replace the
startsWith("<?xml") check in both branches, preserving retry behavior when
parsing or root validation fails and accepting valid XML without an XML
declaration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Framework-agnostic hot module replacement on Android with native ES modules: the device fetches modules over HTTP from the Vite dev server and applies hot updates without restarting the process.
The runtime's entire dev surface is one namespace object,
globalThis.__NS_DEV__, carrying primitives, each traceable to a V8-embedder or OS constraint:configureRuntime(config)ResolveModuleCallbackinvalidateModules(urls)v8::Modulerecords + prewarm cache + arms a CFNetwork cache-bust noncekickstartPrefetch(urls)getLoadedModuleUrls()setDevBootComplete(bool)Every HMR policy decision, boot orchestration,
import.meta.hot, full reload, CSS apply, the WebSocket protocol is JS in@nativescript/vite. No native dev-session state machine, no reload orchestration, no hot-callback registries exist in the runtime, and on-device tests pin both the present members and the absent globals.Summary by CodeRabbit
New Features
ns:moduleAPIs for loader configuration, module invalidation, loaded-module inspection, and development boot control.ns:runtimeAPIs for reading and updating supported runtime settings.Bug Fixes