Skip to content

fix(codegen): root the URLSearchParams receiver across the name lowering - #7462

Merged
proggeramlug merged 2 commits into
mainfrom
fix/layer1-searchparams-receiver-rooting
Aug 5, 2026
Merged

fix(codegen): root the URLSearchParams receiver across the name lowering#7462
proggeramlug merged 2 commits into
mainfrom
fix/layer1-searchparams-receiver-rooting

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Six more sites of #7453's shape, in the URLSearchParams family.

Confirmed in IR, not inferred

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

p_ptr is a raw heap pointer held across a full user function call.

Moving the unbox below the lowering would not fix itp_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 as an untagged one. That is worth stating because it is the obvious "fix" and it is wrong.

Both operands now go 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.

One thing I got wrong first

An earlier draft bound the guard to _operand_guard. That compiled clean with no warnings and never emitted js_gc_temp_root_truncate — so every execution of these arms would push two temp roots and never pop them, growing the stack without bound in a loop. The release has to be placed after the consuming call (the consumer allocates while reading these values), and "it compiles" was not evidence that it was.

Verification

  • 12/12 URL + URLSearchParams gap tests byte-identical to the pinned Node oracle
  • repro exits 0 under PERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1
  • before/after IR inspected directly at the same call site
  • cargo fmt --check clean

Remaining

The scan found 11 sites of this shape in url_main.rs; these are the six uniform params + name arms. The other five (UrlSetHref's value lowering, and three arms where the receiver is lowered second) have different shapes and are left for a follow-up rather than being swept in with a mechanical edit.

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of URLSearchParams operations, including getting, checking, setting, appending, deleting, and retrieving all values.
    • Prevented potential failures during memory-management activity while these operations are running.
    • Added validation covering normal execution and forced memory relocation scenarios.

Ralph Küpper added 2 commits August 5, 2026 17:54
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.
@proggeramlug
proggeramlug merged commit 43bcc49 into main Aug 5, 2026
1 check was pending
@proggeramlug
proggeramlug deleted the fix/layer1-searchparams-receiver-rooting branch August 5, 2026 15:54
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 20402c3b-9c9e-47d4-ab35-f43e533c8c3b

📥 Commits

Reviewing files that changed from the base of the PR and between 01d1249 and 7d7f8f3.

📒 Files selected for processing (2)
  • changelog.d/7462-searchparams-receiver-rooting.md
  • crates/perry-codegen/src/expr/url_main.rs

📝 Walkthrough

Walkthrough

This PR updates six URLSearchParams lowerings to root receiver and name operands together, reload rooted values before unboxing, and release the root guard after consuming runtime calls. It also adds a changelog entry for these rooting fixes.

Changes

URLSearchParams GC safety

Layer / File(s) Summary
Root URLSearchParams operands before runtime calls
crates/perry-codegen/src/expr/url_main.rs, changelog.d/7462-searchparams-receiver-rooting.md
UrlSearchParamsGet, Has, Set, Append, Delete, and GetAll now use rooted lowering for params and name, reload rooted values before unboxing or runtime calls, and release the temporary root guard after the consuming call where implemented. The changelog records the same receiver-rooting updates and validation notes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • PerryTS/perry#6972: Both PRs update codegen to use the lower_exprs_rooted and temp_root_release pattern for precise GC rooting.
  • PerryTS/perry#6975: Both PRs fix GC-soundness gaps by switching lowering sites to rooted and reloaded operand handling.
  • PerryTS/perry#6983: This PR extends the same precise-rooting fix pattern to URLSearchParams method lowerings.

Suggested reviewers: thehypnoo

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/layer1-searchparams-receiver-rooting

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

proggeramlug added a commit that referenced this pull request Aug 5, 2026
…both paths (#7463)

* fix(codegen): root the third URLSearchParams operand, and release on both paths

Corrects two defects in #7462.

INCOMPLETE. Has/Set/Append/Delete take a third operand, and #7462 rooted
only params+name -- so `value` still lowered AFTER the receiver was
unboxed, leaving p_ptr and n_v crossing exactly the window the change was
meant to close. All operands are now lowered together, `value` included
when present.

LEAKED. The automated release placement in #7462 landed inside Delete's
`else` branch only, so the with-value path pushed two temp roots per
execution and never truncated them -- unbounded growth in a loop. It
compiled with no warning, which is why the arms are now audited
programmatically for a top-level release rather than by reading.

All six arms verified: one release each, at arm top level, reachable on
every path.

12/12 URL + URLSearchParams gap tests byte-identical to node. A
three-operand repro (set/append/has/delete, each argument a fresh
allocating call) matches node and is clean under PERRY_GC_HEAP_LIMIT=8
PERRY_GC_FORCE_EVACUATE=1.

* docs: changelog fragment for 7463

---------

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant