fix(runtime): root every saved implicit-this across the user code it brackets — 121 sites, not ~20 (#9445) - #9541
Closed
proggeramlug wants to merge 4 commits into
Closed
Conversation
|
Warning Review limit reachedNext included review available in 6 seconds. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (68)
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 |
added 4 commits
September 2, 2026 15:32
…t brackets (PerryTS#9445) Every `let prev = js_implicit_this_set(receiver); …user code…; js_implicit_this_set(prev)` in the runtime held the caller's receiver in a bare Rust local across a call that allocates. An evacuating young-gen minor inside the window moves that object; the restore then installed a retired from-space address as the caller's `this`, which reads as `undefined` on the next member access (the PerryTS#9417 shape). Root the saved value in a RuntimeHandleScope and re-read it at the restore — the idiom PR PerryTS#9444 used for the accessor sites — at all 121 remaining sites, plus the receivers that three of those sites consume again after the call. Claude-Session: https://claude.ai/code/session_01L11XMMWrR9Wz11dHpq4gXS
…el toJSON/replacer calls (PerryTS#9445) Found by the PerryTS#9445 fixture: with the saved implicit-`this` rooted, an allocating replacer still SIGSEGV'd in js_closure_call2. js_json_stringify_full and js_json_stringify_with_replacer run the root toJSON and the root replacer call — both user code — and then handed the walk the raw closure pointer and the "" key. Root both and re-read at each use.
…se existing handle scopes (PerryTS#9445) Callback loops (Map/Set/URLSearchParams forEach, EventTarget dispatch, the emitters, fs.watch fan-out, the timer batch, TypedArray.from's map callback) root the caller's receiver once before the loop and restore from that handle each iteration, instead of opening a scope per callback. Single-call sites that already own a RuntimeHandleScope push onto it. Three bare-name callers of js_implicit_this_get are fully qualified.
…re across a moving minor, plus changelog
proggeramlug
force-pushed
the
fix/9445-implicit-this-restore-sweep
branch
from
September 2, 2026 13:33
a5796f3 to
125df98
Compare
proggeramlug
added
the
run-extended-tests
proggeramlug
pushed a commit
that referenced
this pull request
Sep 2, 2026
…s replacer closure reads via with_const_ptr
Contributor
Author
8 tasks
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.
Fixes #9445.
What
Every
let prev = js_implicit_this_set(receiver); … user code …; js_implicit_this_set(prev)in the runtime kept the caller's receiver in a bare Rust local across a call that allocates. An evacuating young-gen minor inside the window moves that object; the restore then installed a retired from-space address as the caller'sthis, which reads asundefinedon the caller's next member access (the #9417 shape PR #9444 fixed for the four accessor sites).All remaining sites now root the saved value in a
RuntimeHandleScopeand re-read it at the restore — the idiomprototype_chain.rsand #9444 already use:Inventory: 122 sites, not ~20
The issue lists 24 sites in 12 files. A classifier over every
let X = js_implicit_this_set(…)incrates/perry-runtime/src(test modules excluded; script in the description below) finds 130 saves: 9 already rooted, one routed through the interpreter's own root stack (dyn_eval/bridge.rs), one a#[test]with a numeric sentinel, and 119 restoring a bare local — plus two the regex/filter missed (array/iterator.rs:484, anOption<f64>save;node_submodules/test.rs:808, in a file named like a test module). 121 converted, in 65 files — plus one more that landed with #9518 (os_process_streams.rs's EOF-flush loop) while this sweep was in flight, caught by re-running the classifier after the rebase: 122. The issue's 24 are a subset. This PR converts all 122 rather than the enumerated subset, because the hazard is identical at each and a partial sweep leaves the same silent-wrong-answer bug on paths a later report would misattribute in exactly the way #9417 was.Which sites could have been left alone: none. The issue asks for the per-site check. Every one of the 122 calls user-reachable code in the window — a closure (
js_closure_call*,js_native_call_value), a class getter/setter or static method through a vtable or registry, a Proxy trap, athen, a[Symbol.iterator]/next, atoJSON/replacer/reviver, a stream hook — or a[[Get]]that can reach a getter (proxy/reflect.rs:72). There is no site whose intervening call provably cannot allocate.Per-site mechanics:
forEach,EventTargetdispatch, cluster/child-process/pty emitters,fs.watchfan-out, the timer batch,to_string.rs's toString/valueOf probe,TypedArray.from's map callback): one handle holdsjs_implicit_this_get()before the loop and every iteration restores from it. Sites that already own aRuntimeHandleScope(timers, JSON, ToPrimitive,super[key](), promisify, watchers, …) push onto it instead of opening another.intl_subclass_superandtemporal_subclass_super(this_boxafter the parent constructor), and the threeprocess.stdinlistener loops (thiswas read once before the loop and reused across listeners; it is now re-read from the rooted singleton per listener).array/iterator.rs:484(anOption<f64>save) andnode_submodules/test.rs:808(thenode:testmock-call path, missed by the test-file filter) were converted by hand; the other 119 by the script.js_implicit_this_set's doc comment now states the contract so the next site cannot get it wrong.Verification
test-files/test_gap_9445_implicit_this_restore_sweep.ts— 34 cases, one per synchronously reachable site family. Each is a factory returning a fresh young object whoserunis afunction-expression method (readsthisoff the implicit-thiscell, not a captured slot);rundrives one runtime site with a callback that allocates past the nursery, then readsthis.inner.def. No GC env knobs. Node printsbad=0for every line.On unfixed
main(0b24670), the cases below print a non-zero count; each one is a runtime site the fixture reaches with nothing between it andrun's nextthisread:mainmap_forEachCannot read properties of undefined (reading 'def')set_forEachCannot read properties of undefined (reading 'def')urlsearchparams_forEachCannot read properties of undefined (reading 'def')event_target_dispatchCannot read properties of undefined (reading 'def')to_property_key_toStringCannot read properties of undefined (reading 'def')defineProperty_setterCannot read properties of undefined (reading 'def')function_object_getterCannot read properties of undefined (reading 'def')valueOf_to_primitiveCannot read properties of undefined (reading 'def')toString_templateCannot read properties of undefined (reading 'def')symbol_toPrimitiveCannot read properties of undefined (reading 'def')json_stringify_getterCannot read properties of undefined (reading 'def')json_stringify_toJSONCannot read properties of undefined (reading 'def')json_stringify_replacerjson_parse_reviverCannot read properties of undefined (reading 'def')for_of_user_iteratorspread_user_iteratorCannot read properties of undefined (reading 'def')destructure_user_iteratorCannot read properties of undefined (reading 'def')array_from_user_iteratorCannot read properties of undefined (reading 'def')proxy_get_trapCannot read properties of undefined (reading 'def')proxy_set_trapCannot read properties of undefined (reading 'def')proxy_apply_trapCannot read properties of undefined (reading 'def')proxy_construct_trapCannot read properties of undefined (reading 'def')reflect_applyCannot read properties of undefined (reading 'def')reflect_get_receiver_getterCannot read properties of undefined (reading 'def')bound_function_callstring_replace_callbackCannot read properties of undefined (reading 'def')using_disposewritable_writeCannot read properties of undefined (reading 'def')writable_writevCannot read properties of undefined (reading 'def')transform_transformCannot read properties of undefined (reading 'def')writable_constructCannot read properties of undefined (reading 'def')readable_readCannot read properties of undefined (reading 'def')writable_finalCannot read properties of undefined (reading 'def')transform_flushCannot read properties of undefined (reading 'def')The cases that print 0 on
mainare reached through a dispatch that carries its own (already rooted) save/restore —js_native_call_methodre-installs the correct receiver after the inner stale restore, masking it fromrun. They pin parity and the shape but are not witnesses for their site; those sites carry the mechanical fix only.Event-loop-driven sites (timers,
process.stdin, dgram, cluster,fs.watch, pty, child process, messaging) only ever see a heapprevfrom a nested pump (#1813's shape) and have no deterministic synchronous reproduction; they carry the same mechanical fix.Fixed build: every line
bad=0, byte-identical tonode --experimental-strip-types, exit 0, 2.9 s. Two candidate cases were dropped because they diverge for reasons outside this PR: a typed-arraydefinePropertyaccessor and aDatetoISOStringoverride (parity gaps, #9529), and autil.callbackifythenable whose exit-time microtask drain SIGSEGVs on every build includingmain(#9539, promise-side rooting; it was never a witness here —bad=0onmaintoo).Also fixed, same family, found by the fixture
JSON.stringify(value, replacerFn)still SIGSEGV'd with theprevrooting alone.js_json_stringify_full(and the compact entryjs_json_stringify_with_replacer) run the roottoJSONand the root replacer call — both user code — and then hand the raw replacer closure pointer and the""key to the walk.PERRY_GC_PROTECT_FROMSPACE=1+ aPERRY_KEEP_SYMBOLS=1build faulted injs_closure_call2←call_replacer←stringify_object_with_replacer_pretty: the walk (which roots everything it derives, correctly) was handed a retired closure. Both are now rooted across the root-level calls and re-read at each use (json/replacer.rs, separate commit).Cost
Measured on perrymaster (16-core Linux, other sessions' builds running — treat as ±3%), six alternating rounds each, sorted:
mainmsfor…of, 2M stepsMap.forEach, 2M callbacksJSON.stringifywithtoJSON, 500kvalueOfToPrimitive, 2MThat is ≈ +4 ns per user-iterator step and ≈ +1 ns per
forEachcallback (after the per-loop hoist; the first cut without it was +2.8 ns/callback). The remaining cost at single-call hot sites (js_iterator_next_result,toJSON) is oneRuntimeHandleScopenew/push/get/drop. A dedicated rooted implicit-thissave stack would be cheaper still, but it is new GC machinery (root scanner + trap savepoints) and the issue asked for the existing idiom; flagging it rather than building it.Classifier
(full script in the session.)