Skip to content

Fix WriteBarrier owner and add cellLock for JSCommonJSExtensions::m_registeredFunctions #30524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cirospaciari wants to merge 4 commits into from
+108 −2
Closed

Fix WriteBarrier owner and add cellLock for JSCommonJSExtensions::m_registeredFunctions #30524

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51c492f
Fix WriteBarrier owner and add cellLock for JSCommonJSExtensions::m_r…
cirospaciari May 11, 2026
8b7c175
Add cellLock to JSCommonJSExtensions__setFunction
robobun May 14, 2026
e6c1837
Add concurrent-GC regression guard for Module._extensions
robobun May 14, 2026
7c5ca15
Reword module-extensions-concurrent-gc test header/name for accuracy
robobun May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/jsc/bindings/JSCommonJSExtensions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,23 +201,25 @@
return deleted;
}

extern "C" uint32_t JSCommonJSExtensions__appendFunction(Zig::GlobalObject* globalObject, JSC::JSValue value)
{
JSCommonJSExtensions* extensions = globalObject->lazyRequireExtensionsObject();
WTF::Locker locker { extensions->cellLock() };
extensions->m_registeredFunctions.append(JSC::WriteBarrier<Unknown>());
extensions->m_registeredFunctions.last().set(globalObject->vm(), extensions, value);
return extensions->m_registeredFunctions.size() - 1;

Check warning on line 210 in src/jsc/bindings/JSCommonJSExtensions.cpp

View check run for this annotation

Claude / Claude Code Review

Hardened functions are dead code; m_registeredFunctions is always empty 

Heads-up: `JSCommonJSExtensions__appendFunction` / `__setFunction` / `__swapRemove` are dead code — they're declared as `extern fn` in `src/jsc/NodeModuleModule.zig:82-85` but have no call sites; the Zig side actually stores custom `require.extensions` handlers via `CustomLoader.custom: jsc.Strong`, so `m_registeredFunctions` is always empty. The fixes here are correct GC hygiene but have no runtime effect, so this code path can't be contributing to the `SlotVisitor::drain` segfault — it'd be cl
Comment thread
robobun marked this conversation as resolved.
}

extern "C" void JSCommonJSExtensions__setFunction(Zig::GlobalObject* globalObject, uint32_t index, JSC::JSValue value)
{
JSCommonJSExtensions* extensions = globalObject->lazyRequireExtensionsObject();
extensions->m_registeredFunctions[index].set(globalObject->vm(), globalObject, value);
extensions->m_registeredFunctions[index].set(globalObject->vm(), extensions, value);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

extern "C" uint32_t JSCommonJSExtensions__swapRemove(Zig::GlobalObject* globalObject, uint32_t index)
{
JSCommonJSExtensions* extensions = globalObject->lazyRequireExtensionsObject();
WTF::Locker locker { extensions->cellLock() };
ASSERT(extensions->m_registeredFunctions.size() > 0);
if (extensions->m_registeredFunctions.size() == 1) {
extensions->m_registeredFunctions.clear();
Expand All @@ -226,7 +228,7 @@
ASSERT(index < extensions->m_registeredFunctions.size());
if (index < (extensions->m_registeredFunctions.size() - 1)) {
JSValue last = extensions->m_registeredFunctions.takeLast().get();
extensions->m_registeredFunctions[index].set(globalObject->vm(), globalObject, last);
extensions->m_registeredFunctions[index].set(globalObject->vm(), extensions, last);
return extensions->m_registeredFunctions.size();
} else {
extensions->m_registeredFunctions.removeLast();
Expand Down Expand Up @@ -303,6 +305,11 @@
ASSERT_GC_OBJECT_INHERITS(thisObject, info());
Base::visitChildren(thisObject, visitor);

// m_registeredFunctions is mutated by JSCommonJSExtensions__appendFunction
// and JSCommonJSExtensions__swapRemove on the mutator thread; take cellLock
// so a concurrent Vector reallocation does not free the backing buffer
// mid-scan when this runs on a parallel mark thread.
WTF::Locker locker { thisObject->cellLock() };
for (auto& func : thisObject->m_registeredFunctions) {
visitor.append(func);
}
Expand Down
Loading