fix(gc): root the callee across the instance allocation in new - #7391
Merged
Conversation
added 2 commits
August 4, 2026 20:50
js_new_function_construct allocates the instance, then decodes the
closure pointer out of `func_value` -- a value computed BEFORE that
allocation -- and reads CLOSURE_MAGIC off it via is_closure_ptr. The
allocation can drive an evacuating minor, so the decode names from-space.
The disassembly is unambiguous:
+5208: bl js_object_alloc_with_parent ; allocates
+5304: ldr w8, [x24, #0xc] ; faults
+5308: cmp w8, #0x434c ; "CL" -- CLOSURE_MAGIC
Without the quarantine this is QUIET rather than fatal: the magic check
simply fails, so the user-prototype link is skipped and the instance
silently gets the wrong [[Prototype]] -- `foo.prototype = new Array(1,2,3)`
not taking effect, with no crash and no diagnostic.
js_new_function_construct_with_new_target carries the identical shape with
`nt` in place of `func_value`, used by constructor_prototype_bits on the
line after the same allocation. Fixed too; noted in-code that it is not
independently reproduced.
test_gap_learned_inline_sizing is byte-identical to Node under from-space
quarantine (was SIGBUS). 47 class/proto/construct/new/reflect/inline gap
tests pass; the two that fail also fail on pristine main, and the unit
suite's 4 failures sit inside main's own 3/5/3 noise band (#7365).
|
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 construction paths now root constructor closure and ChangesConstructor GC rooting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
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 |
This was referenced Aug 4, 2026
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
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.
Closes another #7341 catch — and this one was silently returning wrong answers, not just crashing under quarantine.
The bug
js_new_function_constructallocates the instance, then decodes the closure out offunc_value— computed before that allocation — and readsCLOSURE_MAGICoff it viais_closure_ptr. An evacuating minor inside the allocation moves the closure, so the decode names from-space.The disassembly matches the source predicate exactly:
Why it matters beyond the crash
Without the quarantine this is quiet. The magic check simply fails, so the user-prototype link is skipped and the instance gets the wrong
[[Prototype]]—foo.prototype = new Array(1,2,3)not taking effect, with no crash and no diagnostic. That is the failure mode the quarantine exists to surface: evacuation copies rather than zeroes, so a stale address still holds plausible bytes.Sibling
js_new_function_construct_with_new_targethas the identical shape withntin place offunc_value, consumed byconstructor_prototype_bitson the line after the same allocation. Fixed as the same defect — and the code comment says plainly that it is not independently reproduced, since only the plain-newpath has a measured fault.A process note worth keeping
My first attempt placed the republish after the use — it landed inside the
if, past the point wherefpwas already derived from the stale value. Source looked right; the disassembly showed the deref at +5404 still preceding the handle read at +5420. Second attempt puts it immediately after the allocation. A fix that reads correctly and disassembles wrong is still broken.Verification
test_gap_learned_inline_sizing— byte-identical to Node under quarantine (was SIGBUS)main(one already inknown_failures.json)Summary by CodeRabbit
newandReflect.constructretain the correct prototype, including user-assigned prototypes.