fix(worker_threads): preserve structured clone values - #7091
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWorker message channels now serialize typed arrays and ArrayBuffers as structured snapshots, recursively reject uncloneable message graphs, and emit DOM-style clone/state errors. Typed-array property lookup resolves inherited properties through custom prototypes, with cycle protection, GC root scanning, and exception-safe restoration. ChangesStructured clone runtime behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Sender
participant MessagePort
participant CloneValidator
participant SerializedMessage
participant ChannelPump
Sender->>MessagePort: post message
MessagePort->>CloneValidator: validate submitted graph
CloneValidator-->>MessagePort: cloneability result
MessagePort->>SerializedMessage: serialize binary or ordinary value
SerializedMessage->>ChannelPump: enqueue snapshot
ChannelPump->>SerializedMessage: deserialize snapshot
SerializedMessage-->>Sender: deliver cloned message
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/perry/tests/issue_6763_broadcast_clone.rs (1)
12-122: 📐 Maintainability & Code Quality | 🔵 TrivialConsider mirroring this coverage in a unit test.
As per coding guidelines,
crates/*/tests/*.rsintegration suites are lower-priority than unit tests visible tocargo-test: Prefer acceptance coverage in unit tests visible tocargo-test, because integration suites undercrates/*/tests/*.rsdo not run on every PR. Based on learnings, a past PR confirmed that changed suites under this path do run via the diff-scopede2e-scopedCI job for that PR, which reduces (but doesn't eliminate) the risk here.
[recommended_refactor:low_effort_and_low_reward]🤖 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 `@crates/perry/tests/issue_6763_broadcast_clone.rs` around lines 12 - 122, Mirror the coverage from broadcast_and_port_clone_preserve_binary_values_and_errors in a unit test visible to cargo-test, rather than relying solely on the integration test under crates/*/tests. Preserve assertions for typed-array cloning, closed-port errors, markAsUncloneable behavior, and ArrayBuffer cloning, while keeping the existing integration coverage unchanged unless reuse is straightforward.Sources: Coding guidelines, Learnings
crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs (1)
544-571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate prototype-fallback logic between TypedArray and buffer branches.
The prototype-resolution/fallback logic (Lines 553-571 and 616-632) is identical except for the fallback prototype name. Consider extracting a shared helper to avoid future divergence.
♻️ Suggested helper extraction
+fn resolve_inherited_via_prototype( + addr: usize, + key: *const crate::StringHeader, + fallback_name: &str, +) -> Option<JSValue> { + let proto = match super::super::prototype_chain::object_static_prototype(addr) { + Some(crate::value::TAG_NULL) => None, + Some(bits) => Some(f64::from_bits(bits)), + None => Some(crate::object::builtin_prototype_value(fallback_name)), + }; + let proto_value = JSValue::from_bits(proto?.to_bits()); + if proto_value.is_pointer() { + let inherited = super::super::js_object_get_field_by_name( + proto_value.as_pointer::<ObjectHeader>(), + key, + ); + if !inherited.is_undefined() { + return Some(inherited); + } + } + None +}Then both call sites reduce to:
- let proto = match super::super::prototype_chain::object_static_prototype(addr) { - Some(crate::value::TAG_NULL) => None, - Some(bits) => Some(f64::from_bits(bits)), - None => Some(crate::object::builtin_prototype_value( - crate::typedarray::name_for_kind(kind), - )), - }; - if let Some(proto) = proto { - let proto_value = JSValue::from_bits(proto.to_bits()); - if proto_value.is_pointer() { - let inherited = super::super::js_object_get_field_by_name( - proto_value.as_pointer::<ObjectHeader>(), - key, - ); - if !inherited.is_undefined() { - return inherited; - } - } - } + if let Some(inherited) = + resolve_inherited_via_prototype(addr, key, crate::typedarray::name_for_kind(kind)) + { + return inherited; + }Also applies to: 611-632
🤖 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 `@crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs` around lines 544 - 571, Extract the duplicated prototype-resolution and inherited-property lookup logic from the typed-array and buffer branches into a shared helper near the existing field-get utilities. Parameterize the helper with the fallback prototype value or name, then replace both blocks around the typed-array branch and buffer branch with calls to it while preserving custom prototypes, null handling, and undefined-result behavior.
🤖 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 `@crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs`:
- Around line 544-571: Prevent unbounded prototype recursion in the TypedArray
fallback lookup and the custom constructor-prototype hop around
resolve_inherited_field. Reuse the existing max-depth/visited tracking approach
to detect cycles and return undefined when a prototype repeats or the limit is
reached, while preserving inherited property resolution for valid chains.
In `@crates/perry-stdlib/src/worker_threads.rs`:
- Around line 598-651: Exclude generic typed arrays from
message_value_is_uncloneable by checking lookup_typed_array_kind(raw) alongside
is_registered_buffer, while preserving existing cloneability handling. In
crates/perry/tests/issue_6763_broadcast_clone.rs:12-122, extend the fixture to
post a non-Uint8Array typed array such as Int32Array both top-level and nested
in a plain object, asserting both clones round-trip correctly.
- Around line 511-519: Update SerializedMessage and the serialized_message path
to preserve BigInt64Array and BigUint64Array values without converting lanes
through f64. For these typed-array kinds, use a 64-bit-backed snapshot
representation and reconstruct the corresponding typed array with exact integer
values; alternatively, bypass this snapshot path for them. Keep existing
behavior unchanged for other typed-array kinds and JSON values.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs`:
- Around line 544-571: Extract the duplicated prototype-resolution and
inherited-property lookup logic from the typed-array and buffer branches into a
shared helper near the existing field-get utilities. Parameterize the helper
with the fallback prototype value or name, then replace both blocks around the
typed-array branch and buffer branch with calls to it while preserving custom
prototypes, null handling, and undefined-result behavior.
In `@crates/perry/tests/issue_6763_broadcast_clone.rs`:
- Around line 12-122: Mirror the coverage from
broadcast_and_port_clone_preserve_binary_values_and_errors in a unit test
visible to cargo-test, rather than relying solely on the integration test under
crates/*/tests. Preserve assertions for typed-array cloning, closed-port errors,
markAsUncloneable behavior, and ArrayBuffer cloning, while keeping the existing
integration coverage unchanged unless reuse is straightforward.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc50bade-0204-49a7-afa9-66bcb29090fc
📒 Files selected for processing (5)
changelog.d/7091-worker-structured-clone.mdcrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-stdlib/src/worker_threads.rscrates/perry-stdlib/src/worker_threads/channel_pump.rscrates/perry/tests/issue_6763_broadcast_clone.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs`:
- Around line 555-579: Update both fallback paths in get_field_by_name.rs at
lines 555-579 and 624-646 to use a receiver-aware inherited-lookup helper,
passing addr as the accessor receiver instead of invoking
js_object_get_field_by_name with the builtin prototype; apply this for both the
per-kind TypedArray prototype and the Uint8Array prototype.
In `@crates/perry-runtime/src/object/prototype_chain.rs`:
- Around line 47-86: Replace the raw usize owner identities used by
PrototypeResolutionGuard and PROTOTYPE_RESOLUTION_STACK with GC-stable,
rooted/updateable object handles or the runtime’s stable object identity
mechanism, so identities remain valid across accessor/proxy-triggered moving GC
and reentrant lookups. Update enter, Drop, resolution_stack_savepoint, and
resolution_stack_restore consistently, and add a regression test covering GC
reentry during js_object_get_field_by_name that verifies cyclic prototype
resolution terminates.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4489994-d535-4c9f-a180-8ae535599246
📒 Files selected for processing (7)
crates/perry-runtime/src/exception.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/prototype_chain.rscrates/perry-runtime/src/typedarray/access.rscrates/perry-runtime/src/typedarray/mod.rscrates/perry-stdlib/src/worker_threads.rscrates/perry/tests/issue_6763_broadcast_clone.rs
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)
crates/perry-runtime/src/object/prototype_chain.rs (1)
438-442: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot saved receiver state across reentrant property reads. Both paths restore a raw saved receiver after an operation that can invoke user code and trigger moving GC; that saved value is neither rooted nor rewritten.
crates/perry-runtime/src/object/prototype_chain.rs#L438-L442: storeprevious_thisin an updateable runtime handle beforejs_proxy_get, then reload before restoring it.crates/perry-runtime/src/object/prototype_chain.rs#L458-L466: do the same around recursive lookup, includingprev_overridewhen it holds a JSValue.Based on learnings, raw Rust pointer locals are neither GC roots nor reliable pins across user-code-invoking operations.
🤖 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 `@crates/perry-runtime/src/object/prototype_chain.rs` around lines 438 - 442, Update both receiver-saving paths in crates/perry-runtime/src/object/prototype_chain.rs at lines 438-442 and 458-466: store previous_this in an updateable runtime handle before js_proxy_get, and reload it before restoring after the reentrant operation. Apply the same handle-based treatment around recursive lookup, including prev_override when it contains a JSValue; do not restore raw pointer locals across user-code-invoking calls.Source: Learnings
🤖 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 `@crates/perry-runtime/src/object/prototype_chain.rs`:
- Around line 438-442: Update both receiver-saving paths in
crates/perry-runtime/src/object/prototype_chain.rs at lines 438-442 and 458-466:
store previous_this in an updateable runtime handle before js_proxy_get, and
reload it before restoring after the reentrant operation. Apply the same
handle-based treatment around recursive lookup, including prev_override when it
contains a JSValue; do not restore raw pointer locals across user-code-invoking
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a574d51-0b30-406f-9130-2feb6ce40766
📒 Files selected for processing (5)
crates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/tests/runtime_roots.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/prototype_chain.rscrates/perry/tests/issue_6763_broadcast_clone.rs
Contributor
Author
|
Addressed the late CodeRabbit outside-diff finding in 17c3566: both inherited-lookup paths now root and reload the saved implicit receiver state across reentrant Proxy/accessor calls, including the optional accessor override. Validation: cargo fmt/check, both prototype-resolution GC root tests, and the full issue_6763_broadcast_clone integration test (1 passed, 156.55s). |
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DataCloneError/InvalidStateErrorDOMExceptions and validate a closed BroadcastChannel before cloneabilityThis is a focused increment on the worker_threads parity umbrella; it does not close the remaining constructor, EventTarget, transfer-list, or nested structured-clone gaps.
Refs #6763
Verification
cargo check -p perry-runtime -p perry-stdlibcargo test -p perry --test issue_6763_broadcast_clone -- --nocaptureworker_threads/broadcast-channel/close-and-clone.tsworker_threads/broadcast-channel/error-precedence.tsworker_threads/transfer-markers/uncloneable-post.tsSummary by CodeRabbit
worker_threadsstructured cloning to preserveArrayBuffer/typed-array contents and bigint typed-array identity.DataCloneError.Uint8Array) and improved prototype-cycle handling.BroadcastChannelnow throwsInvalidStateError.