Fix WriteBarrier owner and add cellLock for JSCommonJSExtensions::m_registeredFunctions - #30524
Closed
cirospaciari wants to merge 4 commits into
Closed
Fix WriteBarrier owner and add cellLock for JSCommonJSExtensions::m_registeredFunctions#30524cirospaciari wants to merge 4 commits into
cirospaciari wants to merge 4 commits into
Claude / Claude Code Review
completed
May 14, 2026 in 14m 13s
Code review found 1 potential issue
Found 3 candidates, confirmed 1. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/node/module/module-extensions-concurrent-gc.test.ts:1-18 |
Test header/name misleading: does not exercise m_registeredFunctions |
Annotations
Check warning on line 18 in test/js/node/module/module-extensions-concurrent-gc.test.ts
claude / Claude Code Review
Test header/name misleading: does not exercise m_registeredFunctions
The test header comment and name claim it guards `m_registeredFunctions` mutation under concurrent GC, but `Module._extensions` assignment/define/delete route through `onAssign` → `NodeModuleModule__onRequireExtensionModify` → `jsc.Strong`, never calling `__appendFunction`/`__setFunction`/`__swapRemove` (which have no Zig callers), so `m_registeredFunctions` stays empty and `visitChildrenImpl`'s loop iterates zero times. The test would pass identically against pre-PR code and cannot detect a reg
Loading