Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions changelog.d/7815-constructor-receiver-rooting.md
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).

Copy link
Copy Markdown

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 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


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.
10 changes: 10 additions & 0 deletions crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,15 @@ pub extern "C" fn js_boxed_number_new(value: f64) -> f64 {
#[no_mangle]
pub extern "C" fn js_boxed_string_new(value: f64, has_arg: i32) -> f64 {
let obj = crate::object::js_object_alloc(CLASS_ID_BOXED_STRING, 0);
// #6949(b): both branches below allocate — `js_string_from_bytes` for the
// empty-string case and `js_string_coerce` otherwise, the latter running a
// user `toString`/`valueOf` for a POINTER_TAG value — so either can collect
// and EVACUATE while `obj` sits in a raw Rust local. Every use below
// (`register_boxed_primitive_payload`, the two `install_string_wrapper_*`
// calls, `attach_boxed_primitive_prototype`, and the returned NaN-box)
// dereferences or keys on it.
let scope = crate::gc::RuntimeHandleScope::new();
let obj_handle = scope.root_raw_mut_ptr(obj);
// `new String()` (no args) is spec'd to box "", not "undefined".
let ptr = if has_arg == 0 {
crate::string::js_string_from_bytes(std::ptr::null(), 0)
Expand All @@ -309,6 +318,7 @@ pub extern "C" fn js_boxed_string_new(value: f64, has_arg: i32) -> f64 {
}
js_string_coerce(value)
};
let obj = obj_handle.get_raw_mut_ptr::<crate::object::ObjectHeader>();
let boxed = f64::from_bits(crate::value::JSValue::string_ptr(ptr).bits());
register_boxed_primitive_payload(obj, boxed);
install_string_wrapper_indices(obj, ptr);
Expand Down
19 changes: 19 additions & 0 deletions crates/perry-runtime/src/disposable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,25 @@ pub extern "C" fn js_suppressed_error_new(error: f64, suppressed: f64, message:
// properties { writable:true, enumerable:false, configurable:true }. The
// `name` default ("SuppressedError") lives on `SuppressedError.prototype`,
// so it is *not* set as an own property here.
// #6949(b): `obj` is a raw Rust local — neither a GC root nor a shadow slot
// — and everything below it allocates: `js_string_from_bytes` per key,
// `js_object_set_field_by_name` when the object grows, and
// `js_string_coerce` on the message. Any of those can collect and EVACUATE.
//
// A single rebind after the coercion would not be enough here for two
// reasons: the closure captures `obj` BY VALUE, so every `set_nonenum` call
// would keep using the address captured at definition time; and
// `object_set_static_prototype` at the end keys a SIDE TABLE on
// `obj as usize`, so a stale address does not fault — it files the
// prototype under an address nothing will look up, and `instanceof
// SuppressedError` quietly stops resolving.
//
// So root once and re-read at every use, which is what the handle gives.
let scope = crate::gc::RuntimeHandleScope::new();
let obj_handle = scope.root_raw_mut_ptr(obj);
let set_nonenum = |key: &str, value: f64| {
let key_ptr = js_string_from_bytes(key.as_ptr(), key.len() as u32);
let obj = obj_handle.get_raw_mut_ptr::<crate::object::ObjectHeader>();
js_object_set_field_by_name(obj, key_ptr, value);
crate::object::set_property_attrs(
obj as usize,
Expand All @@ -484,11 +501,13 @@ pub extern "C" fn js_suppressed_error_new(error: f64, suppressed: f64, message:
};
set_nonenum("message", message_val);
}
let obj = obj_handle.get_raw_mut_ptr::<crate::object::ObjectHeader>();
let result = js_nanbox_pointer(obj as i64);
// Link the instance to `SuppressedError.prototype` so `name`/`message`
// defaults and `instanceof SuppressedError` resolve through the chain.
let proto = crate::object::builtin_prototype_value("SuppressedError");
if proto.to_bits() != TAG_UNDEFINED && js_nanbox_get_pointer(proto) != 0 {
let obj = obj_handle.get_raw_mut_ptr::<crate::object::ObjectHeader>();
crate::object::prototype_chain::object_set_static_prototype(obj as usize, proto.to_bits());
}
result
Expand Down
8 changes: 8 additions & 0 deletions crates/perry-runtime/src/messaging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.rs

Repository: 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 200

Repository: 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-runtime

Repository: 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 1000

Repository: 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' \
  $files

Repository: 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

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);
Expand Down
Loading