| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…sfer)
Adds the WHATWG structuredClone(value, { transfer }) global, following
the post-context builtin architecture: all spec-level argument coercion
lives in the portable internal/structured-clone.js, and the native side
is one binding function so the Android runtime can reuse the JS
unchanged.
StructuredClone.cpp round-trips the value through v8::ValueSerializer
and v8::ValueDeserializer inside the one isolate, which is what
StructuredDeserialize(StructuredSerializeWithTransfer(...)) reduces to
when there is no second agent. Transferred buffers are validated up
front (ArrayBuffer, not detached, detachable, no duplicates) so a
rejected call never leaves a half-transferred graph, registered with the
serializer before the write, then detached and re-wrapped around their
original backing store for the deserializer.
There is no DOMException in this runtime, so clone failures throw an
Error whose name is "DataCloneError", matching how native exceptions are
surfaced. GetSharedArrayBufferId and AdoptSharedValueConveyor are
overridden purely to preserve that name: with a delegate installed V8's
defaults throw a plain Error straight onto the isolate instead of
routing through ThrowDataCloneError.
Host objects are rejected. A native/interop wrapper serialized without
its native counterpart would deserialize into a wrapper around nothing,
so WriteHostObject reports it as uncloneable.
The transfer list is converted per WebIDL sequence semantics, so any
iterable works and a string primitive does not. ArrayBuffer membership is
brand-checked through the captured byteLength getter, which also excludes
SharedArrayBuffer -- correctly, since it is not transferable.
Adds the SymbolIterator primordial and documents the surface, the
transfer semantics and the deviations in docs/structured-clone.md.
|
Caution Review failedThe pull request is closed. Configuration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 8aa76ed6-6736-40a3-9328-1176960a4bee 📥 CommitsReviewing files that changed from the base of the PR and between e712ceb and f80db5b. 📒 Files selected for processing (5)
📝 Walkthrough WalkthroughAdds the global structuredClone API with V8 serialization, transfer-list validation, worker postMessage support, runtime and build integration, documentation, and an availability test. ChangesStructured Clone API
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agentsVerify 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 21-25: Update the cloneability description to exclude Symbol from the supported primitive values, replacing “all primitives” with “all primitive values except Symbol” while preserving the existing BigInt, undefined, and -0 examples. In `@NativeScript/runtime/js/structured-clone.js`: - Around line 61-80: Capture and validate the iterator’s next method once immediately after creating iterator, then invoke that captured method with FunctionPrototypeCall for each iteration. Update the loop in the transfer-list conversion flow to stop reading iterator.next repeatedly while preserving the existing iterator-result and transferable-item validation. In `@TestRunner/app/shared`: - Line 1: Merge the structured-clone tests into the TestRunner/app/shared submodule, ensuring the resulting master contains all four structured-clone specifications, then update the parent repository’s submodule pointer from a7492cecc9e2be95c7eb58591a5cbbb5dbb0267a to the correct master commit 3a262b979c6b84cdfe69cd495436a7088d016505.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0999161a-9849-4c27-b8b8-3278b181edf5
📥 CommitsReviewing files that changed from the base of the PR and between 5e470f4 and 985606e.
📒 Files selected for processing (11)
Sorry, something went wrong.
Converting `options.transfer` re-read `next` off the iterator on every step. WebIDL creates the iterator record once and captures `next` with it, so a `next` that changes mid-iteration must not be observed; read it once after creating the iterator, reject a non-callable one as a TypeError, and invoke it through the captured reference. Also reconciles the cloneable-types list in the docs with the error list: symbols throw a DataCloneError, so "all primitives" was wrong.
…e and worker postMessage structuredClone and worker postMessage each carried their own serializer and deserializer delegates, so the two could drift on everything the structured clone algorithm leaves to the embedder. StructuredSerialization now owns that machinery once and both call it. SerializedValue keeps serializing and deserializing as separate halves over a neutral buffer plus its backing-store lists, because a worker message is read back on a different isolate than it was written on while structuredClone round-trips on one. Transfer-list validation, id registration and the claim-then-detach ordering move in with it, so the rule that a buffer is detached only after the value is safely written holds for both callers. Consolidating settles three inconsistencies: - SharedArrayBuffer is now shared by structuredClone rather than rejected, which is what the spec asks for and what the worker path already did. - Worker postMessage no longer ignores a failed Serialize; it returns with the exception pending instead of posting an unwritten message. - Both paths raise DataCloneError through NativeScriptException. The two mechanisms built the same object -- Error with name "DataCloneError" -- except that the worker's also carried `fullMessage`, so unifying on it keeps every property worker callers can already see today. Host objects are the one place the callers still differ, and HostObjectPolicy is now the only place that is written down: structuredClone rejects them per spec, postMessage keeps delivering an empty object as it always has. The degrade branch writes no payload and its ReadHostObject counterpart consumes none, so the stream stays balanced. Drops the Node message-port scaffolding the core supersedes along with Worker::Serialize, the pre-structured-clone JSON path whose only remaining callers were commented out.
Both postMessage entry points -- Worker.prototype.postMessage and the worker global -- accepted a second argument and dropped it, so buffers could only ever be copied into a worker. They now hand it to the shared serialization core as the transfer list, which moves each buffer's memory instead: the sender's ArrayBuffer is detached as the message is written, and the receiving isolate wraps the same backing store. The list must be a plain array; anything else is a TypeError. The WebIDL iterable-to-sequence conversion that lets structuredClone accept a Set or any iterable belongs to its JavaScript wrapper, and postMessage has none. Everything past that point -- duplicate detection, the detached and detachable checks, and detaching only after a successful write -- is the same code structuredClone runs. Documents the transfer semantics alongside structuredClone's, including the one behavior the two entry points do not share: a posted native object still arrives as an empty object rather than raising a DataCloneError.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)NativeScript/runtime/StructuredSerialization.cpp (1)🤖 Prompt for all review comments with AI agentsNativeScript/runtime/Worker.mm (1)212-216: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Leave transferredBuffers_ in a safe state after deserialization.
The loop moves each shared_ptr<BackingStore> out but keeps the vector at the same size. Every element becomes null. A second Deserialize call then passes a null backing store to ArrayBuffer::New, which dereferences it and crashes.
The header documents a single call, and both current callers respect that. Make the constraint enforced rather than documented.
♻️ Proposed refactor🤖 Prompt for AI Agentsfor (size_t i = 0; i < transferredBuffers_.size(); i++) { 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 `@NativeScript/runtime/StructuredSerialization.cpp` around lines 212 - 216, Update the deserialization loop in StructuredSerialization so transferredBuffers_ is emptied or otherwise made unusable after its buffers are moved, preventing a second Deserialize call from passing null backing stores to ArrayBuffer::New. Enforce the single-use constraint in the implementation rather than relying only on the header documentation.296-304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract the shared postMessage serialization block. PostMessageToMainCallback and PostMessageCallback now build the same { data } wrapper, read the same optional transfer list, and call Message::Serialize with the same kDegrade policy. The two copies must stay in sync with the transfer-list contract.
🤖 Prompt for AI Agents
- NativeScript/runtime/Worker.mm#L296-L304: move the wrap-and-serialize sequence into a static helper, for example static bool BuildMessage(Isolate*, Local<Context>, const FunctionCallbackInfo<Value>&, std::shared_ptr<worker::Message>&), and call it here.
- NativeScript/runtime/Worker.mm#L356-L365: replace this copy with a call to the same helper.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Worker.mm` around lines 296 - 304, Extract the duplicated `{ data }` wrapping, optional transfer-list handling, and `Message::Serialize` call into a shared static helper such as `BuildMessage`, returning failure when serialization rejects the value while preserving the pending exception. Update `PostMessageToMainCallback` at NativeScript/runtime/Worker.mm:296-304 and `PostMessageCallback` at NativeScript/runtime/Worker.mm:356-365 to call the helper, with no direct change needed beyond replacing each duplicated block.
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 46-47: Update the transfer-list documentation to state that null
and undefined are accepted as no-transfer values by SerializedValue::Serialize,
while other non-array values cause a TypeError. Preserve the existing
explanation that arrays are accepted transfer lists.
In `@NativeScript/runtime/StructuredSerialization.cpp`:
- Around line 185-191: Update the transfer loop in StructuredSerialization so
the result of buffer->Detach(Local<Value>()) is checked; when it returns
Nothing, preserve or replace the pending exception with DataCloneError and
return Nothing<bool>() instead of storing the backing store or reporting
successful serialization. Keep the existing successful transfer behavior
unchanged.
In `@NativeScript/runtime/Worker.mm`:
- Around line 296-304: Move the target runtime lookup in the message-posting
function to before the Serialize call, and return immediately if it yields
nullptr. Keep serialization and its existing pending-exception handling after
this validation so transfer-list ArrayBuffers are not detached when the message
will be dropped.
---
Nitpick comments:
In `@NativeScript/runtime/StructuredSerialization.cpp`:
- Around line 212-216: Update the deserialization loop in
StructuredSerialization so transferredBuffers_ is emptied or otherwise made
unusable after its buffers are moved, preventing a second Deserialize call from
passing null backing stores to ArrayBuffer::New. Enforce the single-use
constraint in the implementation rather than relying only on the header
documentation.
In `@NativeScript/runtime/Worker.mm`:
- Around line 296-304: Extract the duplicated `{ data }` wrapping, optional
transfer-list handling, and `Message::Serialize` call into a shared static
helper such as `BuildMessage`, returning failure when serialization rejects the
value while preserving the pending exception. Update `PostMessageToMainCallback`
at NativeScript/runtime/Worker.mm:296-304 and `PostMessageCallback` at
NativeScript/runtime/Worker.mm:356-365 to call the helper, with no direct change
needed beyond replacing each duplicated block.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b49a49d4-120d-46d8-8cf4-d9b3a5f439a8
📥 CommitsReviewing files that changed from the base of the PR and between 985606e and 3d6d1d9.
📒 Files selected for processing (11)
Sorry, something went wrong.
The shared suite now gates itself on the global being present, so the explicit opt-in call is redundant: runAllTests() picks it up and the suite skips itself on runtimes that do not implement structuredClone. That gate would also turn this runtime losing structuredClone into a green run, so RuntimeImplementedAPIs.js gets an unguarded spec asserting the global exists. The shared suite is free to skip; this one is not.
…sage target before serializing Two ways a transfer could take a buffer's memory and give nothing back. Serialize discarded the result of ArrayBuffer::Detach, so a buffer that refused the null key was recorded as transferred while its contents stayed where they were; the receiver would have got an empty buffer and no error. The failure now propagates. Only a buffer carrying an [[ArrayBufferDetachKey]] can refuse, which nothing here produces -- script cannot set one, this runtime never calls SetDetachKey, and the WebAssembly memory buffers that have one are already turned away as non-detachable -- so the branch is unreachable and stays untested by design; the comment says so, and V8's TypeError is passed along unchanged because it names the mismatch. The worker-to-main path also looked up the target runtime after serializing and returned silently when it was gone -- by which point the caller's transfer-list buffers were already detached. The lookup moves above the serialize call, alongside the IsRunning check, so a message that cannot be delivered costs the caller nothing. The main-to-worker path already resolved its target first and is unchanged. Also documents that postMessage treats a missing, undefined or null transfer list as "transfer nothing", and rejects every other non-array.
The structuredClone suite is merged (#26), so the pointer moves off the feature branch entirely and onto master, where the work now lives alongside the Performance suite merged as #25. master's runAllTests() calls both. Nothing is lost in the move: the suite on master is byte-identical to what the branch carried. The Performance suite arrives with it and gates itself the same way this one does, so on this runtime -- which has no PerformanceObserver -- it reports a single pending spec rather than failures.
…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.
…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.
…sfer) (#2000) * feat: structuredClone global (HTML structured clone, ArrayBuffer transfer) 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. * test: pin shared suite to common-runtime-tests-app master Both tests PRs (#25 Performance, #26 StructuredClone) squash-merged upstream; the branch commit this previously pinned is now orphaned. StructuredClone/ content is byte-identical to the tested commit; master additionally carries the Performance suite, which mainpage.js does not invoke. * test: bump shared suite to current master (Performance detail-clone specs) Matches the pin on ios main. Only Performance/index.js changes -- not invoked by mainpage.js; StructuredClone/ is unchanged.
| Back | FazBrowse Home | New Git URL |
Implements the WHATWG structuredClone(value, { transfer }) global.
Architecture
Follows the post-context JS builtin pattern, same as the performance API in #430:
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 SymbolIterator primordial.
V8 14.9 notes
Deviations from the specification
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). It runs from runAllTests() and gates itself on structuredClone being present — a runtime without the API reports one visible pending spec instead of failures, so the tests PR is safe on Android's master immediately. An unguarded canary in RuntimeImplementedAPIs.js asserts the global exists on iOS, so the gate cannot hide a regression here.
Full TestRunner suite green on the simulator: 1010 tests, 0 failures, 11 pre-existing skips.
Merge sequencing
Done — the tests PR (NativeScript/common-runtime-tests-app#26) is merged and the submodule pointer here references the resulting master (which also carries the self-gating Performance suite from #25; it skips on this branch as one pending spec, verified in the full run). No remaining ordering constraints.
Independent of #430 — both branch off main. Trivial conflicts are expected in whichever merges second: the Runtime.mm post-context init block, project.pbxproj, tools/js2c-inputs.xcfilelist, js/primordials.js, and (between the two tests PRs) shared/index.js and RuntimeImplementedAPIs.js.
Serialization consolidation (added scope)
Follow-up review moved the shared machinery into one core so structuredClone and worker postMessage can never diverge:
Summary by CodeRabbit
New Features
Documentation
Tests