Repository navigation
Fix WriteBarrier initialization to use WriteBarrierEarlyInit - #23435
Conversation
|
Updated 4:34 AM PT - Oct 10th, 2025
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 23435That installs a local version of the PR into your bun-23435 --bun |
WalkthroughMultiple binding classes had field initialization moved into constructors (using WriteBarrierEarlyInit), constructor signatures changed to accept new JSValue members, related create/finishCreation call sites were updated, new accessors/members added, and some initialization routines made defensive or simplified. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📥 CommitsReviewing files that changed from the base of the PR and between a8ada84df0b2639284680db0419ffaef3db4a8cb and be08df3. 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.{cpp,h}📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
**/*.cpp📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/bun.js/bindings/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (2)src/bun.js/bindings/NodeVMSourceTextModule.h (1)
src/bun.js/bindings/NodeVMSourceTextModule.cpp (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
Comment |
This fixes incorrect WriteBarrier initialization patterns where `.set()` or `.setEarlyValue()` was being called in the constructor body or in `finishCreation()`. According to JavaScriptCore's WriteBarrier.h, WriteBarriers that are initialized during construction should use the WriteBarrierEarlyInit constructor in the initializer list to avoid triggering unnecessary write barriers. Changes: - InternalModuleRegistry: Initialize internal fields in constructor using setWithoutWriteBarrier instead of calling .set() in finishCreation - AsyncContextFrame: Pass callback and context to constructor and use WriteBarrierEarlyInit - JSCommonJSModule: Pass id, filename, dirname to constructor and use WriteBarrierEarlyInit - JSMockImplementation: Pass initial values to constructor and use WriteBarrierEarlyInit - JSConnectionsList: Pass connection sets to constructor and use WriteBarrierEarlyInit - JSBunRequest: Pass params to constructor and initialize both m_params and m_cookies using WriteBarrierEarlyInit - JSNodeHTTPServerSocket: Initialize currentResponseObject using WriteBarrierEarlyInit instead of calling setEarlyValue() - JSModuleMock: Pass callback to constructor and use WriteBarrierEarlyInit - JSWrappingFunction: Pass wrappedFn to constructor and use WriteBarrierEarlyInit - NodeVMSourceTextModule: Pass initializeImportMeta to constructor and use WriteBarrierEarlyInit - NodeVMModule: Initialize m_context in initializer list using WriteBarrierEarlyInit Note: CallSite was identified but not fixed as it requires more complex refactoring due to runtime-dependent initialization logic. The generate-classes.ts code generator does not need updates because generated classes intentionally leave WriteBarrier fields uninitialized, to be populated later by Zig code via setter functions.
cd11942 to
175cc35
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between cd11942869ba5d3248604616c3828d3e95639c24 and 175cc35.
📒 Files selected for processing (17)
src/bun.js/bindings/AsyncContextFrame.cpp(2 hunks)src/bun.js/bindings/AsyncContextFrame.h(1 hunks)src/bun.js/bindings/BunPlugin.cpp(1 hunks)src/bun.js/bindings/InternalModuleRegistry.cpp(1 hunks)src/bun.js/bindings/JSBunRequest.cpp(2 hunks)src/bun.js/bindings/JSBunRequest.h(1 hunks)src/bun.js/bindings/JSCommonJSModule.cpp(2 hunks)src/bun.js/bindings/JSCommonJSModule.h(2 hunks)src/bun.js/bindings/JSMockFunction.cpp(2 hunks)src/bun.js/bindings/JSWrappingFunction.cpp(1 hunks)src/bun.js/bindings/JSWrappingFunction.h(1 hunks)src/bun.js/bindings/NodeVMModule.cpp(1 hunks)src/bun.js/bindings/NodeVMSourceTextModule.cpp(1 hunks)src/bun.js/bindings/NodeVMSourceTextModule.h(1 hunks)src/bun.js/bindings/node/JSNodeHTTPServerSocket.cpp(1 hunks)src/bun.js/bindings/node/http/JSConnectionsList.cpp(1 hunks)src/bun.js/bindings/node/http/JSConnectionsList.h(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/InternalModuleRegistry.cppsrc/bun.js/bindings/AsyncContextFrame.hsrc/bun.js/bindings/JSCommonJSModule.hsrc/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/JSBunRequest.hsrc/bun.js/bindings/AsyncContextFrame.cppsrc/bun.js/bindings/node/JSNodeHTTPServerSocket.cppsrc/bun.js/bindings/JSWrappingFunction.cppsrc/bun.js/bindings/JSCommonJSModule.cppsrc/bun.js/bindings/JSMockFunction.cppsrc/bun.js/bindings/NodeVMSourceTextModule.hsrc/bun.js/bindings/NodeVMSourceTextModule.cppsrc/bun.js/bindings/node/http/JSConnectionsList.cppsrc/bun.js/bindings/NodeVMModule.cppsrc/bun.js/bindings/node/http/JSConnectionsList.hsrc/bun.js/bindings/JSBunRequest.cppsrc/bun.js/bindings/JSWrappingFunction.h
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/InternalModuleRegistry.cppsrc/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/AsyncContextFrame.cppsrc/bun.js/bindings/node/JSNodeHTTPServerSocket.cppsrc/bun.js/bindings/JSWrappingFunction.cppsrc/bun.js/bindings/JSCommonJSModule.cppsrc/bun.js/bindings/JSMockFunction.cppsrc/bun.js/bindings/NodeVMSourceTextModule.cppsrc/bun.js/bindings/node/http/JSConnectionsList.cppsrc/bun.js/bindings/NodeVMModule.cppsrc/bun.js/bindings/JSBunRequest.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/InternalModuleRegistry.cppsrc/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/AsyncContextFrame.cppsrc/bun.js/bindings/node/JSNodeHTTPServerSocket.cppsrc/bun.js/bindings/JSWrappingFunction.cppsrc/bun.js/bindings/JSCommonJSModule.cppsrc/bun.js/bindings/JSMockFunction.cppsrc/bun.js/bindings/NodeVMSourceTextModule.cppsrc/bun.js/bindings/node/http/JSConnectionsList.cppsrc/bun.js/bindings/NodeVMModule.cppsrc/bun.js/bindings/JSBunRequest.cpp
🧠 Learnings (3)
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Applied to files:
src/bun.js/bindings/InternalModuleRegistry.cppsrc/bun.js/bindings/JSCommonJSModule.hsrc/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/JSBunRequest.h
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/JSBunRequest.hsrc/bun.js/bindings/AsyncContextFrame.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/BunPlugin.cpp
🧬 Code graph analysis (10)
src/bun.js/bindings/AsyncContextFrame.h (1)
src/bun.js/bindings/BunPlugin.cpp (3)
vm(409-409)vm(410-410)callback(429-429)
src/bun.js/bindings/JSCommonJSModule.h (1)
src/bun.js/bindings/JSCommonJSModule.cpp (12)
finishCreation(408-426)finishCreation(408-408)finishCreation(820-825)finishCreation(820-820)finishCreation(1328-1335)finishCreation(1328-1328)vm(761-769)vm(761-764)vm(771-779)vm(771-774)vm(797-815)vm(797-797)
src/bun.js/bindings/BunPlugin.cpp (2)
src/bun.js/bindings/JSBunRequest.cpp (10)
create(55-64)create(55-55)vm(182-187)vm(182-182)vm(189-194)vm(189-189)vm(211-216)vm(211-211)finishCreation(159-165)finishCreation(159-159)src/bun.js/bindings/NodeVMModule.cpp (10)
create(211-226)create(211-211)create(281-286)create(281-281)create(539-544)create(539-539)finishCreation(298-304)finishCreation(298-298)finishCreation(556-561)finishCreation(556-556)
src/bun.js/bindings/JSBunRequest.h (1)
src/bun.js/bindings/JSBunRequest.cpp (10)
vm(182-187)vm(182-182)vm(189-194)vm(189-189)vm(211-216)vm(211-211)params(81-87)params(81-81)finishCreation(159-165)finishCreation(159-159)
src/bun.js/bindings/JSCommonJSModule.cpp (1)
src/bun.js/bindings/JSCommonJSModule.h (1)
filename(130-130)
src/bun.js/bindings/JSMockFunction.cpp (3)
src/bun.js/bindings/BunPlugin.cpp (2)
vm(409-409)vm(410-410)src/bun.js/bindings/JSBunRequest.cpp (6)
vm(182-187)vm(182-182)vm(189-194)vm(189-189)vm(211-216)vm(211-211)src/bun.js/bindings/JSCommonJSModule.cpp (6)
vm(761-769)vm(761-764)vm(771-779)vm(771-774)vm(797-815)vm(797-797)
src/bun.js/bindings/NodeVMSourceTextModule.h (1)
src/bun.js/bindings/NodeVMSourceTextModule.cpp (2)
initializeImportMeta(414-439)initializeImportMeta(414-414)
src/bun.js/bindings/node/http/JSConnectionsList.cpp (2)
src/bun.js/bindings/JSBunRequest.cpp (8)
finishCreation(159-165)finishCreation(159-159)vm(182-187)vm(182-182)vm(189-194)vm(189-189)vm(211-216)vm(211-211)src/bun.js/bindings/JSCommonJSModule.cpp (12)
finishCreation(408-426)finishCreation(408-408)finishCreation(820-825)finishCreation(820-820)finishCreation(1328-1335)finishCreation(1328-1328)vm(761-769)vm(761-764)vm(771-779)vm(771-774)vm(797-815)vm(797-797)
src/bun.js/bindings/node/http/JSConnectionsList.h (3)
src/bun.js/bindings/JSBunRequest.cpp (8)
vm(182-187)vm(182-182)vm(189-194)vm(189-189)vm(211-216)vm(211-211)finishCreation(159-165)finishCreation(159-159)src/bun.js/bindings/JSMockFunction.cpp (6)
vm(123-126)vm(123-123)globalObject(154-160)globalObject(154-154)finishCreation(981-988)finishCreation(981-981)src/bun.js/bindings/node/http/JSConnectionsList.cpp (2)
finishCreation(15-19)finishCreation(15-15)
src/bun.js/bindings/JSBunRequest.cpp (1)
src/bun.js/bindings/JSBunRequest.h (1)
JSBunRequest(11-47)
🔇 Additional comments (33)
src/bun.js/bindings/JSMockFunction.cpp (3)
154-160: LGTM! Constructor initialization pattern correctly implemented.The refactoring correctly moves WriteBarrier field initialization to the constructor. The conditional logic for
isOnce(passingjsNumber(1)vsjsUndefined()) preserves the original behavior checked by theisOnce()method at lines 193-196.
198-204: Excellent adherence to JavaScriptCore best practices.The constructor correctly uses
WriteBarrierEarlyInitin the initializer list for both WriteBarrier fields. This avoids triggering unnecessary write barriers during object construction, which is the recommended pattern per JavaScriptCore guidelines.
206-209: Refactoring complete: post-construction initialization removed.The simplified
finishCreationmethod correctly reflects the completed refactoring. WriteBarrier field initialization has been successfully moved to the constructor, leaving only the requiredBase::finishCreation(vm)call.src/bun.js/bindings/InternalModuleRegistry.cpp (2)
124-132: Initialization pattern looks correct.Moving field initialization from
finishCreation()to the constructor usingsetWithoutWriteBarrier()correctly avoids unnecessary write barriers during object construction. This aligns with JavaScriptCore best practices.
151-155: LGTM: finishCreation correctly simplified.The removal of field initialization from
finishCreation()is appropriate, as these fields are now initialized in the constructor where write barriers are not needed.src/bun.js/bindings/NodeVMSourceTextModule.h (1)
60-62: LGTM! Correct use of WriteBarrierEarlyInit.The constructor signature and member initialization correctly follow the JSC pattern of using
WriteBarrierEarlyInitfor early initialization. This avoids unnecessary write barriers during object construction.src/bun.js/bindings/NodeVMSourceTextModule.cpp (1)
414-439: LGTM! Visitor and usage correctly handle the refactored member.The
initializeImportMeta()function correctly checks for null/undefined before usingm_initializeImportMeta, and the visitor properly tracks the field for garbage collection.Also applies to: 458-458
src/bun.js/bindings/JSWrappingFunction.h (2)
62-64: LGTM! WriteBarrierEarlyInit pattern correctly applied.The constructor now properly initializes
m_wrappedFnusingWriteBarrierEarlyInitin the initializer list, avoiding unnecessary write barriers during object construction.
62-64: LGTM! Correct use of WriteBarrierEarlyInit.The constructor correctly initializes
m_wrappedFnusingWriteBarrierEarlyInitin the initializer list, avoiding unnecessary write barriers during object construction. This aligns with JavaScriptCore best practices.src/bun.js/bindings/NodeVMModule.cpp (2)
155-155: LGTM! Conditional initialization correctly handled.The
m_contextmember is now initialized in the constructor's initializer list usingWriteBarrierEarlyInit, with proper conditional logic to handle non-object contexts.
155-155: LGTM! Conditional initialization is safe.The conditional initialization
context.isObject() ? asObject(context) : nullptrwithWriteBarrierEarlyInitis correct. WriteBarrier can safely store nullptr, and the logic properly validates the type before conversion.src/bun.js/bindings/JSWrappingFunction.cpp (2)
36-36: LGTM! Constructor call properly updated.The
wrappedFnparameter is now correctly passed to the constructor, enabling initialization viaWriteBarrierEarlyInitas declared in the header.
36-36: LGTM! Constructor call updated correctly.The constructor instantiation correctly passes
wrappedFnas the final parameter, matching the updated constructor signature in the header.src/bun.js/bindings/JSCommonJSModule.cpp (4)
820-824: LGTM! finishCreation properly simplified.The
finishCreationmethod now correctly focuses only on post-construction tasks (setting sourceCode and calling Base::finishCreation), with member initialization moved to the constructor.
847-848: LGTM! Constructor and finishCreation properly coordinated.The
createmethod correctly instantiatesJSCommonJSModulewithid,filename, anddirnameparameters, then calls the simplifiedfinishCreationwith onlyvmandsourceCode.
820-824: LGTM! finishCreation correctly simplified.The
finishCreationmethod has been appropriately simplified to only handlesourceCodeinitialization, withid,filename, anddirnamenow initialized in the constructor.
847-849: LGTM! Constructor and finishCreation calls updated correctly.The
createfunction correctly passesid,filename, anddirnameto the constructor and onlyvmandsourceCodetofinishCreation, matching the updated signatures.src/bun.js/bindings/AsyncContextFrame.h (2)
60-64: LGTM! WriteBarrierEarlyInit pattern correctly applied.The constructor properly initializes
callbackandcontextusingWriteBarrierEarlyInitin the initializer list, avoiding unnecessary write barriers during construction.
60-64: LGTM! Correct WriteBarrierEarlyInit usage.The constructor properly initializes both
callbackandcontextusingWriteBarrierEarlyInitin the initializer list, following JavaScriptCore best practices for early initialization.src/bun.js/bindings/AsyncContextFrame.cpp (4)
17-19: LGTM! Factory method properly updated.The
createmethod correctly passescallbackandcontextto the constructor, enabling initialization viaWriteBarrierEarlyInit.
27-29: LGTM! Factory method properly updated.The overloaded
createmethod correctly passescallbackandcontextto the constructor, enabling initialization viaWriteBarrierEarlyInit.
17-17: LGTM! Constructor instantiation updated correctly.The constructor call correctly passes
callbackandcontextparameters, matching the updated constructor signature.
27-27: LGTM! Second constructor instantiation also correct.This constructor instantiation also correctly passes the
callbackandcontextparameters.src/bun.js/bindings/JSCommonJSModule.h (3)
154-160: LGTM! WriteBarrierEarlyInit pattern correctly applied.The constructor now properly initializes
m_id,m_filename, andm_dirnameusingWriteBarrierEarlyInitin the initializer list, avoiding unnecessary write barriers during object construction.
86-86: LGTM! finishCreation signature correctly simplified.The
finishCreationsignature has been appropriately simplified, removing theid,filename, anddirnameparameters that are now handled in the constructor.
154-160: LGTM! Constructor correctly initializes members with WriteBarrierEarlyInit.The constructor properly initializes
m_id,m_filename, andm_dirnameusingWriteBarrierEarlyInitin the initializer list, following JavaScriptCore best practices for early field initialization.src/bun.js/bindings/JSBunRequest.h (2)
42-43: Use WriteBarrierEarlyInit for m_paramsIn JSBunRequest.cpp line 153, replace the initializer
- , m_params(params, JSC::WriteBarrierEarlyInit) + , m_params(JSC::WriteBarrierEarlyInit(params))⛔ Skipped due to learnings
Learnt from: CR PR: oven-sh/bun#0 File: .cursor/rules/javascriptcore-class.mdc:0-0 Timestamp: 2025-08-30T00:11:00.890Z Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
42-43: Constructor initializer list correctly initializes m_params and m_cookies using WriteBarrierEarlyInit.No further modifications required.
src/bun.js/bindings/BunPlugin.cpp (1)
432-432: Correct WriteBarrier initialization pattern.The constructor correctly initializes
callbackFunctionOrCachedResultusingWriteBarrierEarlyInitin the initializer list, avoiding unnecessary write barriers during object construction. This aligns perfectly with JavaScriptCore's recommended pattern.Also applies to: 449-453
src/bun.js/bindings/JSBunRequest.cpp (1)
61-62: Excellent refactor of WriteBarrier initialization.The changes correctly move WriteBarrier initialization from
finishCreationto the constructor:
- Both
m_paramsandm_cookiesare now initialized usingWriteBarrierEarlyInitin the constructor's initializer list (lines 153-154)- The
paramsparameter is passed through the constructor instead offinishCreationfinishCreationsignature is properly simplified to only acceptvm(line 159)This is a clean, complete refactor that aligns with JavaScriptCore best practices and the PR's objectives.
Also applies to: 151-165
src/bun.js/bindings/node/JSNodeHTTPServerSocket.cpp (1)
96-102: Approve WriteBarrierEarlyInit usage. Verify thatWriteBarrierEarlyInitis available via current includes—add#include <JavaScriptCore/WriteBarrier.h>if necessary to resolve any x64-musl or Windows build failures.
src/bun.js/bindings/node/http/JSConnectionsList.cpp (1)
15-19: Constructor now correctly covers barrier setup.With the write barriers initialized in the ctor, this lean
finishCreationis consistent and avoids redundantsetcalls.src/bun.js/bindings/node/http/JSConnectionsList.h (1)
46-49: Early write‑barrier initialization matches JSC recommendations.Passing both sets through the ctor and using
WriteBarrierEarlyInitensures the fields are ready before GC sees the object.
- Remove unused callback parameter from JSModuleMock::finishCreation - Fix comment in InternalModuleRegistry to match implementation - Format long line in NodeVMSourceTextModule for readability 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When initializeImportMeta is undefined, pass JSValue() instead to preserve the original behavior where the WriteBarrier would remain uninitialized rather than being set to jsUndefined(). Fixes crash in test-vm-timeout-escape-promise-module.js 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/NodeVMSourceTextModule.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/NodeVMSourceTextModule.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/NodeVMSourceTextModule.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/NodeVMSourceTextModule.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/NodeVMSourceTextModule.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (1)
src/bun.js/bindings/NodeVMSourceTextModule.cpp (1)
108-110: Verify JSValue empty vs undefined and reproduce VM module test crashes
- Constructor’s
m_initializeImportMeta(initializeImportMeta, WriteBarrierEarlyInit)usage is correct.- The ternary
initializeImportMeta.isUndefined() ? JSValue() : initializeImportMetaconvertsundefinedinto an emptyJSValue(), notjsUndefined(). Confirm downstream code tolerates this semantic difference.- Reproduce the segfaults in the
test-vm-module-*suite locally to ensure this change isn’t the root cause.
The initializeImportMeta field is optional and should remain uninitialized when not provided. WriteBarrierEarlyInit is not appropriate for optional fields - they should use the original pattern of conditional .set() after finishCreation(). This reverts the NodeVMSourceTextModule changes back to the original implementation where m_initializeImportMeta is only set if the value is not undefined. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
7f2ef8f to
a8ada84
Compare
When using WriteBarrierEarlyInit with optional JSValue fields, we must check if the value exists before calling methods like isObject(). Added context && check in NodeVMModule and initializeImportMeta && check in NodeVMSourceTextModule. Also improved initializeImportMeta to check if the callback is callable before proceeding, and gracefully handle cases where metaValue is not an object instead of asserting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
a8ada84 to
be08df3
Compare
Summary
This PR fixes incorrect WriteBarrier initialization patterns throughout the Bun codebase where
.set()or.setEarlyValue()was being called in the constructor body or infinishCreation(). According to JavaScriptCore'sWriteBarrier.h, WriteBarriers that are initialized during construction should use theWriteBarrierEarlyInitconstructor in the initializer list to avoid triggering unnecessary write barriers.Changes
The following classes were updated to properly initialize WriteBarrier fields:
setWithoutWriteBarrier()instead of calling.set()infinishCreation()WriteBarrierEarlyInitWriteBarrierEarlyInitWriteBarrierEarlyInitWriteBarrierEarlyInitm_paramsandm_cookiesusingWriteBarrierEarlyInitcurrentResponseObjectusingWriteBarrierEarlyInitinstead of callingsetEarlyValue()Why This Matters
From JavaScriptCore's
WriteBarrier.h:Using
WriteBarrierEarlyInitduring construction:Testing
Note on generate-classes.ts
The code generator (
generate-classes.ts) does not need updates because generated classes intentionally leave WriteBarrier fields (callbacks, cached fields, values) uninitialized. They start with default-constructed WriteBarriers and are populated later by Zig code via setter functions, which is the correct pattern for those fields.