fix(gc): root the receiver across the .size arm's three allocating helpers - #7385
Merged
Conversation
…lpers js_object_get_field_by_name's Map/Set subclass .size arm calls three helpers that allocate, in sequence, with the receiver in a bare local across all of them: own_key_present (inside the if condition), class_instance_has_member (builds a String for its cache probe), and subclass_backing_of (calls js_string_from_bytes to rebuild its constant BACKING_KEY on every call). Any of the three can drive an evacuating minor, and on the None fall-through every later arm dereferences the receiver again. The faults are the GC-type probes at +560 and +664, reached after the plausibility checks pass because a retired from-space address still looks like a plausible heap pointer. The scope opens only after the key is confirmed to be "size". Opening it ahead of the key test would put a RuntimeHandleScope on every property read reaching this block, which is the one cost this arm must not have. This does NOT close test_gap_field_lane_semantics or test_gap_put_value_plan_cache: each fix moves the fault to the next site in the same function (+664 -> +560 -> +820), so at least a fourth remains. The three closed here are real -- a no-op leaves the offset byte-identical -- but the function is a chain. 58 pass / 2 fail on object/assign/class/field/shape, byte-identical to pristine main (both failures pre-existing).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ChangesMap and Set size receiver rooting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: ✨ 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 4, 2026
I wrote both fragments before opening their PRs and guessed the next number, so each was off by one: the mirror-remaining-sites work is #7383 (not 7382) and the size-arm rooting is #7385 (not 7384). 7384 is worse than cosmetic -- it is an open PR by another author, so the fragment claimed a number already in use and would have collided when that PR adds its own. Fragment names are PR-keyed precisely so in-flight PRs never collide; guessing the number defeats that. Open the PR first, then name the fragment. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 4, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* feat(gc): RuntimeHandle::across_* and a raw-handle debt ratchet The layer-3 half of the rooting-by-construction RFC. A RuntimeHandleScope gives an object liveness -- the collector marks it and rewrites the slot -- and does nothing for a raw pointer already read out of that slot. That copy is invisible to the collector, and if the object moves it names from-space. Every rooting bug fixed in the #7341 sweep had rooting ALREADY. What was missing each time was ordering the re-read against the collection point. across_* runs the allocating call and returns the post-collection address in one step, so the pre-call pointer is never bound. The .size arm of js_object_get_field_by_name is converted as the worked example. This is a debt counter, NOT a soundness proof, and the script says so. Rust has no effect system to mark "may allocate", so no signature can reject holding a stale copy across such a call, and a &mut Heap token cannot cross extern "C". The ratchet instead makes the unconverted count visible and monotonically decreasing: 1006 sites across 110 files, wired into test.yml beside the address-classification audit. Checked against negative controls, per the four-ways-a-gate-cannot-fail rule: sabotaging across_mut to return the pre-call pointer fails the test with the intended message; the ratchet fails on a rise and refuses to raise its own baseline; and --self-test asserts the matcher still fires on the shapes it counts and still ignores across_*, so a broken matcher cannot report zero and pass forever. Also drops a redundant republish in the .size arm that rustc flagged as assigned-never-read (introduced by #7385, superseded by the arm-level one). * docs: name the fragment for its real PR (#7389) --------- 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.
Three real stale-receiver sites in
js_object_get_field_by_name's Map/Set subclass.sizearm. It does not close its two tests — see below.The bug
The arm calls three helpers that allocate, in sequence, with the receiver held in a bare local across all of them:
own_key_presentifconditionclass_instance_has_memberStringfor its cache probesubclass_backing_ofjs_string_from_bytesrebuilds its constantBACKING_KEYon every callAny can drive an evacuating minor, and on the
Nonefall-through every later arm dereferences the receiver again. The faults are the GC-type probes at +560 and +664 — reached after the plausibility checks pass, because a retired from-space address still looks like a plausible heap pointer.Not the fast lane
The arm is gated on the key being exactly
"size", and the scope opens only after that test. Opening it ahead would put aRuntimeHandleScopeon every property read reaching this block — the one cost this arm must not have. I had it wrong in the first draft and caught it before this PR.This also retires the standing question of whether the property-read fast lane must root: it never arose. This is a
.sizespecial case, not the general path.What it does not do
test_gap_field_lane_semanticsandtest_gap_put_value_plan_cacheare still red. Each fix moves the fault to the next site in the same function — +664 → +560 → +820 — so at least a fourth remains.The three closed here are real: a no-op leaves the faulting offset byte-identical, and these moved twice. But the function is a chain, not a single defect, and I'd rather land three verified sites with that stated than imply a closed catch.
Regression
58 pass / 2 fail on the object/assign/class/field/shape gap set — byte-identical to pristine
main, both failures pre-existing (one already inknown_failures.json).Summary by CodeRabbit
.sizeproperty on Map and Set subclasses during memory management operations.