mirror of
https://github.com/oven-sh/bun
synced 2026-02-10 10:58:56 +00:00
codegen: Add WriteBarrierEarlyInit support for classes with values and valuesArray (#23624)
## Summary
Adds comprehensive support to `generate-classes.ts` for JavaScript
classes that need both named WriteBarrier members (like callbacks) and a
dynamic array of JSValues, all properly tracked by the garbage
collector. This replaces error-prone manual `protect()/unprotect()`
calls with proper GC integration.
## Motivation
The shell interpreter was using `JSValue.protect()/unprotect()` to keep
JavaScript objects alive, which caused memory leaks when cleanup paths
didn't properly unprotect values. This is a common pattern that needed a
better solution.
## What Changed
### Code Generator (`generate-classes.ts`)
When a class has both `values: ["resolve", "reject"]` and `valuesArray:
true`:
**Generated C++ class gets:**
- `WTF::FixedVector<JSC::WriteBarrier<JSC::Unknown>> jsvalueArray`
member for dynamic array
- Individual `JSC::WriteBarrier<JSC::Unknown> m_resolve, m_reject`
members for named values
- 4 `create()` overloads covering all combinations:
1. Basic: `create(vm, globalObject, structure, ptr)`
2. Array only: `create(..., FixedVector<WriteBarrier<Unknown>>&&)`
3. Named values: `create(..., JSValue resolve, JSValue reject)`
4. Both: `create(..., FixedVector&&, JSValue resolve, JSValue reject)`
**Constructor overloads using `WriteBarrierEarlyInit`:**
```cpp
JSShellInterpreter(VM& vm, Structure* structure, void* ptr,
JSValue resolve, JSValue reject)
: Base(vm, structure)
, m_resolve(resolve, JSC::WriteBarrierEarlyInit) // ← Key technique
, m_reject(reject, JSC::WriteBarrierEarlyInit)
{
m_ctx = ptr;
}
```
The `WriteBarrierEarlyInit` tag allows initializing WriteBarriers in the
constructor initializer list before the object is fully constructed,
which is required for proper GC integration.
**Extern C bridge functions:**
- `TypeName__createWithValues(globalObject, ptr, markedArgumentBuffer*)`
- `TypeName__createWithInitialValues(globalObject, ptr, resolve,
reject)`
- `TypeName__createWithValuesAndInitialValues(globalObject, ptr,
buffer*, resolve, reject)`
**Zig convenience wrappers:**
- `toJSWithValues(this, globalObject, markedArgumentBuffer)`
- `toJSWithInitialValues(this, globalObject, resolve, reject)`
- `toJSWithValuesAndInitialValues(this, globalObject, buffer, resolve,
reject)`
### Shell Interpreter Memory Leak Fix
**Before:**
```zig
const js_value = JSShellInterpreter.toJS(interpreter, globalThis);
resolve.protect(); // Manual reference counting
reject.protect();
// ... later in cleanup ...
resolve.unprotect(); // Easy to forget/miss in error paths
reject.unprotect();
```
**After:**
```zig
const js_value = Bun__createShellInterpreter(
globalThis,
interpreter,
parsed_shell_script,
resolve, // Stored with WriteBarrierEarlyInit
reject, // GC tracks automatically
);
// No manual memory management needed!
```
### Supporting Changes
- Added `MarkedArgumentBuffer.wrap()` helper in Zig for safe
MarkedArgumentBuffer usage
- Created `ShellBindings.cpp` with `Bun__createShellInterpreter()` using
the new API
- Removed all `protect()/unprotect()` calls from shell interpreter
- Applied pattern to both `ShellInterpreter` and `ShellArgs` classes
## Benefits
1. **No memory leaks**: GC tracks all references automatically
2. **Safer**: Cannot forget to unprotect values
3. **Cleaner code**: No manual reference counting
4. **Reusable**: Pattern works for any class needing to store JSValues
5. **Performance**: Same cost as manual protect/unprotect but safer
## Testing
Existing shell tests verify the functionality. The pattern is already
used throughout JavaScriptCore for similar cases (see
`JSWrappingFunction`, `AsyncContextFrame`, `JSModuleMock`, etc.)
## When to Use This Pattern
Use `values` + `valuesArray` + `WriteBarrierEarlyInit` when:
- Your C++ class needs to keep JavaScript values alive
- You have both known named callbacks AND dynamic arrays of values
- You want the GC to track references instead of manual
protect/unprotect
- Your class extends `JSDestructibleObject`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This commit is contained in:
@@ -41,9 +41,6 @@ pub fn finalize(
|
||||
|
||||
if (this.export_env) |*env| env.deinit();
|
||||
if (this.cwd) |*cwd| cwd.deref();
|
||||
for (this.jsobjs.items) |jsobj| {
|
||||
jsobj.unprotect();
|
||||
}
|
||||
if (this.args) |a| a.deinit();
|
||||
bun.destroy(this);
|
||||
}
|
||||
@@ -102,8 +99,12 @@ pub fn setEnv(this: *ParsedShellScript, globalThis: *JSGlobalObject, callframe:
|
||||
return .js_undefined;
|
||||
}
|
||||
|
||||
pub fn createParsedShellScript(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!JSValue {
|
||||
pub const createParsedShellScript = jsc.MarkedArgumentBuffer.wrap(createParsedShellScriptImpl);
|
||||
|
||||
fn createParsedShellScriptImpl(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame, marked_argument_buffer: *jsc.MarkedArgumentBuffer) bun.JSError!JSValue {
|
||||
var shargs = ShellArgs.init();
|
||||
var needs_to_free_shargs = true;
|
||||
defer if (needs_to_free_shargs) shargs.deinit();
|
||||
|
||||
const arguments_ = callframe.arguments_old(2);
|
||||
const arguments = arguments_.slice();
|
||||
@@ -124,7 +125,7 @@ pub fn createParsedShellScript(globalThis: *jsc.JSGlobalObject, callframe: *jsc.
|
||||
}
|
||||
var jsobjs = std.ArrayList(JSValue).init(shargs.arena_allocator());
|
||||
var script = std.ArrayList(u8).init(shargs.arena_allocator());
|
||||
try bun.shell.shellCmdFromJS(globalThis, string_args, &template_args, &jsobjs, &jsstrings, &script);
|
||||
try bun.shell.shellCmdFromJS(globalThis, string_args, &template_args, &jsobjs, &jsstrings, &script, marked_argument_buffer);
|
||||
|
||||
var parser: ?bun.shell.Parser = null;
|
||||
var lex_result: ?shell.LexResult = null;
|
||||
@@ -159,9 +160,12 @@ pub fn createParsedShellScript(globalThis: *jsc.JSGlobalObject, callframe: *jsc.
|
||||
.args = shargs,
|
||||
.jsobjs = jsobjs,
|
||||
});
|
||||
parsed_shell_script.this_jsvalue = jsc.Codegen.JSParsedShellScript.toJS(parsed_shell_script, globalThis);
|
||||
const this_jsvalue = jsc.Codegen.JSParsedShellScript.toJSWithValues(parsed_shell_script, globalThis, marked_argument_buffer);
|
||||
parsed_shell_script.this_jsvalue = this_jsvalue;
|
||||
|
||||
bun.analytics.Features.shell += 1;
|
||||
return parsed_shell_script.this_jsvalue;
|
||||
needs_to_free_shargs = false;
|
||||
return this_jsvalue;
|
||||
}
|
||||
|
||||
const std = @import("std");
|
||||
|
||||
Reference in New Issue
Block a user