fix(runtime): root constructor receivers across a later ToString coercion (#6949 shape b) - #7815
Open
proggeramlug wants to merge 1 commit into
Open
fix(runtime): root constructor receivers across a later ToString coercion (#6949 shape b)#7815proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThree runtime constructors now root newly allocated receivers during coercions that can trigger garbage collection. They reload relocated pointers before initialization continues. A changelog entry documents the changes and verification results. ChangesGC-safe object initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
proggeramlug
force-pushed
the
fix/6949b-constructor-receiver-rooting
branch
from
August 10, 2026 23:34
85c89c3 to
b1eec82
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: 2
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/builtins/formatting/boxed_primitives.rs (1)
321-325: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRoot each coerced string until its destination owns it.
Rooting
objprotects only the receiver. The coercion result can also relocate before installation. Root the string value with the sameRuntimeHandleScope, then reload it after every allocation before dereferencing or storing it.
crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs#L321-L325: Rootptrand reload it inside both wrapper-install helpers, because they allocate keys and reuse the string pointer.crates/perry-runtime/src/disposable.rs#L480-L502: Rootmessage_valbeforeset_nonenum, becauseset_nonenumallocates its key before it stores the value.crates/perry-runtime/src/messaging.rs#L616-L619: Root the coerced channel name untilset_fieldhas installed it.Based on learnings: raw Rust pointer locals and NaN-boxed values are not GC roots across allocating or 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/builtins/formatting/boxed_primitives.rs` around lines 321 - 325, Root every coerced string value with the existing RuntimeHandleScope before operations that may allocate, and reload it before use: in crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs#L321-L325, update both wrapper-install helpers to use the reloaded rooted ptr; in crates/perry-runtime/src/disposable.rs#L480-L502, root message_val before set_nonenum and preserve that rooted value through storage; in crates/perry-runtime/src/messaging.rs#L616-L619, root the coerced channel name until set_field completes.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
changelog.d/7815-constructor-receiver-rooting.md (1)
1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce this fragment to shipped behavior.
Remove the internal GC analysis, test counts, issue references, and deferred-file narrative. Keep one concise release-note entry that describes the runtime fix.
🤖 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 `@changelog.d/7815-constructor-receiver-rooting.md` around lines 1 - 13, Replace the changelog fragment with one concise release-note entry describing that the affected constructors now safely root newly allocated receivers across coercion calls, preventing invalid references during garbage collection. Remove the internal GC analysis, test results, issue and PR references, and deferred-file discussion.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.
Inline comments:
In `@changelog.d/7815-constructor-receiver-rooting.md`:
- Line 1: Rename the changelog fragment from the 7815-prefixed filename to
changelog.d/6949-constructor-receiver-rooting.md so it uses the current PR key
`#6949`; leave the fragment content unchanged.
In `@crates/perry-runtime/src/messaging.rs`:
- Around line 609-617: Root obj immediately after js_object_alloc and keep the
root alive through the entire BroadcastChannel construction. Root name_ptr as
well, and reload obj_handle’s pointer after every potentially allocating
operation, including constructor_prototype, key(name), closure_value, set_field,
and install_method, before using it; reload obj again before boxing the result.
---
Outside diff comments:
In `@crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs`:
- Around line 321-325: Root every coerced string value with the existing
RuntimeHandleScope before operations that may allocate, and reload it before
use: in
crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs#L321-L325,
update both wrapper-install helpers to use the reloaded rooted ptr; in
crates/perry-runtime/src/disposable.rs#L480-L502, root message_val before
set_nonenum and preserve that rooted value through storage; in
crates/perry-runtime/src/messaging.rs#L616-L619, root the coerced channel name
until set_field completes.
---
Nitpick comments:
In `@changelog.d/7815-constructor-receiver-rooting.md`:
- Around line 1-13: Replace the changelog fragment with one concise release-note
entry describing that the affected constructors now safely root newly allocated
receivers across coercion calls, preventing invalid references during garbage
collection. Remove the internal GC analysis, test results, issue and PR
references, and deferred-file discussion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7328b5d3-0312-4947-b898-5b9995348a43
📒 Files selected for processing (4)
changelog.d/7815-constructor-receiver-rooting.mdcrates/perry-runtime/src/builtins/formatting/boxed_primitives.rscrates/perry-runtime/src/disposable.rscrates/perry-runtime/src/messaging.rs
| @@ -0,0 +1,13 @@ | |||
| **Three constructors now root their freshly-allocated receiver across a later ToString coercion** (#6949 shape b). | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the current PR key in the fragment path.
This PR is identified as #6949, but this fragment is named changelog.d/7815-constructor-receiver-rooting.md. Rename it to changelog.d/6949-constructor-receiver-rooting.md so the changeset is attributed to the correct PR.
🤖 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 `@changelog.d/7815-constructor-receiver-rooting.md` at line 1, Rename the
changelog fragment from the 7815-prefixed filename to
changelog.d/6949-constructor-receiver-rooting.md so it uses the current PR key
`#6949`; leave the fragment content unchanged.
Sources: Coding guidelines, Learnings
Comment on lines
+609
to
+617
| // #6949(b): `js_string_coerce` allocates for every shape except an | ||
| // already-heap STRING_TAG value, so it can collect and EVACUATE — and | ||
| // `obj`, allocated a few lines up, is a raw Rust local: neither a GC root | ||
| // nor a shadow slot. Every `set_field`/`install_method` below writes | ||
| // through it. Root it across the coercion and re-read. | ||
| let scope = crate::gc::RuntimeHandleScope::new(); | ||
| let obj_handle = scope.root_raw_mut_ptr(obj); | ||
| let name_ptr = crate::builtins::js_string_coerce(name); | ||
| let obj = obj_handle.get_raw_mut_ptr::<object::ObjectHeader>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether the pre-root initialization helper can allocate or invoke GC.
rg -n -C 8 'fn\s+(set_field|key)\b|set_field\s*\(' crates/perry-runtime/src/messaging.rsRepository: PerryTS/perry
Length of output: 7838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant runtime APIs and implementations ---'
rg -n -C 12 \
'fn\s+(js_object_set_field_by_name|js_object_define_accessor|js_object_alloc|set_object_prototype|js_string_from_bytes|js_string_coerce|get_global_constructor)\b|struct\s+RuntimeHandleScope|impl\s+RuntimeHandleScope|root_raw_mut_ptr|get_raw_mut_ptr|gc_register_mutable_root_scanner|EVACUATE|evacuate' \
crates/perry-runtime/src crates/perry-runtime 2>/dev/null | head -n 1200
printf '%s\n' '--- focused object and GC source files ---'
git ls-files 'crates/perry-runtime/src/**/*.rs' | grep -E '/(object|gc|builtins|value|messaging)(/|\.rs)' | head -n 200Repository: PerryTS/perry
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- messaging dependencies ---'
rg -n -C 10 \
'^(fn|pub .*fn|unsafe fn|extern .*fn).*(set_field|key|get_global_constructor|js_object_set_field_by_name|set_object_prototype|js_string_from_bytes|js_string_coerce|js_object_alloc)|js_object_set_field_by_name|set_object_prototype' \
crates/perry-runtime/src/messaging.rs crates/perry-runtime/src/object.rs crates/perry-runtime/src/gc.rs crates/perry-runtime/src/builtins.rs
printf '%s\n' '--- exact RuntimeHandleScope implementation ---'
rg -n -C 25 \
'struct RuntimeHandleScope|impl RuntimeHandleScope|fn root_raw_mut_ptr|fn get_raw_mut_ptr|fn across_mut' \
crates/perry-runtime/src/gc.rs
printf '%s\n' '--- exact object field setter implementation ---'
rg -n -C 30 \
'fn js_object_set_field_by_name|pub.*js_object_set_field_by_name|unsafe.*js_object_set_field_by_name' \
crates/perry-runtime/src crates/perry-runtimeRepository: PerryTS/perry
Length of output: 7986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runtime source layout ---'
git ls-files 'crates/perry-runtime/**' | grep -E '(^|/)(gc|object|builtins|value)(/|\.rs$)' | head -n 200
printf '%s\n' '--- definitions of the referenced APIs ---'
rg -n -C 18 \
'fn\s+(js_object_set_field_by_name|js_object_alloc|js_string_from_bytes|js_string_coerce)\b|pub\s+(unsafe\s+)?fn\s+(js_object_set_field_by_name|js_object_alloc|js_string_from_bytes|js_string_coerce)\b|root_raw_mut_ptr|get_raw_mut_ptr|struct\s+RuntimeHandleScope|impl\s+RuntimeHandleScope' \
crates/perry-runtime --glob '*.rs' | head -n 1000Repository: PerryTS/perry
Length of output: 11041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=$(rg -l \
'js_object_set_field_by_name|js_object_alloc|js_string_from_bytes|js_string_coerce|struct RuntimeHandleScope|root_raw_mut_ptr' \
crates/perry-runtime/src --glob '*.rs')
printf '%s\n' '--- matching files ---'
printf '%s\n' "$files"
printf '%s\n' '--- focused definitions ---'
rg -n -C 22 \
'fn\s+(js_object_set_field_by_name|js_object_alloc|js_string_from_bytes|js_string_coerce)\b|pub\s+(unsafe\s+)?fn\s+(js_object_set_field_by_name|js_object_alloc|js_string_from_bytes|js_string_coerce)\b|struct\s+RuntimeHandleScope|impl\s+RuntimeHandleScope|fn\s+root_raw_mut_ptr|fn\s+get_raw_mut_ptr' \
$filesRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- field setter ---'
cat -n crates/perry-runtime/src/object/field_set_by_name.rs | sed -n '33,145p'
printf '%s\n' '--- object allocation ---'
cat -n crates/perry-runtime/src/object/alloc.rs | sed -n '1,130p'
printf '%s\n' '--- string coercion ---'
cat -n crates/perry-runtime/src/builtins/numbers.rs | sed -n '625,705p'
printf '%s\n' '--- string allocation and interning ---'
rg -n -C 18 \
'pub extern "C" fn js_string_from_bytes|fn js_string_from_bytes|js_string_materialize_to_heap|fn.*intern|intern.*string' \
crates/perry-runtime/src/string crates/perry-runtime/src --glob '*.rs' | sed -n '1,500p'Repository: PerryTS/perry
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- remaining field-setter paths ---'
cat -n crates/perry-runtime/src/object/field_set_by_name.rs | sed -n '140,330p'
cat -n crates/perry-runtime/src/object/field_set_by_name/tail.rs | sed -n '1,260p'
cat -n crates/perry-runtime/src/object/field_set_by_name/write_helpers.rs | sed -n '1,220p'
printf '%s\n' '--- allocation bodies ---'
cat -n crates/perry-runtime/src/object/alloc.rs | sed -n '115,245p'
cat -n crates/perry-runtime/src/string/alloc.rs | sed -n '1,18p'
cat -n crates/perry-runtime/src/string/alloc.rs | sed -n '124,170p'
printf '%s\n' '--- messaging constructor sequence ---'
cat -n crates/perry-runtime/src/messaging.rs | sed -n '597,630p'Repository: PerryTS/perry
Length of output: 47461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
messaging = Path("crates/perry-runtime/src/messaging.rs").read_text()
setter = Path("crates/perry-runtime/src/object/field_set_by_name.rs").read_text()
tail = Path("crates/perry-runtime/src/object/field_set_by_name/tail.rs").read_text()
string_alloc = Path("crates/perry-runtime/src/string/alloc.rs").read_text()
numbers = Path("crates/perry-runtime/src/builtins/numbers.rs").read_text()
def body(text, signature):
start = text.index(signature)
brace = text.index("{", start)
depth = 0
for i in range(brace, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
return text[brace:i + 1]
raise AssertionError(f"unterminated body: {signature}")
key_body = body(messaging, "fn key(name: &str)")
set_field_body = body(messaging, "fn set_field(obj:")
coerce_body = body(numbers, "pub extern \"C\" fn js_string_coerce")
setter_body = body(setter, "pub extern \"C\" fn js_object_set_field_by_name")
tail_body = body(tail, "pub(super) fn set_field_by_name_object_tail")
assert "js_string_from_bytes" in key_body
assert "key(name)" in set_field_body
assert "js_object_set_field_by_name" in set_field_body
assert "js_string_from_bytes_with_capacity" in body(string_alloc, "pub extern \"C\" fn js_string_from_bytes")
assert "string_storage_alloc" in body(string_alloc, "pub extern \"C\" fn js_string_from_bytes_with_capacity")
assert "return jsval.as_string_ptr()" in coerce_body
assert "js_string_materialize_to_heap" in coerce_body
assert "js_string_from_bytes(result.as_ptr()" in coerce_body
assert "RuntimeHandleScope::new()" in tail_body
assert "root_raw_mut_ptr(obj)" in tail_body
assert "root_string_ptr(key)" in tail_body
# Check the caller's order: the handle is created only after the first store.
broadcast = body(messaging, "pub extern \"C\" fn js_broadcast_channel_new")
root_pos = broadcast.index("root_raw_mut_ptr(obj)")
first_store_pos = broadcast.index('set_field(\n obj,\n "constructor"')
assert first_store_pos < root_pos
print("key() allocates before js_object_set_field_by_name() receives obj")
print("js_string_coerce() allocates for non-heap-string inputs")
print("js_object_set_field_by_name() roots obj only after its caller-side key allocation")
print("broadcast constructor performs set_field(obj, ...) before root_raw_mut_ptr(obj)")
print("coerced name_ptr is not rooted before the next set_field()")Repository: PerryTS/perry
Length of output: 581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n crates/perry-runtime/src/messaging.rs | sed -n '88,125p'Repository: PerryTS/perry
Length of output: 1647
Root obj for the complete BroadcastChannel construction.
Create the handle immediately after js_object_alloc. constructor_prototype, key(name), and closure_value can allocate before the setter receives obj. Make set_field and install_method re-read the pointer after each allocation, root the coerced name_ptr, and reload obj before boxing the result.
🤖 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/messaging.rs` around lines 609 - 617, Root obj
immediately after js_object_alloc and keep the root alive through the entire
BroadcastChannel construction. Root name_ptr as well, and reload obj_handle’s
pointer after every potentially allocating operation, including
constructor_prototype, key(name), closure_value, set_field, and install_method,
before using it; reload obj again before boxing the result.
Sources: Coding guidelines, Learnings
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.
Three constructors now root their freshly-allocated receiver across a later ToString coercion (#6949 shape b).
The shape:
js_object_allocinto a raw Rust local, thenjs_string_coerce(or another allocating call) further down, then writes through that local.js_string_coercereturns without allocating only for an already-heapSTRING_TAGvalue; every other shape allocates — an SSO string materialises, a number/bool/null/BigInt builds its stringification, aPOINTER_TAGobject runs a usertoString/valueOf— and any of those can collect and evacuate. A raw local is neither a GC root nor a shadow slot.messaging.rsjs_broadcast_channel_new—objallocated, then the channelnamecoerced, then eightset_field/install_methodwrites throughobj.builtins/formatting/boxed_primitives.rsjs_boxed_string_new—objallocated, then both branches allocate (js_string_from_bytesfornew String(),js_string_coerceotherwise), then the payload registration, the twoinstall_string_wrapper_*calls and the prototype attach all useobj.disposable.rsjs_suppressed_error_new— needed more than a rebind, for two reasons worth recording. Itsset_nonenumclosure capturesobjby value, so a single re-read after the coercion would leave every property write using the address captured at definition time. Andobject_set_static_prototype(obj as usize, …)keys a side table on the address, so a stale one does not fault — it files the prototype under an address nothing looks up, andinstanceof SuppressedErrorquietly stops resolving. The handle is therefore re-read at every use rather than once.Same
RuntimeHandleScopeidiom #6943 established, and the same honest caveat as #7811 (shape a): no failing witness. A fixture driving all of these with non-string arguments matches Node exactly on both arms. The window needs the pointee to move during that specific coercion, and this family is documented as invisible to runtime probes at the moment of collection; the justification is the repo's own rooting invariant — a raw heap pointer held across a call that can allocate is a defect regardless of whether today's allocator layout exposes it.One site from the issue's shape-(b) list is deliberately not here.
object/class_registry/construct.rs's rebound-RegExparm (where the coercedpatternspans theflagscoercion) is a real instance and the fix is written, but that file sits at 1999 lines against the 2000-line CI cap, so any addition tripscheck_file_size.sh. PR #7779 already restructures that file for #7524; this site should land on top of it rather than fight the cap twice in two PRs.Verified:
cargo test -p perry-runtime --lib2051 passed / 0 failed;test_gap_regexp2/2,test_gap_disposable1/1,test_gap_string5/5,test_gap_error2/2; fmt and file-size clean.Summary by CodeRabbit
Bug Fixes
Documentation