feat(workers): move worker threading and messaging to C++, mirroring the iOS runtime, and support SharedArrayBuffer#1972
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors worker lifecycle to Looper-backed native threads, adds V8 structured-clone messaging with SharedArrayBuffer tracking, implements eventfd/ALooper-backed task/message queues, introduces WorkerWrapper for lifecycle and messaging, updates runtime cache synchronization and Java lifecycle APIs, and adds SharedArrayBuffer tests. ChangesWorker Thread Architecture and Message System Refactoring
Sequence diagram (high-level parent ↔ worker flow): sequenceDiagram
participant JSMain as Parent Runtime
participant WorkerWrapper as Native WorkerWrapper
participant WorkerIsolate as Worker V8 Isolate
JSMain->>WorkerWrapper: PostMessage(worker::Message)
WorkerWrapper->>WorkerIsolate: onmessage({data})
WorkerIsolate->>WorkerWrapper: PostMessageToParent(worker::Message)
WorkerWrapper->>JSMain: invoke parent worker.onmessage({data})
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
… a C++ implementation mirroring iOS Previously each Worker ran on a Java HandlerThread and every postMessage was stringified to JSON in JS, marshalled to Java as a String, routed between threads with android.os.Handler/Message, and re-entered V8 through five JNI callbacks. Workers could only be created from the main thread, and SharedArrayBuffer could not cross threads at all. Worker threading and lifecycle now live in C++ (WorkerWrapper, modeled on the iOS runtime's class of the same name): - WorkerWrapper owns a detached std::thread per worker that holds a shared_ptr to the wrapper until shutdown. The thread attaches to the JVM, calls Runtime.initWorkerRuntime (which prepares a Java Looper before the native runtime binds its fds, and creates the worker's Java Runtime with a Looper-backed scheduler so Android Handlers and cross-thread Java->JS calls keep working), runs the worker script, drains messages posted before bootstrap finished, then blocks in Looper.loop(). A single looper per worker pumps both Java messages and the native fds. - Messaging no longer round-trips through Java or JSON. Payloads are serialized with v8::ValueSerializer (structured clone): ArrayBuffers and typed arrays are deep-copied; SharedArrayBuffer contents are shared across isolates via their backing stores; Java proxy host objects are recreated as plain objects. parent->worker delivery uses a ConcurrentQueue woken through an eventfd on the worker's looper; worker->parent delivery (messages, onerror, cleanup) posts to the parent runtime's new LooperTasks queue - the Android analogue of the iOS runtime's ExecuteOnRunLoop. LooperTasks is held via weak_ptr by children, so posts to an already-destroyed parent are dropped safely. - The Worker constructor is attached to every isolate's global, not just the main one, enabling nested workers (parity with iOS). The worker registry (replacing CallbackHandlers::id2WorkerMap) is mutex-guarded, and terminating a worker - or disposing its isolate - cascades termination to its children. - New `androidPriority` Worker option: accepts THREAD_PRIORITY_* names or an integer nice value clamped to [-20, 19]; defaults to THREAD_PRIORITY_BACKGROUND (10), the previously hardcoded value. - Runtime's id->runtime and isolate->runtime caches are now mutex-guarded since runtimes are created/destroyed off the main thread, each Runtime owns a LooperTasks bound to its looper, and a new WORKER_WRAPPER isolate-data slot resolves a worker isolate back to its wrapper. SharedArrayBuffer is also now accepted in JS->Java argument marshalling (JsArgConverter, JsArgToArrayConverter, JSToJavaObjectsConverter): it converts to a direct ByteBuffer over the same memory, like ArrayBuffer. Covered by the new shared-array-buffer-test.js (conversion, view offset/length, zero-copy sharing). Removed now-dead plumbing: the runWorker JNI entry point, the WorkerGlobalOnMessageCallback / WorkerObjectOnMessageCallback / TerminateWorkerCallback / ClearWorkerPersistent / CallWorkerObjectOnErrorHandleMain JNI callbacks, WorkerThreadHandler, pendingWorkerMessages, workerIdToHandler, MessageType.java and JavaScriptErrorMessage.java.
edusperoni
force-pushed
the
feat/worker-debug
branch
from
04a92cb to
9859f98
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-app/runtime/src/main/cpp/CallbackHandlers.cpp (1)
1200-1210:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate the receiver before reading the private
workerId.Both prototype methods assume
args.This()is a realWorkerinstance.Worker.prototype.postMessage.call({}, ...)or.terminate.call({})leavesjsIdunset, and the unconditionalInt32Value(...).ToChecked()turns that into a native crash/assert rather than a JSTypeError.Also applies to: 1305-1312
🤖 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 1200 - 1210, The code reads the private "workerId" unconditionally and calls jsId->Int32Value(...).ToChecked(), causing native crashes when args.This() isn't a real Worker; instead, validate the receiver and the result of V8GetPrivateValue before converting: check the return (maybejsId) or whether jsId is empty and whether jsId->IsInt32() (or ToInt32 succeeded); if validation fails, throw a JS TypeError via the current isolate/context and return early. Update both the block around V8GetPrivateValue/Int32Value (where jsId and id are used) and the similar block at lines 1305-1312 to perform these checks and use ArgConverter::ConvertToV8String for the private key as before.
🤖 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/shared-array-buffer-test.js`:
- Around line 62-74: The test can hang because it only calls done() from
worker.onmessage; add a worker error/timeout path: attach worker.onerror and
worker.onmessageerror handlers that call done.fail(...) (or fail the test) and
terminate the worker, and implement a short watchdog timer (clear it on success)
that fails the spec and terminates the worker if no message/error arrives within
e.g. a few seconds; update the block that creates
Worker("../shared/Workers/EvalWorker.js"), worker.postMessage(...), and
worker.onmessage to also set up these handlers and the timeout so the spec
always completes on error or timeout.
In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`:
- Around line 995-1052: GetWorkerThreadPriority currently only recognizes custom
lowercase aliases for options.androidPriority and throws for Android constants;
update the string-handling branch in GetWorkerThreadPriority to accept
documented THREAD_PRIORITY_* names (e.g., "THREAD_PRIORITY_BACKGROUND",
"THREAD_PRIORITY_FOREGROUND", etc.) by normalizing the input (trim, uppercase)
and stripping an optional "THREAD_PRIORITY_" prefix before comparing against the
existing aliases
("LOWEST","BACKGROUND","LESSFAVORABLE","DEFAULT","MOREFAVORABLE","FOREGROUND","DISPLAY","URGENTDISPLAY","VIDEO","AUDIO","URGENTAUDIO");
map those normalized names to the same numeric values the existing lowercase
checks return so callers passing either form succeed.
In `@test-app/runtime/src/main/cpp/ConcurrentQueue.cpp`:
- Around line 23-29: Push() can enqueue messages after Terminate() tears down
the wakeup FD causing stranded messages; fix by introducing and checking a
lifecycle flag under the same mutex used in Push (e.g., add a bool like
terminating_ or shutdown_ protected by mutex_), have
Push(std::shared_ptr<worker::Message>) check that flag while holding mutex_ and
reject/drop (or return an error) instead of pushing into messagesQueue_ when
shutdown is in progress, and modify Terminate() to acquire the same mutex_, set
the flag, clear messagesQueue_ (or drain it safely), then call SignalAndWakeUp()
so no messages remain stranded; apply the same pattern to the code paths
referenced in 54-69.
- Around line 18-20: ConcurrentQueue::Initialize and LooperTasks::Initialize
must check and handle failures from eventfd() and ALooper_addFd(): after calling
eventfd(), verify the returned fd is >= 0 and on error return/fail
initialization and do not proceed; after calling ALooper_addFd(), check its
return (success/failure) and if it fails, close the created fd and call
ALooper_release() for the looper acquired earlier before returning an error;
ensure no path leaves a valid fd or held looper without a registered callback
(i.e., on any failure unwind by closing fd_ and releasing looper_), propagate
the failure status to the caller so queued work cannot silently accumulate.
In `@test-app/runtime/src/main/cpp/WorkerMessage.cpp`:
- Around line 29-33: The current WriteHostObject
(SerializerDelegate::WriteHostObject) and DeserializerDelegate::ReadHostObject
silently accept host objects and materialize empty objects; instead, detect
unsupported host objects and surface a DataCloneError via the V8 isolate and
return Nothing<bool>() from WriteHostObject (and return Nothing/throw in
ReadHostObject) per the V8 delegate contract. Update WriteHostObject to not
return Just(true) when the object is not serializable: call the isolate
exception throw helper for a DataCloneError (or v8 equivalent) and return
Nothing<bool>(). Likewise update ReadHostObject to throw the same DataCloneError
and return the appropriate Nothing result rather than creating a blank object.
Ensure you reference the existing SerializerDelegate::WriteHostObject and
DeserializerDelegate::ReadHostObject symbols when making these changes.
In `@test-app/runtime/src/main/cpp/WorkerMessage.h`:
- Around line 11-14: Malloc<T> currently allocates n bytes but callers (and
UncheckedRealloc) treat n as an element count, causing under-allocation for
non-char T; modify the template Malloc<T> to allocate n * sizeof(T) bytes (i.e.,
multiply n by sizeof(T)) and make the same change for the other Malloc
overloads/usages referenced (see UncheckedRealloc and MallocedBuffer<T>) so all
allocations use element count * sizeof(T) consistently.
In `@test-app/runtime/src/main/cpp/WorkerWrapper.cpp`:
- Around line 54-69: PostMessageToParent currently only checks isTerminating_,
so messages can still be posted after Close() begins; update
WorkerWrapper::PostMessageToParent to also check the "closing" state (the flag
set by Close(), e.g., isClosing_ or whatever Close() toggles) and return early
if either isClosing_ or isTerminating_ is true, and apply the same check to the
other PostMessageToParent overload/duplicate referenced (lines 92-102) so both
paths drop pending messages once Close() has started.
- Around line 274-299: The code currently stops at DEBUG_WRITE when no onerror
handler exists or it returns falsy, which silently swallows the worker failure;
update the path after the callback check in WorkerWrapper.cpp (symbols:
hasOnError, errEvent, callback, DEBUG_WRITE) to surface the error to the parent
runtime instead of only logging: build a V8 exception/value from
message/stackTrace/filename/lineno (e.g. Exception::Error or a proper Error
object) and either throw it into the current isolate
(isolate->ThrowException(...)) or invoke the parent error propagation mechanism
so the application receives the uncaught worker error; keep the existing
DEBUG_WRITE but ensure you call the chosen propagation (throw or host error
dispatch) when result is empty or falsy.
- Around line 333-356: The teardown can run with a null isolate if exceptions
occur before workerIsolate_ is published; after calling
Runtime::GetRuntime(runtimeId) and obtaining Isolate* isolate, immediately
publish it via workerIsolate_.store(isolate) (before calling MY_LOOPER /
NewGlobalRef / queue_.Initialize) and/or make the shutdown path defensively
check workerIsolate_.load() for null before entering v8::Locker / Isolate::Scope
/ Dispose(); update references to Runtime::GetRuntime, workerIsolate_.store,
MY_LOOPER/NewGlobalRef, queue_.Initialize, and the teardown that uses
v8::Locker/Isolate::Scope/Dispose accordingly.
In `@test-app/runtime/src/main/java/com/tns/Runtime.java`:
- Around line 488-493: The bootstrap path is registering a Runtime into
currentRuntime and runtimeCache before init() and
runScript("internal/ts_helpers.js") complete, causing leaked/stale runtimes if
bootstrap throws; update initializeRuntimeWithConfiguration/initRuntime to
either move the cache registration (currentRuntime and runtimeCache) until after
init() and runScript() succeed, or add a rollback in initRuntime that catches
bootstrap exceptions and removes the partially-registered entries
(currentRuntime and runtimeCache) and unsubscribes any GcListener/worker hooks
(and ensures detachWorkerRuntime is invoked for worker startup failure) so
failed startup cannot leave a discoverable dead runtime.
- Around line 488-491: The initializeRuntimeWithConfiguration method currently
constructs WorkThreadScheduler with Looper.myLooper(), which allows callers on
background loopers to bind the runtime to the wrong looper; change it to verify
the caller is on the real main looper and use Looper.getMainLooper() to build
the WorkThreadScheduler. Specifically, in initializeRuntimeWithConfiguration
check Thread/Looper identity and throw an IllegalStateException (or similar) if
Looper.myLooper() != Looper.getMainLooper(), then create new
WorkThreadScheduler(new Handler(Looper.getMainLooper())) so
getMainThreadHandler() and isNotOnMainThread() consistently reference the true
main looper.
---
Outside diff comments:
In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`:
- Around line 1200-1210: The code reads the private "workerId" unconditionally
and calls jsId->Int32Value(...).ToChecked(), causing native crashes when
args.This() isn't a real Worker; instead, validate the receiver and the result
of V8GetPrivateValue before converting: check the return (maybejsId) or whether
jsId is empty and whether jsId->IsInt32() (or ToInt32 succeeded); if validation
fails, throw a JS TypeError via the current isolate/context and return early.
Update both the block around V8GetPrivateValue/Int32Value (where jsId and id are
used) and the similar block at lines 1305-1312 to perform these checks and use
ArgConverter::ConvertToV8String for the private key as before.
🪄 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: a4db806d-b4e2-4ac1-b632-a81e5dbb61c1
📒 Files selected for processing (23)
test-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/shared-array-buffer-test.jstest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/CallbackHandlers.cpptest-app/runtime/src/main/cpp/CallbackHandlers.htest-app/runtime/src/main/cpp/ConcurrentQueue.cpptest-app/runtime/src/main/cpp/ConcurrentQueue.htest-app/runtime/src/main/cpp/JsArgConverter.cpptest-app/runtime/src/main/cpp/JsArgToArrayConverter.cpptest-app/runtime/src/main/cpp/LooperTasks.cpptest-app/runtime/src/main/cpp/LooperTasks.htest-app/runtime/src/main/cpp/MethodCache.cpptest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/Runtime.htest-app/runtime/src/main/cpp/WorkerMessage.cpptest-app/runtime/src/main/cpp/WorkerMessage.htest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/cpp/WorkerWrapper.htest-app/runtime/src/main/cpp/com_tns_Runtime.cpptest-app/runtime/src/main/cpp/conversions/objects/JSToJavaObjectsConverter.cpptest-app/runtime/src/main/java/com/tns/JavaScriptErrorMessage.javatest-app/runtime/src/main/java/com/tns/MessageType.javatest-app/runtime/src/main/java/com/tns/Runtime.java
💤 Files with no reviewable changes (3)
- test-app/runtime/src/main/java/com/tns/MessageType.java
- test-app/runtime/src/main/cpp/com_tns_Runtime.cpp
- test-app/runtime/src/main/java/com/tns/JavaScriptErrorMessage.java
| var worker = new Worker("../shared/Workers/EvalWorker.js"); | ||
| worker.postMessage({ | ||
| value: sab, | ||
| eval: "var v = new Int32Array(value); v[0] = 1234; postMessage('written');" | ||
| }); | ||
| worker.onmessage = function (msg) { | ||
| expect(msg.data).toBe("written"); | ||
| // the worker's write is visible both to this isolate and to Java | ||
| expect(view[0]).toBe(1234); | ||
| expect(javaBuf.getInt(0)).toBe(1234); | ||
| worker.terminate(); | ||
| done(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file explicitly
FILE="test-app/app/src/main/assets/app/tests/shared-array-buffer-test.js"
ls -la "$FILE"
# Show the relevant section around lines 50-90
nl -ba "$FILE" | sed -n '40,110p'
# Search in repo for other references to this test or similar patterns
rg -n "shared-array-buffer-test\.js|EvalWorker\.js|new Worker\\(\"\\.\\./shared/Workers/EvalWorker\.js\"\\)" -S .
# Inspect EvalWorker.js for any messaging/error behavior
WORKER="test-app/app/src/main/assets/app/tests/shared/Workers/EvalWorker.js"
ls -la "$WORKER" || true
if [ -f "$WORKER" ]; then
nl -ba "$WORKER" | sed -n '1,220p'
fi
# Find any test harness utilities: "done", timeouts, or expect patterns
rg -n "function \\(msg\\)|onmessage|onerror|setTimeout\\(|jest\\.setTimeout|done\\(" -S test-app/app/src/main/assets/app/tests | headRepository: NativeScript/android
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test-app/app/src/main/assets/app/tests/shared-array-buffer-test.js"
ls -la "$FILE"
nl -ba "$FILE" | sed -n '40,110p'
rg -n "shared-array-buffer-test\\.js|EvalWorker\\.js|new Worker\\(\"\\.\\./shared/Workers/EvalWorker\\.js\"\\)" -S .
WORKER="test-app/app/src/main/assets/app/tests/shared/Workers/EvalWorker.js"
ls -la "$WORKER" || true
if [ -f "$WORKER" ]; then
nl -ba "$WORKER" | sed -n '1,260p'
fi
rg -n "onerror|addEventListener\\(['\"]error['\"]|setTimeout\\(|jest\\.setTimeout|done\\(" -S test-app/app/src/main/assets/app/tests | head -n 80Repository: NativeScript/android
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test-app/app/src/main/assets/app/tests/shared-array-buffer-test.js"
ls -la "$FILE"
nl -ba "$FILE" | sed -n '40,110p'
echo "---- searching for worker error handling patterns ----"
rg -n "onerror|addEventListener\\(['\"]error['\"]|addEventListener\\(\\s*\"error\"|onmessageerror|messageerror|worker\\.onerror|worker\\.addEventListener\\(['\"]error['\"]" -S test-app/app/src/main/assets/app/tests/shared test-app/app/src/main/assets/app/tests | head -n 200 || true
echo "---- inspect EvalWorker.js ----"
WORKER="test-app/app/src/main/assets/app/tests/shared/Workers/EvalWorker.js"
ls -la "$WORKER" || true
if [ -f "$WORKER" ]; then
nl -ba "$WORKER" | sed -n '1,260p'
fi
echo "---- search for similar tests that add onerror/timeout ----"
rg -n "new Worker\\(|postMessage\\(|terminate\\(\\);\\s*\\n\\s*done\\(|worker\\.terminate\\(\\)" -S test-app/app/src/main/assets/app/tests | head -n 200Repository: NativeScript/android
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test-app/app/src/main/assets/app/tests/shared-array-buffer-test.js"
WORKER="test-app/app/src/main/assets/app/tests/shared/Workers/EvalWorker.js"
echo "== file exists =="
ls -la "$FILE"
echo "== shared-array-buffer-test.js (lines 1-170) =="
awk 'NR>=1 && NR<=170 { printf "%6d %s\n", NR, $0 }' "$FILE"
echo "== shared-array-buffer-test.js (lines 40-110) =="
awk 'NR>=40 && NR<=110 { printf "%6d %s\n", NR, $0 }' "$FILE"
echo "== search for worker error handling / timeout in this test folder =="
rg -n "onerror|addEventListener\\(['\"]error['\"]\\)|onmessageerror|messageerror|setTimeout\\(|jest\\.setTimeout|timeout" \
-S test-app/app/src/main/assets/app/tests/shared-array-buffer-test.js test-app/app/src/main/assets/app/tests/shared || true
echo "== inspect EvalWorker.js =="
if [ -f "$WORKER" ]; then
ls -la "$WORKER"
awk 'NR>=1 && NR<=220 { printf "%6d %s\n", NR, $0 }' "$WORKER"
else
echo "MISSING: $WORKER"
fi
echo "== search for other shared-array-buffer tests with worker.onerror =="
rg -n "new Worker\\(|worker\\.onerror|addEventListener\\(['\"]error['\"]\\).*Worker|onmessageerror" \
-S test-app/app/src/main/assets/app/tests || trueRepository: NativeScript/android
Length of output: 5318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate EvalWorker.js by filename =="
fd -a -u -i "EvalWorker.js" . || true
echo "== locate any Workers/EvalWorker* =="
fd -a -u -i "*/EvalWorker.js" . || true
echo "== list directories named shared under this assets/tests =="
fd -a -u -i -t d "shared" test-app/app/src/main/assets/app/tests . || true
echo "== search for onerror handling in test-app/app/src/main/assets/app/tests =="
rg -n "worker\\.(onerror|addEventListener\\(['\"]error['\"])|onmessageerror|onerror\\s*=" \
test-app/app/src/main/assets/app/tests -S || true
echo "== inspect referenced worker in testWebAssembly.js for error handling patterns =="
FILE2="test-app/app/src/main/assets/app/tests/testWebAssembly.js"
if [ -f "$FILE2" ]; then
awk 'NR>=40 && NR<=120 { printf "%6d %s\n", NR, $0 }' "$FILE2"
else
echo "Missing $FILE2"
fiRepository: NativeScript/android
Length of output: 2324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a -u -i "EvalWorker.js" . || trueRepository: NativeScript/android
Length of output: 46
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate EvalWorker.js by filename =="
fd -a -u -i "EvalWorker.js" . || true
echo "== locate any file containing 'EvalWorker' =="
rg -n "EvalWorker" -S . | head -n 50 || true
echo "== search for worker error handling patterns in assets/app/tests =="
rg -n "onerror|onmessageerror|worker\\.onerror|addEventListener\\(['\"]error['\"]\\)" \
-S test-app/app/src/main/assets/app/tests | head -n 200 || true
echo "== inspect testWebAssembly.js around worker setup =="
FILE2="test-app/app/src/main/assets/app/tests/testWebAssembly.js"
if [ -f "$FILE2" ]; then
awk 'NR>=45 && NR<=110 { printf "%6d %s\n", NR, $0 }' "$FILE2"
fiRepository: NativeScript/android
Length of output: 1752
🏁 Script executed:
# no-op placeholderRepository: NativeScript/android
Length of output: 46
Add a worker error/timeout path so this spec can’t hang.
The async test ("should share a SharedArrayBuffer's memory with a worker and a Java buffer at once") calls done() only from worker.onmessage; there’s no worker.onerror/onmessageerror handler (and EvalWorker.js isn’t present to inspect any built-in handling). If worker startup or postMessage fails before onmessage, the spec will wait for the harness timeout instead of failing immediately.
🤖 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/shared-array-buffer-test.js` around
lines 62 - 74, The test can hang because it only calls done() from
worker.onmessage; add a worker error/timeout path: attach worker.onerror and
worker.onmessageerror handlers that call done.fail(...) (or fail the test) and
terminate the worker, and implement a short watchdog timer (clear it on success)
that fails the spec and terminates the worker if no message/error arrives within
e.g. a few seconds; update the block that creates
Worker("../shared/Workers/EvalWorker.js"), worker.postMessage(...), and
worker.onmessage to also set up these handlers and the timeout so the spec
always completes on error or timeout.
| static int GetWorkerThreadPriority(Isolate *isolate, Local<Context> context, | ||
| const v8::FunctionCallbackInfo<v8::Value> &args) { | ||
| const int defaultPriority = 10; // android.os.Process.THREAD_PRIORITY_BACKGROUND | ||
|
|
||
| if (args.Length() < 2 || !args[1]->IsObject()) { | ||
| return defaultPriority; | ||
| } | ||
|
|
||
| auto options = args[1].As<Object>(); | ||
| Local<Value> value; | ||
| if (!options->Get(context, ArgConverter::ConvertToV8String(isolate, "androidPriority")) | ||
| .ToLocal(&value) || | ||
| value->IsNullOrUndefined()) { | ||
| return defaultPriority; | ||
| } | ||
|
|
||
| if (value->IsNumber()) { | ||
| int priority = value->Int32Value(context).FromMaybe(defaultPriority); | ||
| if (priority < -20) { | ||
| priority = -20; | ||
| } else if (priority > 19) { | ||
| priority = 19; | ||
| } | ||
| return priority; | ||
| } | ||
|
|
||
| if (value->IsString()) { | ||
| auto name = ArgConverter::ConvertToString(value.As<String>()); | ||
| if (name == "lowest") { | ||
| return 19; | ||
| } else if (name == "background") { | ||
| return 10; | ||
| } else if (name == "lessFavorable") { | ||
| return 1; | ||
| } else if (name == "default") { | ||
| return 0; | ||
| } else if (name == "moreFavorable") { | ||
| return -1; | ||
| } else if (name == "foreground") { | ||
| return -2; | ||
| } else if (name == "display") { | ||
| return -4; | ||
| } else if (name == "urgentDisplay") { | ||
| return -8; | ||
| } else if (name == "video") { | ||
| return -10; | ||
| } else if (name == "audio") { | ||
| return -16; | ||
| } else if (name == "urgentAudio") { | ||
| return -19; | ||
| } | ||
| } | ||
|
|
||
| throw NativeScriptException( | ||
| "Invalid value for the Worker 'androidPriority' option. Expected one of: " | ||
| "'lowest', 'background', 'lessFavorable', 'default', 'moreFavorable', " | ||
| "'foreground', 'display', 'urgentDisplay', 'video', 'audio', 'urgentAudio' " | ||
| "or a number between -20 and 19."); |
There was a problem hiding this comment.
Accept the documented THREAD_PRIORITY_* names too.
This parser only recognizes custom aliases like background and urgentDisplay, but the new worker option is described as accepting Android THREAD_PRIORITY_* names. Callers passing THREAD_PRIORITY_BACKGROUND will currently get a runtime exception instead of the advertised behavior.
🤖 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 995 - 1052,
GetWorkerThreadPriority currently only recognizes custom lowercase aliases for
options.androidPriority and throws for Android constants; update the
string-handling branch in GetWorkerThreadPriority to accept documented
THREAD_PRIORITY_* names (e.g., "THREAD_PRIORITY_BACKGROUND",
"THREAD_PRIORITY_FOREGROUND", etc.) by normalizing the input (trim, uppercase)
and stripping an optional "THREAD_PRIORITY_" prefix before comparing against the
existing aliases
("LOWEST","BACKGROUND","LESSFAVORABLE","DEFAULT","MOREFAVORABLE","FOREGROUND","DISPLAY","URGENTDISPLAY","VIDEO","AUDIO","URGENTAUDIO");
map those normalized names to the same numeric values the existing lowercase
checks return so callers passing either form succeed.
| Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object) override { | ||
| // Host objects (e.g. Java proxies) carry no transferable native state; | ||
| // they are recreated as plain objects on the receiving side. | ||
| return Just(true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In V8's ValueSerializer::Delegate API, is it valid to return Just(true)fromWriteHostObject without encoding host-object state, and what is the expected behavior for unsupported host objects during structured cloning?
💡 Result:
In V8's ValueSerializer::Delegate API, returning Just(true) from WriteHostObject without encoding any additional data is technically valid in terms of type signature, but it is incorrect behavior because it fails to represent the host object in the serialized output [1][2][3]. The WriteHostObject method is responsible for serializing the host object's state [1][3]. When you return Just(true), the ValueSerializer assumes that the host object has been successfully serialized into the buffer (typically via the serializer's own writing methods, such as WriteUint32 or WriteRawBytes) [2][4]. If you return Just(true) without having written any bytes for that object, the resulting serialized data will be corrupted or incomplete, leading to issues during deserialization [2][3]. Regarding unsupported host objects, the expected behavior is for the delegate to signal an error [1][3]. If an embedder cannot serialize a specific host object, it must: 1. Throw a suitable exception (typically a DataCloneError) within the Isolate [1][3][5]. 2. Return Nothing to indicate failure to the serializer [1][2][3]. If a delegate is not provided or if the delegate does not handle the object, the V8 ValueSerializer will throw a DataCloneError automatically [6][2][7]. Therefore, for unsupported objects, you should explicitly trigger this error and return Nothing to ensure the structured clone process terminates correctly [1][3][5].
Citations:
- 1: https://v8.github.io/api/head/classv8_1_1ValueSerializer_1_1Delegate.html
- 2: https://github.com/v8/v8/blob/ffdfdbb8/src/objects/value-serializer.cc
- 3: https://chromium.googlesource.com/v8/v8/+/ec06bb6ce5641cf65e400ec55b7421f87d04b999/include/v8-value-serializer.h
- 4: https://chromium.googlesource.com/v8/v8/+/ec06bb6ce5641cf65e400ec55b7421f87d04b999/src/objects/value-serializer.h
- 5: vimokoshi/browser@c3c4653
- 6: https://chromium.googlesource.com/v8/v8/+/65c54c7ac8483e2d7eaec4205e1b196c76b45d34/src/value-serializer.cc
- 7: https://chromium.googlesource.com/v8/v8/+/b0c05f4ab351a0a595c551489e08fec83be75d11/src/value-serializer.cc
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test-app/runtime/src/main/cpp/WorkerMessage.cpp"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== Locations: WriteHostObject / ReadHostObject ==="
rg -n "WriteHostObject|ReadHostObject" "$FILE" || true
echo
echo "=== Context around WriteHostObject (lines ~1-120) ==="
nl -ba "$FILE" | sed -n '1,160p'
echo
echo "=== Context around ReadHostObject (lines ~1-220) ==="
nl -ba "$FILE" | sed -n '1,260p'Repository: NativeScript/android
Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test-app/runtime/src/main/cpp/WorkerMessage.cpp"
echo "== File stats =="
wc -l "$FILE"
echo
echo "== WriteHostObject context (approx. 1-110) =="
cat -n "$FILE" | sed -n '1,110p'
echo
echo "== ReadHostObject context (approx. 55-140) =="
cat -n "$FILE" | sed -n '55,160p'Repository: NativeScript/android
Length of output: 9048
🌐 Web query:
V8 ValueSerializer::Delegate::WriteHostObject return Just(true) semantics; does it require writing clone data via serializer delegate? what triggers DataCloneError for unsupported host objects?
💡 Result:
In the V8 ValueSerializer::Delegate API, the WriteHostObject method is invoked by the serializer when it encounters an object that it cannot serialize using its default mechanisms (such as a JS object with internal embedder fields or specific types configured via SetTreatArrayBufferViewsAsHostObjects) [1][2][3]. Semantics of WriteHostObject return values: * Returning Just(true) signifies that the host has successfully handled the serialization of the object [1][2]. When returning Just(true), the embedder is expected to have written the necessary representation of that object into the serializer's buffer using the provided writing methods (e.g., WriteRawBytes, WriteUint32, WriteDouble) [4][3]. * Returning Nothing() (typically accompanied by throwing a JavaScript exception in the isolate) indicates that the host failed to serialize the object, or that the object is not supported [5][4]. Regarding DataCloneError: * A DataCloneError is triggered when the ValueSerializer encounters an object it cannot serialize and either no Delegate is provided, or the Delegate fails to handle the object [1][2]. * If no Delegate is provided, V8 automatically throws a DataCloneError when it encounters a host object (e.g., an object with internal fields) [1][6]. * If a Delegate is provided, the responsibility shifts to the implementation of WriteHostObject [1]. If the embedder determines that a specific object is not serializable, it must explicitly throw a DataCloneError (often using isolate->Throw(...)) and then return Nothing() to stop the serialization process [5][4][7]. The ThrowDataCloneError method in the Delegate is also available for the embedder to report such errors in a standardized way [8]. In summary, Just(true) requires the embedder to manually write the object's serialized data to the buffer [3], whereas the DataCloneError is the standard mechanism to report that an object is unsupported or cannot be serialized during the traversal [5][1][9].
Citations:
- 1: https://chromium.googlesource.com/v8/v8/+/65c54c7ac8483e2d7eaec4205e1b196c76b45d34/src/value-serializer.cc
- 2: https://chromium.googlesource.com/v8/v8.git/+/b301203e5aec9c8ff32f93aa31f8d764311e6e6e/src/value-serializer.cc
- 3: https://chromium.googlesource.com/v8/v8/+/ec06bb6ce5641cf65e400ec55b7421f87d04b999/src/objects/value-serializer.h
- 4: https://chromium.googlesource.com/v8/v8/+/ec06bb6ce5641cf65e400ec55b7421f87d04b999/include/v8-value-serializer.h
- 5: https://v8.github.io/api/head/classv8_1_1ValueSerializer_1_1Delegate.html
- 6: https://chromium.googlesource.com/v8/v8/+/b0c05f4ab351a0a595c551489e08fec83be75d11/src/value-serializer.cc
- 7: vimokoshi/browser@c3c4653
- 8: https://v8docs.nodesource.com/node-22.4/d8/dbc/classv8_1_1_value_serializer_1_1_delegate.html
- 9: fix(ext/web): structuredClone of non-serializable Web types throws DataCloneError denoland/deno#33465
🌐 Web query:
V8 ValueDeserializer::Delegate::ReadHostObject expected behavior: called only when WriteHostObject returns true and writes host-object data; what does it receive/expect?
💡 Result:
In the V8 engine, the ReadHostObject method within v8::ValueDeserializer::Delegate is intended for the embedder to reconstruct "host objects" that were previously serialized via the corresponding WriteHostObject method [1][2]. Expected Behavior: When the V8 deserializer encounters a serialization tag corresponding to a host object (specifically kHostObject) in the serialized data, it calls the embedder's ReadHostObject implementation to handle the reconstruction [3][4]. What it receives and expects: 1. Arguments: It receives an Isolate* as the current execution context [1][2]. 2. Input Data: The method does not receive the raw data directly as an argument; instead, the embedder is expected to read the necessary data from the deserializer's internal buffer [5][2]. The embedder uses the v8::ValueDeserializer instance's reading methods—such as ReadUint32, ReadUint64, ReadDouble, or ReadRawBytes—to consume the specific data it previously wrote during the serialization process [5][2][6]. 3. Return Value: It is expected to return a MaybeLocal [1][2]. If the host object is successfully reconstructed, it returns the JavaScript object wrapper; if reconstruction fails, it should throw an appropriate exception and return an empty MaybeLocal (Nothing) [1][2]. In summary, ReadHostObject is the mechanism for custom deserialization logic, where the embedder is responsible for manually reading back the exact byte-stream or format they defined during the serialization phase [3][2].
Citations:
- 1: https://v8.github.io/api/head/classv8_1_1ValueDeserializer_1_1Delegate.html
- 2: https://chromium.googlesource.com/v8/v8/+/ec06bb6ce5641cf65e400ec55b7421f87d04b999/include/v8-value-serializer.h
- 3: https://chromium.googlesource.com/v8/v8/+/65c54c7ac8483e2d7eaec4205e1b196c76b45d34/src/value-serializer.cc
- 4: https://chromium.googlesource.com/v8/v8.git/+/b301203e5aec9c8ff32f93aa31f8d764311e6e6e/src/value-serializer.cc
- 5: https://v8.github.io/api/head/classv8_1_1ValueDeserializer.html
- 6: https://v8.github.io/api/head/classv8_1_1ValueSerializer.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List related files
fd -i "WorkerMessage.*" test-app/runtime/src/main/cpp -t f || true
fd -i "Message.*Worker.*" test-app/runtime/src/main/cpp -t f || true
echo "---- WorkerMessage.h ----"
FILE_H="test-app/runtime/src/main/cpp/WorkerMessage.h"
if [ -f "$FILE_H" ]; then
wc -l "$FILE_H"
cat -n "$FILE_H" | sed -n '1,200p'
fi
echo "---- Search for Serialize/Deserialize usage ----"
rg -n "Message::Serialize\\(|Message::Deserialize\\(|WorkerMessage::" test-app/runtime/src/main/cpp || true
echo "---- Search for host-object handling / structured clone / DataCloneError ----"
rg -n "HostObject|DataCloneError|WriteHostObject|ReadHostObject|structured clone|Transfer" test-app/runtime/src/main/cpp || true
echo "---- Show entire WorkerMessage.cpp for full context (<=160 lines) ----"
cat -n test-app/runtime/src/main/cpp/WorkerMessage.cppRepository: NativeScript/android
Length of output: 14795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test-app/runtime/src/main/cpp/include/v8-value-serializer.h"
wc -l "$FILE"
echo "---- first 260 lines ----"
cat -n "$FILE" | sed -n '1,260p'Repository: NativeScript/android
Length of output: 11036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ROOT="test-app/runtime/src/main/cpp"
rg -n "Message::Serialize\\(" "$ROOT" || true
rg -n "Message::Deserialize\\(" "$ROOT" || true
rg -n "DataCloneError" "$ROOT/CallbackHandlers.cpp" "$ROOT" || true
echo "---- CallbackHandlers.cpp around first DataCloneError occurrence ----"
# pick a window near the earlier rg line numbers we saw: 1221/1273
sed -n '1190,1310p' "$ROOT/CallbackHandlers.cpp"Repository: NativeScript/android
Length of output: 6611
Don’t silently accept host objects during worker structured cloning
SerializerDelegate::WriteHostObject (29-33) always returns Just(true) without writing any host-object data, and DeserializerDelegate::ReadHostObject (68-72) always materializes a fresh {}. This causes unsupported host objects to “clone” successfully while dropping identity/native state instead of reporting a DataCloneError, which conflicts with the V8 delegate contract (return Nothing<bool>() after throwing when host objects aren’t serializable).
Safer default
Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object) override {
- // Host objects (e.g. Java proxies) carry no transferable native state;
- // they are recreated as plain objects on the receiving side.
- return Just(true);
+ ThrowDataCloneException(
+ context_,
+ ArgConverter::ConvertToV8String(
+ isolate, "Cannot clone host objects across workers"));
+ return Nothing<bool>();
}🤖 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/WorkerMessage.cpp` around lines 29 - 33, The
current WriteHostObject (SerializerDelegate::WriteHostObject) and
DeserializerDelegate::ReadHostObject silently accept host objects and
materialize empty objects; instead, detect unsupported host objects and surface
a DataCloneError via the V8 isolate and return Nothing<bool>() from
WriteHostObject (and return Nothing/throw in ReadHostObject) per the V8 delegate
contract. Update WriteHostObject to not return Just(true) when the object is not
serializable: call the isolate exception throw helper for a DataCloneError (or
v8 equivalent) and return Nothing<bool>(). Likewise update ReadHostObject to
throw the same DataCloneError and return the appropriate Nothing result rather
than creating a blank object. Ensure you reference the existing
SerializerDelegate::WriteHostObject and DeserializerDelegate::ReadHostObject
symbols when making these changes.
| void WorkerWrapper::PostMessageToParent(std::shared_ptr<worker::Message> message) { | ||
| if (isTerminating_) { | ||
| return; | ||
| } | ||
|
|
||
| auto parentTasks = parentTasks_.lock(); | ||
| if (parentTasks == nullptr) { | ||
| // the parent runtime is gone (e.g. a parent worker that shut down) | ||
| return; | ||
| } | ||
|
|
||
| int workerId = workerId_; | ||
| parentTasks->Post([workerId, message]() { | ||
| WorkerWrapper::FireMessageOnParentWorkerObject(workerId, message); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Block worker→parent posts once close() has started.
Close() marks the worker as closing, but PostMessageToParent() only rejects isTerminating_. A worker can still do close(); postMessage(...) in the same turn and deliver a message after shutdown has begun, which contradicts the drop-pending-messages behavior described here.
Also applies to: 92-102
🤖 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 54 - 69,
PostMessageToParent currently only checks isTerminating_, so messages can still
be posted after Close() begins; update WorkerWrapper::PostMessageToParent to
also check the "closing" state (the flag set by Close(), e.g., isClosing_ or
whatever Close() toggles) and return early if either isClosing_ or
isTerminating_ is true, and apply the same check to the other
PostMessageToParent overload/duplicate referenced (lines 92-102) so both paths
drop pending messages once Close() has started.
| if (hasOnError) { | ||
| auto errEvent = Object::New(isolate); | ||
| errEvent->Set(context, ArgConverter::ConvertToV8String(isolate, "message"), | ||
| ArgConverter::ConvertToV8String(isolate, message)); | ||
| errEvent->Set(context, ArgConverter::ConvertToV8String(isolate, "stackTrace"), | ||
| ArgConverter::ConvertToV8String(isolate, stackTrace)); | ||
| errEvent->Set(context, ArgConverter::ConvertToV8String(isolate, "filename"), | ||
| ArgConverter::ConvertToV8String(isolate, filename)); | ||
| errEvent->Set(context, ArgConverter::ConvertToV8String(isolate, "lineno"), | ||
| Number::New(isolate, lineno)); | ||
|
|
||
| Local<Value> args[] = {errEvent}; | ||
|
|
||
| // If the handler returns a truthy value the exception is handled | ||
| // and must not be raised to application level | ||
| Local<Value> result; | ||
| callback.As<Function>()->Call(context, Undefined(isolate), 1, args).ToLocal(&result); | ||
| if (!result.IsEmpty() && result->BooleanValue(isolate)) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| DEBUG_WRITE( | ||
| "Unhandled exception in '%s' thread. file: %s, line %d, message: %s\nStackTrace: %s", | ||
| threadName.c_str(), filename.c_str(), lineno, message.c_str(), | ||
| stackTrace.c_str()); |
There was a problem hiding this comment.
Unhandled worker errors are only logged, not surfaced.
When the parent worker.onerror is missing or returns falsy, this path falls through to DEBUG_WRITE(...) and exits. That silently swallows uncaught worker failures instead of raising them on the parent runtime/application.
🤖 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 274 - 299, The
code currently stops at DEBUG_WRITE when no onerror handler exists or it returns
falsy, which silently swallows the worker failure; update the path after the
callback check in WorkerWrapper.cpp (symbols: hasOnError, errEvent, callback,
DEBUG_WRITE) to surface the error to the parent runtime instead of only logging:
build a V8 exception/value from message/stackTrace/filename/lineno (e.g.
Exception::Error or a proper Error object) and either throw it into the current
isolate (isolate->ThrowException(...)) or invoke the parent error propagation
mechanism so the application receives the uncaught worker error; keep the
existing DEBUG_WRITE but ensure you call the chosen propagation (throw or host
error dispatch) when result is empty or falsy.
| public static Runtime initializeRuntimeWithConfiguration(StaticConfiguration config) { | ||
| staticConfiguration = config; | ||
| WorkThreadScheduler mainThreadScheduler = new WorkThreadScheduler(new Handler(Looper.myLooper())); | ||
| DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(0, mainThreadScheduler, null); |
There was a problem hiding this comment.
Bind the main runtime to the real main looper.
Line 490 builds the scheduler from Looper.myLooper() even though this entry point is documented as main-thread only. If a host app ever calls initializeRuntimeWithConfiguration(...) from a background looper thread, the runtime gets pinned to that looper while getMainThreadHandler() and isNotOnMainThread() still target Looper.getMainLooper(), which splits JS work across two different threads. Fail fast on non-main callers and create the handler from Looper.getMainLooper().
Proposed fix
public static Runtime initializeRuntimeWithConfiguration(StaticConfiguration config) {
+ if (Looper.myLooper() != Looper.getMainLooper()) {
+ throw new IllegalStateException("initializeRuntimeWithConfiguration must run on the main thread");
+ }
staticConfiguration = config;
- WorkThreadScheduler mainThreadScheduler = new WorkThreadScheduler(new Handler(Looper.myLooper()));
+ WorkThreadScheduler mainThreadScheduler = new WorkThreadScheduler(new Handler(Looper.getMainLooper()));
DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(0, mainThreadScheduler, null);
Runtime runtime = initRuntime(dynamicConfiguration);
return runtime;
}📝 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.
| public static Runtime initializeRuntimeWithConfiguration(StaticConfiguration config) { | |
| staticConfiguration = config; | |
| WorkThreadScheduler mainThreadScheduler = new WorkThreadScheduler(new Handler(Looper.myLooper())); | |
| DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(0, mainThreadScheduler, null); | |
| public static Runtime initializeRuntimeWithConfiguration(StaticConfiguration config) { | |
| if (Looper.myLooper() != Looper.getMainLooper()) { | |
| throw new IllegalStateException("initializeRuntimeWithConfiguration must run on the main thread"); | |
| } | |
| staticConfiguration = config; | |
| WorkThreadScheduler mainThreadScheduler = new WorkThreadScheduler(new Handler(Looper.getMainLooper())); | |
| DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(0, mainThreadScheduler, null); |
🤖 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/Runtime.java` around lines 488 - 491,
The initializeRuntimeWithConfiguration method currently constructs
WorkThreadScheduler with Looper.myLooper(), which allows callers on background
loopers to bind the runtime to the wrong looper; change it to verify the caller
is on the real main looper and use Looper.getMainLooper() to build the
WorkThreadScheduler. Specifically, in initializeRuntimeWithConfiguration check
Thread/Looper identity and throw an IllegalStateException (or similar) if
Looper.myLooper() != Looper.getMainLooper(), then create new
WorkThreadScheduler(new Handler(Looper.getMainLooper())) so
getMainThreadHandler() and isNotOnMainThread() consistently reference the true
main looper.
- WorkerWrapper: dispose the isolate via runtime_->GetIsolate() during shutdown instead of the workerIsolate_ atomic, which is still null if the bootstrap fails between initWorkerRuntime and the isolate publish (v8::Locker on a null isolate would crash the worker thread) - ConcurrentQueue: hold the lifecycle lock across enqueue + wakeup so a Push racing Terminate can no longer strand a message (and its backing stores) in a queue nothing will ever drain; Terminate also clears anything a racer already enqueued - ConcurrentQueue/LooperTasks: check eventfd()/ALooper_addFd() results and unwind on failure instead of registering a broken wakeup path - WorkerMessage: Malloc<T> now allocates sizeof(T) * n - n is an element count (latent footgun; only instantiated with char today) - Runtime.initRuntime: roll back runtimeCache/currentRuntime/GcListener registrations when init() or ts_helpers.js fails, so a failed bootstrap doesn't leave a stale half-initialized runtime reachable - tests: replace the Objects.requireNonNull zero-copy probe with a typed ByteBufferHolder fixture - generic erasure made the returned proxy Object-typed, so no ByteBuffer methods were reachable; also assert Java-side writes are visible through the SharedArrayBuffer and fail fast via worker.onerror instead of hanging to the jasmine timeout
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-app/runtime/src/main/java/com/tns/Runtime.java (1)
242-246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstructor-time publication still bypasses this rollback.
The new catch only runs after
new Runtime(...)returns, but the constructor already storesthisincurrentRuntimeandruntimeCacheat Lines 242-245 beforeGcListener.getInstance(...)finishes on Line 246. If that constructor step throws, the thread-local and cache still retain a half-built runtime, so retries on the same thread can fail with the existing-runtime guard.Proposed fix
- currentRuntime.set(this); - - runtimeCache.put(this.runtimeId, this); - gcListener = GcListener.getInstance(config.appConfig.getGcThrottleTime(), config.appConfig.getMemoryCheckInterval(), config.appConfig.getFreeMemoryRatio()); // capture static configuration to allow native lookups when currentRuntime is unavailable staticConfiguration = config; + + currentRuntime.set(this); + runtimeCache.put(this.runtimeId, this);Also applies to: 558-569
🤖 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/Runtime.java` around lines 242 - 246, The constructor publishes a partially-initialized Runtime (via currentRuntime.set(this) and runtimeCache.put(this.runtimeId, this)) before calling GcListener.getInstance(...) so if that call (or anything later in the ctor) throws the thread-local/cache retain a bad instance; fix by deferring publication until after all initialization that can throw completes (move currentRuntime.set(this) and runtimeCache.put(this.runtimeId, this) to after successful GcListener.getInstance(...) and any other throwing init), or alternatively wrap the constructor body in a try/catch that on any exception clears currentRuntime and removes runtimeCache entry for this.runtimeId before rethrowing, ensuring no half-built Runtime remains accessible.
🤖 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.
Outside diff comments:
In `@test-app/runtime/src/main/java/com/tns/Runtime.java`:
- Around line 242-246: The constructor publishes a partially-initialized Runtime
(via currentRuntime.set(this) and runtimeCache.put(this.runtimeId, this)) before
calling GcListener.getInstance(...) so if that call (or anything later in the
ctor) throws the thread-local/cache retain a bad instance; fix by deferring
publication until after all initialization that can throw completes (move
currentRuntime.set(this) and runtimeCache.put(this.runtimeId, this) to after
successful GcListener.getInstance(...) and any other throwing init), or
alternatively wrap the constructor body in a try/catch that on any exception
clears currentRuntime and removes runtimeCache entry for this.runtimeId before
rethrowing, ensuring no half-built Runtime remains accessible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f0c76b0-fe99-48aa-b3bb-6b4a00d2a6b8
📒 Files selected for processing (9)
test-app/app/src/main/assets/app/sharedtest-app/app/src/main/assets/app/tests/shared-array-buffer-test.jstest-app/app/src/main/java/com/tns/tests/ByteBufferHolder.javatest-app/runtime/src/main/cpp/ConcurrentQueue.cpptest-app/runtime/src/main/cpp/ConcurrentQueue.htest-app/runtime/src/main/cpp/LooperTasks.cpptest-app/runtime/src/main/cpp/WorkerMessage.htest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/java/com/tns/Runtime.java
💤 Files with no reviewable changes (1)
- test-app/runtime/src/main/cpp/ConcurrentQueue.h
🚧 Files skipped from review as they are similar to previous changes (4)
- test-app/runtime/src/main/cpp/WorkerMessage.h
- test-app/app/src/main/assets/app/tests/shared-array-buffer-test.js
- test-app/runtime/src/main/cpp/LooperTasks.cpp
- test-app/runtime/src/main/cpp/WorkerWrapper.cpp
…ptions during construction
2 participants
What
Replaces the Java
Handler/HandlerThread-based worker implementation with a C++ one that mirrors the iOS runtime'sWorkerWrapper, and addsSharedArrayBuffersupport both in worker messaging and in JS→Java argument marshalling.Why
Worker messaging previously round-tripped every
postMessagethrough Java as JSON strings (WorkerGlobalOnMessageCallback,WorkerObjectOnMessageCallback, etc.), which meant no structured clone, no way to shareSharedArrayBuffermemory between threads, and a worker lifecycle split awkwardly across Java and C++. Aligning with the iOS implementation gives both runtimes the same architecture and unlocks real structured-clone semantics.How
New C++ worker infrastructure:
WorkerWrapper— owns the worker'sstd::threadand full lifecycle (start/close/terminate). The thread attaches to the JVM and prepares a JavaLooperso worker/plugin code can keep using AndroidHandlers; that single looper pumps both Java messages and the worker's C++ inbox. Also hosts the live-worker registry (replacingCallbackHandlers::id2WorkerMap).WorkerMessage— structured-clone payloads viav8::ValueSerializer/ValueDeserializer;SharedArrayBuffercontents cross threads by sharing their backing stores instead of copying.ConcurrentQueue— thread-safe parent→worker inbox attached to the worker'sALoopervia eventfd; messages posted before the worker finishes bootstrapping are drained after the worker script runs.LooperTasks— per-runtime task queue, the Android analogue of iOS'sExecuteOnRunLoop; workers post messages, errors, and cleanup notifications to their parent runtime's queue. Held viaweak_ptrby children, so a child outliving its parent safely drops its posts.Worker semantics:
androidPriorityWorker option mapping toandroid.os.Processthread priorities (acceptsTHREAD_PRIORITY_*names or a clamped nice value; defaults to the previously hardcodedTHREAD_PRIORITY_BACKGROUND).onerroras plain strings (no cross-isolate handles).SharedArrayBuffer marshalling:
JsArgConverter,JsArgToArrayConverter, andJSToJavaObjectsConverternow acceptSharedArrayBufferwhereverArrayBuffer/typed arrays were accepted, exposing it to Java as a directByteBufferover the same memory (no copy).Removed Java plumbing:
WorkerThreadHandler,mainThreadHandler,workerIdToHandler,pendingWorkerMessages,MessageType.java,JavaScriptErrorMessage.java, therunWorkernative, and the worker JNI callbacks —Runtime.javashrinks by ~370 lines.Tests
shared-array-buffer-test.jscovering SAB→ByteBufferconversion,byteOffset/lengthof typed-array views over a SAB, and shared (zero-copy) memory semantics.Summary by CodeRabbit
New Features
Tests