fix(codegen): root the URL constructor's coerced string across base lowering (Layer 1) - #7453
Merged
Merged
Conversation
added 2 commits
August 5, 2026 16:37
…owering `js_url_coerce_string` returns a RAW StringHeader pointer, not a NaN-boxed value, so nothing else keeps it alive. In `new URL(input, base)` two collection points then stood between it and its use: lowering `base` runs arbitrary user code, and the second coercion allocates whenever `base` is not already a string. An evacuating cycle in either window left the pointer forwarded and `js_url_new_with_base` parsing freed bytes. Emitted IR before: %r23 = call js_url_coerce_string(...) %r24 = call perry_fn__mk() ; user call, may collect %r25 = call js_url_coerce_string(...) ; may collect %r26 = call js_url_new_with_base(%r23, %r25) and after: %r23 = call js_url_coerce_string(...) %r24 = call js_gc_temp_root_push(%r23) %r25 = call perry_fn__mk() %r26 = call js_url_coerce_string(%r25) %r27 = call js_gc_temp_root_get(%r24) %r28 = call js_url_new_with_base(%r27, %r26) call js_gc_temp_root_truncate(%r24) UrlPatternNew held its NaN-boxed input in a bare register across the same window; it now lowers both operands through lower_exprs_rooted. Latent, as every member of this class is: the value is only wrong if a collection lands inside the window, and at collection time there is nothing for the collector to find. The evidence is the IR, per docs/src/internals/gc-rooting-invariant.md.
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ 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
added a commit
that referenced
this pull request
Aug 5, 2026
…7454) * fix(gc-check): recognise js_url_coerce_string as a heap-value source ALLOC_RE covers fresh-string producers by naming convention. There are exactly three `coerce -> *mut StringHeader` helpers in the runtime; `js_string_coerce` and `js_jsvalue_to_string_coerce` are matched by existing patterns, but `js_url_coerce_string` reads `url_coerce_string` after the `js_` prefix and matched none of them. That gap is why the checker did not flag #7453. On the pre-fix IR the widened matcher reports it, and reports it as MOVING: source (alloc): %r23 = call i64 @js_url_coerce_string(double %r22) stale use : %r26 = call i64 @js_url_new_with_base(i64 %r23, %r25) between : js_url_coerce_string, perry_fn_url_root_ts__mk MOVING : YES via perry_fn_url_root_ts__mk Measured, per the extrapolated-suffix warning in the same list: the symbol was verified to exist before the pattern was added, stale-register uses on that IR go 3 -> 4 (the new one is the bug), and both gated arms over the 144-module corpus stay at 0 violations with 40/40 seeded violations caught. * docs: changelog fragment for #7454 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 5, 2026
Merged
proggeramlug
pushed a commit
that referenced
this pull request
Aug 5, 2026
First production lowering on the layer 1 discipline, and it closed a window the hand-fix in #7453 left behind. FnCtx has no interior mutability -- ctx.block() needs &mut -- so the borrow-carrying Raw from #7459 cannot be built on it: root(self) would need a second borrow while the handle holds the first. The shape that works against a &mut-only emitter is the combinator, which is what the runtime settled on for layer 3 (RuntimeHandle::across_*): never hand out an unrooted handle at all. call_rooted emits the collecting call and roots its result in one step. Emitted window, #7453 vs now: before after coerce url coerce url root url root url mk() <- user call mk() coerce base <- UNROOTED coerce base read url root base <- closed new_with_base(url, base_raw) read url read base new_with_base(read, read) base_ptr was live and unrooted across js_gc_temp_root_get in the hand-written version. Small window, but a window -- and I did not see it when writing that fix by hand, which is the argument for the API. 11/11 URL gap tests byte-identical to node; repro clean under PERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1.
proggeramlug
added a commit
that referenced
this pull request
Aug 5, 2026
* feat(codegen): migrate UrlNew onto the Layer 1 rooting API First production lowering on the layer 1 discipline, and it closed a window the hand-fix in #7453 left behind. FnCtx has no interior mutability -- ctx.block() needs &mut -- so the borrow-carrying Raw from #7459 cannot be built on it: root(self) would need a second borrow while the handle holds the first. The shape that works against a &mut-only emitter is the combinator, which is what the runtime settled on for layer 3 (RuntimeHandle::across_*): never hand out an unrooted handle at all. call_rooted emits the collecting call and roots its result in one step. Emitted window, #7453 vs now: before after coerce url coerce url root url root url mk() <- user call mk() coerce base <- UNROOTED coerce base read url root base <- closed new_with_base(url, base_raw) read url read base new_with_base(read, read) base_ptr was live and unrooted across js_gc_temp_root_get in the hand-written version. Small window, but a window -- and I did not see it when writing that fix by hand, which is the argument for the API. 11/11 URL gap tests byte-identical to node; repro clean under PERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1. * docs: changelog fragment for 7461 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 5, 2026
…ing (#7462) * fix(codegen): root the URLSearchParams receiver across the name lowering Six arms held a raw heap pointer across arbitrary user code. Same shape as #7453, found by scanning url_main.rs for a raw pointer bound before a lower_expr and used after it, then confirmed in emitted IR: %r22 = and i64 %r21, 281474976710655 ; p_ptr, raw heap pointer %r23 = call double @perry_fn_sp_root_ts__mk() ; user call, can collect %r24 = call i64 @js_url_search_params_get(i64 %r22, double %r23) Moving the unbox below the lowering would not fix it: p_v is the same pointer with a NaN-box tag on it, and a tagged pointer in an SSA register is exactly as invisible to the collector. Both operands are now lowered through lower_exprs_rooted and the receiver is unboxed from the RELOADED value: %r22 = call i32 @js_gc_temp_root_push(i64 %r21) ; root before %r23 = call double @perry_fn_sp_root_ts__mk() %r24 = call i64 @js_gc_temp_root_get(i32 %r22) ; re-read after %r26 = and i64 %r24, 281474976710655 ; unbox the reload %r27 = call i64 @js_url_search_params_get(i64 %r26, double %r23) call void @js_gc_temp_root_truncate(i32 %r22) ; release after use Arms: Get, Has, Set, Append, Delete, GetAll. The guard is released after the consuming call, not before -- the consumer allocates while reading these values. An earlier draft bound it to _operand_guard, which compiled clean and never emitted the truncate; in a loop that grows the temp-root stack without bound. 12/12 URL + URLSearchParams gap tests byte-identical to node; repro clean under PERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1. * docs: changelog fragment for 7462 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Layer 1 of the engine plan (#7294) — "the remaining question is not build the mechanism but where is it still not used". This is one of those sites, found by scanning for sequential
lower_exprcalls with no rooted variant nearby and then confirming against emitted IR.The window
js_url_coerce_stringreturns a rawStringHeaderpointer, not a NaN-boxed value, so nothing else keeps it alive. Innew URL(input, base)two collection points stood between it and its use — loweringbaseruns arbitrary user code, and the second coercion allocates wheneverbaseis not already a string.Emitted IR for
new URL("/p" + i, mk()), before:after:
The ordering is the fix, not the presence of a root: pushing after the coercion would have rooted an already-stale pointer, which is the shape #7192 and #7184 both had.
UrlPatternNewheld its NaN-boxed input in a bare register across the same window and now lowers both operands throughlower_exprs_rooted.Evidence, and its limit stated plainly
This is latent and I did not produce a failing repro. The value is only wrong if a collection lands inside the window, and at that moment there is nothing for the collector to find — which is exactly why
gc-rooting-invariant.mdmakes the static IR the instrument for this class and why #7154's from-space scan only ever caught offenders whose targets had already died. The IR above is the evidence.The repository's
--stale-registerschecker did not flag this: its heap-source heuristic recognises string handles and slot loads, not a raw pointer returned by a coercion helper. That is a coverage gap in the checker rather than an absence of the bug, and it is why this was found by reading rather than by the gate.Verification
ok, including underPERRY_GC_HEAP_LIMIT=8 PERRY_GEN_GC_EVACUATE=1 PERRY_GC_FORCE_EVACUATE=1new URL/URLPatterngap tests byte-identical to the pinned Node oraclecargo check -p perry-codegenclean,cargo fmt --checkcleanStacked on the corpus repair in #7452 — without it the dominance gate cannot see any of this class, since its corpus had zero root stores.