fix(runtime): register the per-thread globalThis cache as a GC root#6103
Conversation
`js_get_global_this` caches this thread's `globalThis` in a thread-local raw-pointer cell (`THREAD_GLOBAL_THIS`) but, unlike its sibling `js_module_top_this`, never registers that cache slot as a GC root. The authoritative `GLOBAL_THIS_PTR` static IS scanned and relocated by `scan_object_cache_roots_mut`, so when a copying minor evacuates `globalThis` the static and the object's overflow side-table are rekeyed to the moved address — while the thread-local cache keeps pointing at the stale from-space copy. Subsequent `js_get_global_this()` calls then hand back that dead pointer. Reading an overflow-stored builtin such as `globalThis.Error` returns `undefined` (its overflow entry was rekeyed onto the moved object), which surfaces downstream as `Class extends value is not a constructor` in programs that construct many classes during startup and trigger a collection between two global reads. Register the cache slot as a mutable global root (exactly as `js_module_top_this` already does) so the collector rewrites the raw pointer to the forwarding address on every move. Raw-pointer global-root slots are handled by `mark_global_root_bits` / `rewrite_value_bits` and covered by `test_rewrite_mutable_root_slots_updates_shadow_and_global_roots`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn ChangesGC Root Registration Fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Related PRs: None specified. Suggested labels: bug, gc, runtime Suggested reviewers: None specified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Problem
js_get_global_thiscaches this thread'sglobalThisin a thread-local raw-pointer cell (THREAD_GLOBAL_THIS) but — unlike its siblingjs_module_top_this— never registers that cache slot as a GC root.The authoritative
GLOBAL_THIS_PTRstatic is scanned and relocated byscan_object_cache_roots_mut. So when a copying minor evacuatesglobalThis, that static and the object's overflow side-table are rekeyed to the moved address — but the thread-local cache still points at the stale from-space copy.Subsequent
js_get_global_this()calls hand back that dead pointer. Reading an overflow-stored builtin such asglobalThis.Errorthen returnsundefined, because the overflow entry was rekeyed onto the moved object. Downstream this surfaces asClass extends value is not a constructorin a program that constructs many classes during startup and happens to trigger a collection between two global reads (e.g.class X extends Error {}whereglobalThis.Errorreadsundefined).Fix
Register the
THREAD_GLOBAL_THIScache slot as a mutable global root (viajs_gc_register_global_root), exactly asjs_module_top_thisalready does for its own thread-local cache. The collector then rewrites the raw pointer to the forwarding address on every move.Raw-pointer global-root slots are already handled by
mark_global_root_bits/rewrite_value_bits(the same pathjs_module_top_thisrelies on), and their rewrite-on-move is covered bytest_rewrite_mutable_root_slots_updates_shadow_and_global_roots.Validation
globalThisreads: before the fix,globalThis.Errorreadundefinedand the app threwClass extends value is not a constructor; after the fix that throw is gone deterministically across repeated runs and the app proceeds further into its pipeline.test_rewrite_mutable_root_slots_updates_shadow_and_global_roots).Summary by CodeRabbit