feat: structuredClone global (HTML structured clone, ArrayBuffer transfer) - #2000
feat: structuredClone global (HTML structured clone, ArrayBuffer transfer)#2000edusperoni merged 3 commits into
Conversation
Sorry, something went wrong.
WalkthroughAdds the global structuredClone API with V8 serialization support, transfer-list handling, and runtime-specific errors. Reuses the serialization core for worker messages, wires the builtin into runtime initialization, adds startup tests, and documents the behavior. ChangesStructured clone support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant structuredClone
participant StructuredClone
participant SerializedValue
Caller->>structuredClone: Pass value and options
structuredClone->>StructuredClone: Pass value and transfer list
StructuredClone->>SerializedValue: Serialize value
SerializedValue-->>StructuredClone: Deserialize cloned value
StructuredClone-->>structuredClone: Return cloned value
structuredClone-->>Caller: Return result or DataCloneError
Possibly related PRs
Suggested reviewers: nathanwalker Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
…sfer) Android port of NativeScript/ios#431, in lockstep with the iOS runtime. Implements the WHATWG structuredClone(value, { transfer }) global as a post-context JS builtin: js/structured-clone.js (shared unchanged with iOS) owns the WebIDL argument coercion, and a thin native binding runs a v8::ValueSerializer -> ValueDeserializer round-trip in the one isolate. The serialization machinery is consolidated into StructuredSerialization.{h,cpp} (tns::serialization) so structuredClone and worker postMessage run on one core: delegate pair, DataCloneError construction (an Error carrying that name -- previously the worker path threw a plain Error with a message prefix), transfer-list validation, and the register->write->claim->detach ordering that V8 14.9 requires (Detach() aborts on non-detachable buffers; Release() must be claimed even after a failed write). WorkerMessage.cpp is deleted and WorkerMessage.h reduced to an alias. postMessage gains the ArrayBuffer transfer list on both entry points. Host objects stay intentionally asymmetric via HostObjectPolicy: structuredClone rejects (spec), postMessage keeps the shipped degrade-to-{} behavior. Tests: shared cross-runtime suite (common-runtime-tests-app#26, 54 specs) wired via shared.runStructuredCloneTests(), plus an unguarded canary in testRuntimeImplementedAPIs.js. Documented in docs/structured-clone.md.
…pecs) Matches the pin on ios main. Only Performance/index.js changes -- not invoked by mainpage.js; StructuredClone/ is unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)test-app/runtime/src/main/cpp/StructuredClone.cpp (1)🤖 Prompt for all review comments with AI agentstest-app/runtime/src/main/cpp/StructuredSerialization.cpp (2)46-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Propagate initialization failures instead of only asserting.
In a release build NDEBUG disables every assert here. If Function::New, the Set, or RunBuiltin fails, Init returns normally with structuredClone missing and a pending exception left on the isolate. Runtime::PrepareV8Runtime then continues into Interop::Init and the failure surfaces far from its cause. ErrorEvents::Init and Events::Init throw NativeScriptException for the same failures.
Consider matching that behavior so bootstrap failures are visible in release builds.
🤖 Prompt for AI AgentsVerify 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/StructuredClone.cpp` around lines 46 - 65, Update StructuredClone::Init to propagate failures from Function::New, binding->Set, and BuiltinLoader::RunBuiltin instead of relying solely on assert. Match the NativeScriptException behavior used by ErrorEvents::Init and Events::Init so failures throw immediately in release builds and preserve the underlying isolate exception details.test-app/runtime/src/main/cpp/js/structured-clone.js (1)233-237: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Make the single-use contract of Deserialize enforceable.
std::move(transferredBuffers_[i]) empties each shared_ptr but leaves the vector entries in place. A second Deserialize call then passes an empty shared_ptr<BackingStore> to ArrayBuffer::New, which fails a V8 CHECK instead of reporting an error. The header documents the single-use contract, but nothing enforces it. Clear the vector after the loop and assert that the transferred entries are still owned.
♻️ Proposed guard🤖 Prompt for AI Agentsfor (size_t i = 0; i < transferredBuffers_.size(); i++) { + assert(transferredBuffers_[i] != nullptr); deserializer.TransferArrayBuffer( static_cast<uint32_t>(i), ArrayBuffer::New(isolate, std::move(transferredBuffers_[i]))); } + transferredBuffers_.clear();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/StructuredSerialization.cpp` around lines 233 - 237, Update Deserialize’s transferred-buffer loop to assert each transferredBuffers_ entry is still non-null before moving it into ArrayBuffer::New, then clear transferredBuffers_ after the loop so a second Deserialize cannot reuse emptied entries.
219-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Create the HandleScope before the Context::Scope.
Serialize enters the HandleScope first (Line 169-170), Deserialize reverses the order. Both work, but matching the order keeps scope destruction consistent with the rest of the runtime.
♻️ Proposed reordering🤖 Prompt for AI Agents- Context::Scope contextScope(context); EscapableHandleScope handleScope(isolate); + Context::Scope contextScope(context);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/StructuredSerialization.cpp` around lines 219 - 222, In SerializedValue::Deserialize, construct the EscapableHandleScope before entering Context::Scope, matching the established ordering in Serialize. Keep the existing scopes and behavior unchanged apart from this declaration reorder.93-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Install structuredClone with WebIDL property attributes.
A plain assignment creates an enumerable own property on globalThis. WebIDL requires global interface members to be writable, configurable, and non-enumerable. Code that enumerates globalThis now sees structuredClone.
♻️ Proposed change-g.structuredClone = function structuredClone(value, options = undefined) { +function structuredClone(value, options = undefined) { if (arguments.length < 1) { throw new TypeError("structuredClone: 1 argument required, but only 0 present"); } @@ return clone(value, transfer); -}; +} + +ObjectDefineProperty(g, "structuredClone", { + value: structuredClone, + writable: true, + enumerable: false, + configurable: true, +});ObjectDefineProperty must be added to the destructured primordials list.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/js/structured-clone.js` around lines 93 - 110, Update the installation of structuredClone around the structuredClone function to define the global property via ObjectDefineProperty instead of plain assignment, using writable and configurable true with enumerable false. Add ObjectDefineProperty to the destructured primordials list and preserve the existing function behavior.
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 `@docs/structured-clone.md`: - Around line 37-47: Update the “postMessage” documentation to name both worker postMessage entry points, including the worker-side API, and state that both accept an optional transfer list only when it is an array; omitted, undefined, or null transfer nothing, while other non-array values throw TypeError. Keep the existing shared serialization and host-object behavior details unchanged. In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`: - Around line 1323-1328: Update the WorkerGlobalScope.postMessage serialization failure path around Serialize to call tc.ReThrow() before returning when Serialize yields Nothing, preserving the pending DataCloneError or TypeError so JavaScript can catch the synchronous failure. --- Nitpick comments: In `@test-app/runtime/src/main/cpp/js/structured-clone.js`: - Around line 93-110: Update the installation of structuredClone around the structuredClone function to define the global property via ObjectDefineProperty instead of plain assignment, using writable and configurable true with enumerable false. Add ObjectDefineProperty to the destructured primordials list and preserve the existing function behavior. In `@test-app/runtime/src/main/cpp/StructuredClone.cpp`: - Around line 46-65: Update StructuredClone::Init to propagate failures from Function::New, binding->Set, and BuiltinLoader::RunBuiltin instead of relying solely on assert. Match the NativeScriptException behavior used by ErrorEvents::Init and Events::Init so failures throw immediately in release builds and preserve the underlying isolate exception details. In `@test-app/runtime/src/main/cpp/StructuredSerialization.cpp`: - Around line 233-237: Update Deserialize’s transferred-buffer loop to assert each transferredBuffers_ entry is still non-null before moving it into ArrayBuffer::New, then clear transferredBuffers_ after the loop so a second Deserialize cannot reuse emptied entries. - Around line 219-222: In SerializedValue::Deserialize, construct the EscapableHandleScope before entering Context::Scope, matching the established ordering in Serialize. Keep the existing scopes and behavior unchanged apart from this declaration reorder.
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: d22843f7-7af5-44bc-91c6-8cbc33f9c4c2
📥 CommitsReviewing files that changed from the base of the PR and between 7ff8e85 and 975e724.
📒 Files selected for processing (16)- docs/README.md
- docs/structured-clone.md
- test-app/app/src/main/assets/app/mainpage.js
- test-app/app/src/main/assets/app/shared
- test-app/app/src/main/assets/app/tests/testRuntimeImplementedAPIs.js
- test-app/runtime/CMakeLists.txt
- test-app/runtime/src/main/cpp/CallbackHandlers.cpp
- test-app/runtime/src/main/cpp/Runtime.cpp
- test-app/runtime/src/main/cpp/StructuredClone.cpp
- test-app/runtime/src/main/cpp/StructuredClone.h
- test-app/runtime/src/main/cpp/StructuredSerialization.cpp
- test-app/runtime/src/main/cpp/StructuredSerialization.h
- test-app/runtime/src/main/cpp/WorkerMessage.cpp
- test-app/runtime/src/main/cpp/WorkerMessage.h
- test-app/runtime/src/main/cpp/js/primordials.js
- test-app/runtime/src/main/cpp/js/structured-clone.js
- test-app/runtime/src/main/cpp/WorkerMessage.cpp
Sorry, something went wrong.
| `structuredClone` and worker `postMessage` run on the same serialization core, so everything above — which types clone, graph identity, cycles, `SharedArrayBuffer` sharing — holds for messages too. `postMessage` takes the same transfer list as a second argument: | ||
|
|
||
| ```js | ||
| worker.postMessage({ pixels: buffer }, [buffer]); // buffer is detached here, | ||
| // its memory now in the worker | ||
| ``` | ||
|
|
||
| Two differences are intentional: | ||
|
|
||
| - **The transfer list must be an array.** Omitting it, or passing `undefined` or `null`, means "transfer nothing"; every other non-array value is a `TypeError`. The WebIDL iterable-to-sequence conversion that lets `structuredClone` take a `Set` or any iterable lives in the JavaScript wrapper around `structuredClone`; `postMessage` is native all the way down and has no such wrapper. | ||
| - **Host objects degrade instead of throwing.** Posting a native/interop object delivers an empty object to the receiver rather than raising a `DataCloneError`. This is long-standing shipped behavior, and app code relies on it; `structuredClone`, being new, follows the spec and rejects. The asymmetry is encoded in exactly one place — the `HostObjectPolicy` enum in `test-app/runtime/src/main/cpp/StructuredSerialization.h` — and unifying the two on rejection is a breaking change that needs the iOS runtime to move at the same time. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both postMessage entry points.
The PR adds transfer-list support to both worker postMessage entry points, but this section only names and demonstrates worker.postMessage. State both entry points and their shared array-only transfer-list rule so the worker-side API is also discoverable.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/structured-clone.md` around lines 37 - 47, Update the “postMessage” documentation to name both worker postMessage entry points, including the worker-side API, and state that both accept an optional transfer list only when it is an array; omitted, undefined, or null transfer nothing, while other non-array values throw TypeError. Keep the existing shared serialization and host-object behavior details unchanged.
Sorry, something went wrong.
| if (message->Serialize(isolate, context, args[0], transferList, | ||
| serialization::HostObjectPolicy::kDegrade) | ||
| .IsNothing()) { | ||
| // The transfer list was rejected or the value could not be cloned; | ||
| // the exception is already pending and nothing may be posted. | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how worker callback errors are dispatched and whether worker clone
# failures have regression coverage.
rg -n -C 8 '\bWorkerGlobalPostMessageCallback\b|\bCallWorkerScopeOnErrorHandle\b' \
test-app/runtime/src/main/cpp/CallbackHandlers.cpp
rg -n -C 5 'DataCloneError|structuredClone|postMessage\s*\(' \
test-app/app/src/main/assets/appRepository: NativeScript/android
Length of output: 10333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the complete callback, error-dispatch helper, and Message::Serialize
# implementation without executing repository code.
rg -n -l 'CallWorkerScopeOnErrorHandle|class Message|Message::Serialize|\.Serialize\(' \
test-app/runtime/src/main/cpp
printf '\n--- callback and error helper ---\n'
sed -n '1288,1535p' test-app/runtime/src/main/cpp/CallbackHandlers.cpp
printf '\n--- serialization declarations and definitions ---\n'
rg -n -C 12 'Serialize\s*\(|class Message|struct Message' \
test-app/runtime/src/main/cpp test-app/runtime/src/mainRepository: NativeScript/android
Length of output: 40340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete worker error helper ---'
sed -n '1504,1585p' test-app/runtime/src/main/cpp/CallbackHandlers.cpp
printf '%s\n' '--- serialization failure paths ---'
sed -n '1,230p' test-app/runtime/src/main/cpp/StructuredSerialization.cpp
sed -n '230,430p' test-app/runtime/src/main/cpp/StructuredSerialization.cpp
printf '%s\n' '--- related postMessage callback and tests ---'
sed -n '1210,1290p' test-app/runtime/src/main/cpp/CallbackHandlers.cpp
rg -n -C 10 'postMessage|DataCloneError|transfer list|structured clone|clone' \
test-app/app/src/main/assets/app/tests test-app/runtime/src/main/cpp \
-g '*.js' -g '*.cpp' -g '*.h'Repository: NativeScript/android
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- worker execution and exception forwarding ---'
rg -n -C 12 'PassUncaughtExceptionFromWorkerToParent|TryCatch|Execute|RunScript|Script::Run|onerror' \
test-app/runtime/src/main/cpp \
-g '*.cpp' -g '*.h' \
| grep -v 'robin_hood.h' | head -n 500
printf '%s\n' '--- all local TryCatch rethrow patterns ---'
rg -n -C 5 'TryCatch|ReThrow|IsNothing\(\).*return|IsNothing\(\)' \
test-app/runtime/src/main/cpp \
-g '*.cpp' -g '*.h' | grep -v 'robin_hood.h' | head -n 500
printf '%s\n' '--- relevant test names and worker error assertions ---'
rg -n -C 8 'worker\.onerror|onerror\s*=|DataCloneError|postMessage.*catch|catch.*postMessage|could not be cloned|transfer list' \
test-app/app/src/main/assets/app/tests \
-g '*.js'Repository: NativeScript/android
Length of output: 44955
🌐 Web query:
V8 TryCatch ReThrow exception caught callback destructor documentation
💡 Result:
In the V8 engine, the v8::TryCatch class is an external exception handler used to catch JavaScript exceptions [1][2]. Its destructor (~TryCatch) is responsible for unregistering and deleting the try/catch block from V8 [3][4]. Regarding the specific behaviors you mentioned: 1. Destructor Functionality: The destructor ~TryCatch performs necessary cleanup by unregistering the block [3][1]. Because it is designed to be stack-allocated, its cleanup happens automatically when the object goes out of scope [3][5]. 2. ReThrow Method: The ReThrow method is used to re-throw an exception that has been caught by the current v8::TryCatch instance [1]. Crucially, it re-throws the exception in a manner that avoids it being caught again by the same TryCatch instance [3][4]. After calling ReThrow, it is illegal to execute any further JavaScript operations; the caller must return immediately to the point where the exception is propagated [1][2]. 3. Destructor Callback: V8 does not provide a specific callback mechanism tied to the destruction of a v8::TryCatch instance itself. If you are looking for callbacks related to object lifecycles (such as cleaning up C++ resources associated with a JavaScript object), V8 provides WeakPersistent handles and finalizers (e.g., v8::Global::SetWeak or v8::FinalizationRegistry) rather than hooks on the TryCatch destructor [6]. In summary, the v8::TryCatch destructor handles internal engine state management to ensure exception handling chains remain consistent [3][1]. It is not a user-extensible callback point [3][2].
Citations:
- 1: https://github.com/v8/v8/blob/ea4e879b/include/v8-exception.h
- 2: https://v8docs.nodesource.com/node-22.4/d4/dc6/classv8_1_1_try_catch.html
- 3: https://v8.github.io/api/head/classv8_1_1TryCatch.html
- 4: https://v8docs.nodesource.com/node-25.0/d4/dc6/classv8_1_1_try_catch.html
- 5: https://v8docs.nodesource.com/node-24.1/d4/dc6/classv8_1_1_try_catch.html
- 6: https://groups.google.com/g/v8-users/c/oRST3iG5b8o
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- worker message-dispatch control flow ---'
sed -n '130,245p' test-app/runtime/src/main/cpp/WorkerWrapper.cpp
sed -n '365,415p' test-app/runtime/src/main/cpp/WorkerWrapper.cpp
printf '%s\n' '--- focused test inventory ---'
rg -n -C 6 'worker\.onerror|onerror\s*=|DataCloneError|postMessage.*catch|catch.*postMessage|transfer list|could not be cloned' \
test-app/app/src/main/assets/app/tests \
-g '*.js' || true
printf '%s\n' '--- static path check ---'
python3 - <<'PY'
from pathlib import Path
p = Path("test-app/runtime/src/main/cpp/CallbackHandlers.cpp")
text = p.read_text()
start = text.index("CallbackHandlers::WorkerGlobalPostMessageCallback")
end = text.index("void\nCallbackHandlers::WorkerObjectTerminateCallback", start)
fn = text[start:end]
checks = {
"callback creates TryCatch": "TryCatch tc(isolate);" in fn,
"serialization can return Nothing": ".IsNothing()" in fn,
"serialization failure returns directly": " return;" in fn[fn.index(".IsNothing()"):],
"serialization failure calls worker error helper": "CallWorkerScopeOnErrorHandle(isolate, tc);" in fn[fn.index(".IsNothing()"):],
"serialization failure rethrows": "tc.ReThrow();" in fn[fn.index(".IsNothing()"):],
}
for name, result in checks.items():
print(f"{name}: {result}")
PYRepository: NativeScript/android
Length of output: 9333
Re-throw the serialization exception from WorkerGlobalScope.postMessage.
When Serialize returns Nothing, it leaves a DataCloneError or TypeError pending in tc. Call tc.ReThrow() before returning so JavaScript can catch the synchronous failure.
🤖 Prompt for AI AgentsVerify 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 1323 - 1328, Update the WorkerGlobalScope.postMessage serialization failure path around Serialize to call tc.ReThrow() before returning when Serialize yields Nothing, preserving the pending DataCloneError or TypeError so JavaScript can catch the synchronous failure.
Sorry, something went wrong.
Android port of NativeScript/ios#431 (now merged on iOS main) — implements the WHATWG structuredClone(value, { transfer }) global, in lockstep with the iOS runtime.
Architecture
Follows the post-context JS builtin pattern from the js-builtins stack (#1989–#1992, all now in main):
ArrayBuffer membership in the transfer list is brand-checked from JS through the captured ArrayBuffer.prototype.byteLength getter (tamper-proof, and correctly excludes SharedArrayBuffer, which is not transferable); the native side re-checks with IsArrayBuffer().
Adds the Error and SymbolIterator primordials.
Serialization consolidation
Mirrors the iOS PR's consolidation so structuredClone and worker postMessage can never diverge:
V8 14.9 notes (same as iOS)
Deviations from the specification
Same set as iOS, documented in docs/structured-clone.md:
Tests
Shared cross-runtime suite: NativeScript/common-runtime-tests-app#26 (54 specs — clone semantics, graph identity and cycles, transfer, SharedArrayBuffer sharing, worker message transfer, DataCloneError cases), wired up via shared.runStructuredCloneTests() in mainpage.js. An unguarded canary in testRuntimeImplementedAPIs.js asserts the global exists, so the suite's self-gating cannot hide a regression.
Full suite green on a Pixel 3a API 36 emulator (arm64): 701 specs, 0 failures, 5 skipped, 11 disabled.
Merge sequencing
Summary by CodeRabbit
New Features
Documentation
Tests