fix(gc): refresh obj before the class-static mirror on both own-data-write arms - #7381
Conversation
…write arms js_object_set_field_by_name performs the property write -- an inline slot store, or overflow_set into the overflow map -- and then calls mirror_class_object_static_write. Both writes can reach an allocator (overflow_set inserts into a Rust map, and a minor GC triggers on the malloc-count threshold as well as on arena blocks), and the mirror's first instruction dereferences obj. A collection in that write leaves the hook reading from-space. The function already has refresh_roots_after_alloc!() and calls it after every other allocating step; these two sites were missed. Scoped to these two arms deliberately. The macro republishes value from its handle as well as obj, so arms that intentionally rebind value locally must not refresh -- applying it to all eight mirror sites is wrong for that reason. Found via test_gap_gc_assign_string_source_rooting under #7341 quarantine. The fault moves out of the mirror entirely and into a separate catch in js_string_index_get, which stays open. Baselines verified rather than assumed: the two gap tests that fail alongside this also fail on pristine main, and perry-runtime --lib fails 3/5/3 across three runs of pristine main (#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)
📝 WalkthroughWalkthroughThis PR fixes a bug in ChangesStatic-write root refresh
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
✨ 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 |
…7383) Completes #7381, which fixed two of eight and scoped itself there on the theory that refresh_roots_after_alloc!() -- which republishes obj, key, value and interned_key together -- could clobber an arm that rebinds value locally. That theory was wrong. None of the eight arms rebinds any of the four after its handle is taken, so republishing is a no-op except for the relocation it repairs. Measured rather than inspected: the full-coverage build scores 58 pass / 2 fail on the object/assign/class/field/shape gap set, byte-identical to pristine main, with both failures pre-existing. With all eight refreshed the fault leaves mirror_class_object_static_write entirely and surfaces the next catch in the chain at js_jsvalue_equals. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
…7401) * fix(lint): bring field_set_by_name.rs back under the 2000-line cap #7381 and #7383 took the file from 1990 to 2048 lines, so `lint` -- a REQUIRED check -- has been red on main, and every merge since has bypassed it. The overrun is comment volume, not code: eight refresh_roots_after_alloc! call sites each carried a multi-line rationale block. The rationale now sits once at the macro that implements it, which is where it belonged, and the call sites are bare. Worth recording how I got this wrong first: I checked the gate against origin/main, saw identical output, and called it pre-existing. That baseline already contained the two merges that caused it. The right comparison is a commit from before the change. * docs: name the fragment for its real PR (#7401) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Another of the catches left open after #7373–#7376/#7380.
The bug
js_object_set_field_by_name's two own-data-write arms perform the write — an inline slot store, oroverflow_setinto the overflow map — and then callmirror_class_object_static_write.Both writes can reach an allocator:
overflow_setinserts into a Rust map, and a minor GC triggers on the malloc-count threshold as well as on arena-block allocation. The mirror's first instruction dereferencesobj(ldr w8, [x0]at +24), so a collection inside that write leaves the hook reading from-space.The function already carries a
refresh_roots_after_alloc!()macro and calls it after every other allocating step. These two sites were simply missed.Why it's scoped to two sites and not eight
There are eight
mirror_class_object_static_write(obj, key, value)call sites. I patched all eight first — and the macro republishesvaluefrom its handle as well asobj, so arms that intentionally rebindvaluelocally get clobbered. Only the two own-data-write arms are load-bearing for the fault, and only they are safe to refresh.Verification
The fault moves out of
mirror_class_object_static_writeentirely, into a separate and unrelated catch injs_string_index_get→js_string_char_atwhich stays open for follow-up. A fault that moves is the signal that the patch is real; a fault that doesn't move by a byte means the value was already dead before you touched it.Baselines were checked rather than assumed:
test_gap_2159_defineproperty_class_prototypeandtest_gap_6301_event_target_subclassfail identically on pristinemain— pre-existing, one is already inknown_failures.jsonperry-runtime --libfails 3 / 5 / 3 tests across three runs of pristinemain(cargo-test: perry-runtime's suite fails a different number of tests on every run #7365 nondeterminism), so the failure count seen with this patch sits inside main's own noise bandSummary by CodeRabbit