-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): root constructor receivers across a later ToString coercion (#6949 shape b) #7815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| **Three constructors now root their freshly-allocated receiver across a later ToString coercion** (#6949 shape b). | ||
|
|
||
| The shape: `js_object_alloc` into a raw Rust local, then `js_string_coerce` (or another allocating call) further down, then writes through that local. `js_string_coerce` returns without allocating only for an already-heap `STRING_TAG` value; every other shape allocates — an SSO string materialises, a number/bool/null/BigInt builds its stringification, a `POINTER_TAG` object runs a user `toString`/`valueOf` — and any of those can collect and **evacuate**. A raw local is neither a GC root nor a shadow slot. | ||
|
|
||
| * **`messaging.rs` `js_broadcast_channel_new`** — `obj` allocated, then the channel `name` coerced, then eight `set_field`/`install_method` writes through `obj`. | ||
| * **`builtins/formatting/boxed_primitives.rs` `js_boxed_string_new`** — `obj` allocated, then *both* branches allocate (`js_string_from_bytes` for `new String()`, `js_string_coerce` otherwise), then the payload registration, the two `install_string_wrapper_*` calls and the prototype attach all use `obj`. | ||
| * **`disposable.rs` `js_suppressed_error_new`** — needed more than a rebind, for two reasons worth recording. Its `set_nonenum` closure captures `obj` **by value**, so a single re-read after the coercion would leave every property write using the address captured at definition time. And `object_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, and `instanceof SuppressedError` quietly stops resolving. The handle is therefore re-read at every use rather than once. | ||
|
|
||
| Same `RuntimeHandleScope` idiom #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-`RegExp` arm (where the coerced `pattern` spans the `flags` coercion) 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 trips `check_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 --lib` 2051 passed / 0 failed; `test_gap_regexp` 2/2, `test_gap_disposable` 1/1, `test_gap_string` 5/5, `test_gap_error` 2/2; fmt and file-size clean. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -606,7 +606,15 @@ pub extern "C" fn js_broadcast_channel_new(name: f64) -> f64 { | |
| "constructor", | ||
| get_global_constructor("BroadcastChannel"), | ||
| ); | ||
| // #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>(); | ||
|
Comment on lines
+609
to
+617
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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 Create the handle immediately after 🤖 Prompt for AI AgentsSources: Coding guidelines, Learnings |
||
| let name_value = f64::from_bits(JSValue::string_ptr(name_ptr).bits()); | ||
| set_field(obj, "name", name_value); | ||
| install_method(obj, "close", noop0 as *const u8, 0); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 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 namedchangelog.d/7815-constructor-receiver-rooting.md. Rename it tochangelog.d/6949-constructor-receiver-rooting.mdso the changeset is attributed to the correct PR.🤖 Prompt for AI Agents
Sources: Coding guidelines, Learnings