Merge train 219: implicit-this/new.target exception safety, event-dispatch rooting, instanceof split, shadow-stack and regex perf, AsyncResource and heritage fixes (v0.5.1597) - #10710
Merged
Conversation
Several event/listener dispatch loops clone listener callbacks and/or call arguments into plain Rust locals (a Vec<Listener>, a Vec<f64>, or a raw array pointer), then call into user code that can allocate and trigger a moving minor collection, then reuse those unrooted copies for the next listener. Root every such copy through a RuntimeHandleScope and re-read the current (possibly relocated) value before each dispatch, instead of trusting the pre-call copy. Reproduced at crates/perry-runtime/src/node_stream_event_emitter.rs's emit_stream_event/call_listener_args (the path `class X extends EventEmitter` actually dispatches through): a 3-listener emitter whose second listener allocates heavily segfaults dereferencing the third listener's stale closure pointer, reliably, with no GC env knobs. Gone under PERRY_GEN_GC=0 (full mark-sweep, non-moving). Under PERRY_GC_DIAG=1 PERRY_GC_PROTECT_FROMSPACE=1 the from-space quarantine reports the exact fault: a retired-from-space deref of a GC_TYPE_CLOSURE object, matching a symbolized backtrace through js_native_call_value <- call_listener_args <- emit_stream_event. The same pattern is fixed at the four other sites an earlier code-read audit named: perry-stdlib's events.rs (js_event_emitter_emit/emit0, dispatch_error_monitor, emit_meta_event), domain.rs (emit_domain_event), worker_threads/worker_surface.rs (stream_emit_event), and events/warnings.rs (emit_warning). events.rs's own asynchronous dispatch branch already rooted its callback/receiver/ args via js_async_resource_run_in_async_scope; the synchronous branch did not, which is the same finding by construction, independent of the runtime repro.
#8595's entry outliner only ever chunked hir.init. For a CommonJS module, cjs_wrap::wrap_commonjs_for_target wraps the whole body as text inside a `function __perry_cjs_factory() {...}` closure nested in an anonymous IIFE; hir.init ends up with only a handful of wrapper statements, so admission never fired and the real body stayed one giant function. On typescript 5.9.3's _tsc.js this was a single 463,716-instruction/6.30 MB closure, past the machine-pipeline budget. find_cjs_factory_closure(_mut) locates that closure by walking hir.init's statement/expression tree (it is a Stmt::Let naming an Expr::Closure, not a hir.functions entry, since it is lexically nested). outline_entry_module now tries hir.init first (unchanged #8595 behavior) and falls back to the factory's body with the identical chunk_statements/analyze_stmts_outlining machinery, so a module is only ever outlined from one origin per compile. The factory always captures its own name from the wrapper's IIFE scope (`__cjs_module.__perry_cjs_factory = __perry_cjs_factory;`, which perry-runtime's module_require.rs calls through on a circular-require recovery path). A chunk is a plain, non-capturing function and can't read a captured id, so classify_for_chunking keeps any statement referencing one inline in the residual body rather than promoting it to a module global -- a global would turn a per-invocation-fresh capture into one program-wide instance and could silently break that recovery path. module_globals_emit.rs folds the factory's own logical statements into the same cross-chunk-let promotion emit_module_globals already does for hir.init, so a var shared across the factory's new chunks gets the same @perry_global_* treatment hir.init cross-chunk lets get. Verified on a synthetic 2107-statement CJS fixture (cross-chunk vars plus a closure created early and invoked from far-later statements) against Node's own output, and on a full typescript 5.9.3 build: the 463,716-instruction closure is gone, entry-outline reports "cjs factory: ... candidate=true", nm shows __perry_entry_chunk_* symbols, and `--noEmit demo.ts` / `--version` output and exit codes are byte-identical to before.
… numeric conversion refresh() called set_timer_ref_state(id, true) unconditionally, so refreshing an unrefd Timeout/Interval re-refd it -- hasRef() flipped to true and the process stayed alive for a callback that had been deliberately detached from the event loop. Node refresh() reschedules only and never touches ref state; drop the forced ref-state write and let the existing entry (set at schedule time, updated by any ref()/unref() since, pinned while the timer is queued) stand. js_number_coerce gave setImmediate handles the same numeric-conversion shortcut as Timeout handles, so +setImmediate(...) returned a number instead of NaN. Node only gives Timeout (setTimeout/setInterval) a numeric conversion; Immediate has none. Add is_immediate_timer_id and gate the shortcut on it so an Immediate falls through to the generic toPrimitive/toString path, which already yields NaN. Fixes #10541 Fixes #10542
…lue, not just the bare-import name class X extends AsyncResource threw "Class constructor AsyncResource cannot be invoked without 'new'" at super() for every heritage shape except a bare import binding. A local alias, a namespace member, and a CJS destructured require() all resolve to the identical bound native export value the canonical import does, but only the bare import shape was recognized statically at HIR-lowering time, so super() fell through to a plain call of the export -- which AsyncResource throws on by design. Recognize the bound export VALUE in js_fetch_or_value_super, exactly as the existing WASI arm does, and run the same native-backing init the canonical path already uses.
…per-evaluation identity
A class expression returned from a function (a mixin/factory —
function withCommands(Base) { return class extends Base {}; }) had no
per-evaluation identity when the function lived in a non-entry module:
every call returned the SAME shared-template class object, re-parented
to the most recently passed Base. specialize_captured_class_factories
already fixes this for same-module callers by cloning a distinct class
per call site, but it only ever sees call sites in the SAME module as
the factory -- a caller in another module reaches the factory through
an ordinary cross-module call that pass never visits, so an exported
factory's own template stayed shared and got silently re-parented on
each call.
Give an exported factory real per-evaluation identity directly: when
its body is nothing but the single-statement
return class extends <expr> {} shape, upgrade the class's own
ClassRef to ClassExprFresh, exactly what the same class expression
would already lower to had it needed per-evaluation statics/captures/a
private brand. This closes the gap for every caller, local or
cross-module, without touching the existing same-module
specialization (a locally-cloned call site never calls the factory at
runtime at all, so it is unaffected).
A subclass constructor assigning this.<name> where <name> is a method inherited from a parent class allocated an own inline field slot for it, hiding the inherited method from the moment super() returned. Track own+inherited instance method names per class (mirroring the existing accessor-name tracking) and consult the union when deciding whether a constructor-body this.<name> = ... assignment is a new data field or a method override.
A hoisted var reaches lower_let as two Stmt::Lets sharing one local id (a body-entry predefine, then the real declaration); the second takes the #1803 redeclaration early return before ctx.local_types is updated. proven_local_types (consulted by is_numeric_expr) IS refreshed on redeclaration, but local_types (consulted by expr_may_return_boxed_value_from_raw_f64_fallback) was not, so the two predicates disagreed about the same local: a strict-equality compare against an out-of-bounds/hole read of a var-declared number array took the bare-fcmp numeric fast path, which cannot represent the NaN-boxed undefined tag such a read can produce. Refresh local_types on the redeclaration path too.
… parent
A subclass with no explicit constructor, extending a capture-bearing
class expression held in a local (const Base = class {...}; const Sub
= class extends Base {...};), never forwarded Base's captured
enclosing-scope locals to the synthesized subclass constructor: the
lowering deliberately drops the static extends_name for such a
lexically-local heritage identifier (avoiding a same-named-class
collision, #5437), and capture propagation was keyed off that same
name. Resolve the heritage identifier through resolve_class_alias
instead - the same table Expr::New's own capture lookup already uses
for let X = class {...}; new X() - for capture forwarding only,
gated to subclasses with no own constructor (an explicit constructor
already forwards captures correctly via a separate mechanism).
…uiltin namespace Math[k], JSON[k], Object[k] and other builtin namespace/constructor member reads with a non-literal computed key collapsed to the bare GlobalGet(0) intrinsic sentinel, so the read landed on the number 0 instead of the real object (Math[key](x) threw "(number).x is not a function"). #973's value-form reroute wraps these idents as PropertyGet{GlobalGet(0), name}; member_tail.rs undoes that reroute in member-object position so the intrinsic call/constant-fold paths for a STATICALLY-KNOWN member name (Math.max(...)) keep their pre-#973 bare receiver. That undo is only safe when the member name is known at lowering time -- outer_static_member is None for a computed non-literal key, which zeroed out the outer_is_reified_*/ outer_is_inherited_* guards instead of blocking the undo itself. Add outer_is_dynamic_computed_key to the existing conjunction so any dynamic key keeps the reified receiver, letting the runtime property lookup resolve against the real namespace/constructor object. Verified this needs no console carve-out (console[m](...) already falls back correctly to the generic dynamic-dispatch path once its receiver survives). Literal-key paths are untouched by construction -- the flag only fires for MemberProp::Computed with a non-string-literal key.
…rted sibling by value An exported function whose body references another exported function BY VALUE (`x === f`, not just as a call target) was a candidate for the cross-module function inliner, which bundled a private clone of the sibling into the destination module under a fresh symbol. Every function value materializes into a heap closure keyed by its wrapper symbol, so the clone's `f` and the canonical `f` every importer resolves through produced two distinct closures -- an in-module identity check silently disagreed with every importer's own view of the same function. gather_cross_module_functions now refuses a candidate whose dependency graph would need to bundle a separately-exported sibling referenced by value; it falls back to the ordinary cross-module call instead, which resolves through the shared canonical wrapper. Self-recursion is unaffected. Fixes #10554
Object.prototype.toString.call(x) fell through to the generic [object Object] for URL, URLSearchParams, Headers, Request, Response, FormData, Blob, File, AbortController, AbortSignal, TextEncoder, TextDecoder, EventTarget, Event and CustomEvent, and x[Symbol.toStringTag] read back undefined -- breaking the standard cross-realm type check utility/HTTP libraries use (axios decides body serialization this way). Two representations, two gaps: the Web Fetch family and TextEncoder/ TextDecoder are small-integer registry handles with no brand/property case; URL/URLSearchParams and AbortController/AbortSignal/EventTarget/ Event/CustomEvent are real objects whose instances are never linked to their .prototype via object_static_prototype, so a property installed only there would never be reached from an instance. A new web_builtin_to_string_tag answers both Object.prototype.toString and x[Symbol.toStringTag] from one place, and a real, correctly-shaped descriptor is also installed on each constructor's own .prototype for reflection. Fixes #10555
…evaluation instanceof's class-chain walk resolved a dynamic parent purely by the shared TEMPLATE class_id, so evaluating a heritage-carrying class expression more than once shadowed an EARLIER evaluation's parent once a LATER evaluation of the same factory ran. Each per-evaluation class object already pins its own heritage (js_class_object_pin_parent, consulted by super() and capture resolution since #9364); instanceof never consulted it. Pin the constructing class object onto each new instance too, and give instanceof a value-aware chain walk that prefers a pinned VALUE at each hop (falling back to the plain class_id registry once no further per-evaluation precision is available). Gated behind a monotone latch armed only when a class object is ever pinned, so the common never-evaluated-twice case pays a single idle-load check.
Post-rebase onto current main, instanceof.rs (with #10624's own subclass_of_builtin_reaches / class_chain_reaches_dynamic additions) sits at 2028 lines, over the 2000-line cap check_file_size.sh enforces. Move the two `#[no_mangle]` dispatch entry points -- js_instanceof_dynamic and js_instanceof -- verbatim into instanceof/dynamic_dispatch.rs and instanceof/static_dispatch.rs, following the class_registry.rs `<mod>.rs` + `<mod>/` split pattern already used in this crate. Each new file pulls in every helper it needs via `use super::*;`, same as every other submodule under object/. Pure relocation: no behaviour change, no reordering of logic. instanceof.rs: 907 lines. instanceof/dynamic_dispatch.rs: 406 lines. instanceof/static_dispatch.rs: 738 lines.
A pure file-relocation moves grandfathered addr_class findings to a new path, and both of this audit's mechanisms are path-keyed: - scripts/addr_class_ratchet_baseline.txt's handle-floor count for instanceof.rs (6) redistributes to instanceof.rs (2, still there) and the new instanceof/static_dispatch.rs (4, moved with js_instanceof) -- regenerated via --write-baseline and diffed to confirm no other file's baseline changed. - scripts/addr_class_allowlist.txt gets a new instanceof/static_dispatch.rs entry for the one GcHeader cast that moved there, following the same precedent already recorded for array/indexing_keyed.rs and array/indexing_proto_chain.rs's own 2,000-line-cap splits. Also cargo fmt: a double blank line left behind by the extraction.
This was referenced Sep 19, 2026
Closed
fix(runtime): AsyncResource super() via any bound-export heritage shape, not just bare import
#10621
Closed
Closed
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (71)
✨ 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 |
This was referenced Sep 19, 2026
Closed
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
2 participants
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.
This train lands 12 PRs as v0.5.1597. Each source commit is verified to preserve its patch-id and authorship.
hir.init.#10600) — roots event/listener dispatch copies across moving GC.#10541,#10542) —timeout.refresh()preserves ref state;Immediatehas no numeric conversion.#10453) —AsyncResourcesuper()via any bound export.#10455) — an exported dynamic-heritage class gets its value.#10487) — excludes inherited method names from ctor-body field detection.#10488) — refresheslocal_typeson var redeclaration.Part of #10486, not a full fix — its body names what remains, so no close keyword is carried for it.#10483) — keeps the reified receiver for a computed dynamic call.#10554) — stops cross-module inlining bundling a second copy.#10555) — addsSymbol.toStringTagto Web/runtime built-ins.#10624) — splitsobject/instanceof.rs(906 / 406 / 738) and fixes instanceof against aClassExprFresh.#10619 was pulled at the publish step
It was in this train until its author converted it to a draft after assembly:
cargo-testSIGSEGVs on that branch (signal 11 in the debug
perry-runtime --libharness, right afterasync_hooks::test_support::tests::before_after_restore_execution_ids), while the same job isgreen on four sibling PRs. A shadow-stack root-state change on a red segfault is not one to land,
and the train was re-rolled without it.
Worth noting where that was caught: the publish step re-checks every source PR's draft status
and head against the assembly record. Assembly-time checks had passed it — the PR was open and
non-draft when the train was built.
#10606's rooting fix is witnessed behaviourally, not just compiled
A rooting fix that compiles proves nothing, so its own fixture was run on both arms.
main's armfails three different ways and the train's is correct every time:
main(unfixed)900 third:t299✅899 third:t299— a silently lost listener call900 third:t299✅899 third:t299900 third:t299✅The
899matters more than the crashes: unfixed, this sometimes completes with a wrong answer —one listener call lost to an unrooted copy — which is the failure mode that survives review.
Both arms were run under forced evacuation, and
PERRY_GC_DIAG=1reports[gc-fromspace-protect] retired_set=#N19,283 times on each. So the two arms executed the samenumber of copying minors with from-space protection armed: the difference is not that the fixed arm
took an easier path, it is that the union roots a receiver the unfixed code loses. A stale
from-space pointer would have faulted at the dereference rather than passing quietly.
(The first attempt at this witness was invalid and is recorded rather than discarded: run under
PERRY_GC_SCHEDULE_RATE=1withALLOC_KB=0, all six executions timed out on both arms, and theharness's own "discriminating" check counted
main's timeout as evidence of failure — both armsproducing nothing, read as agreement. Re-run with default pacing and a 600 s budget, and a timeout
no longer counts as discrimination.)
Validation
Validated head
a10ca1d695. Five-package release build pinned and hash-verified, and re-verifiedafter the gap run (
artifacts_match_pin_after_gap=True).lint1 of 83 — only the public-benchmark freshness step known-red onmain. Read from thelog rather than inferred: on the previous train the same single
rc=1concealed three failures,one of which reached
main.runtime_preflightiscargo check --workspace --all-targetsunder-D warnings, not-p perry-runtime --lib, whichcompiles no
cfg(test)code at all and let three separate breaks through in one session.test_gap_*count, none vacuous — and10600is fix(runtime): root event/listener dispatch copies across moving GC #10606's own fixture:Issues closed by this train
A merge train closes its source PRs rather than merging them, so the
Fixes #Nkeywords in thosePR bodies never evaluate. They are carried here, on the PR that actually merges, so they fire:
Fixes #10600
Fixes #10541
Fixes #10542
Fixes #10453
Fixes #10455
Fixes #10487
Fixes #10488
Fixes #10483
Fixes #10554
Fixes #10555
Fixes #10624