From b2ca7fb648a667d80f79646ce33d7d902beab501 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 11 Sep 2026 17:59:17 +0800 Subject: [PATCH] mem: free v8's backing store pointer whenever we get a backingstore's shared pointer, we need to release it --- src/browser/js/Local.zig | 5 ++- src/browser/js/Value.zig | 5 ++- src/browser/js/js.zig | 70 ++++++++++++++++++++++++++++++++++------ 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/browser/js/Local.zig b/src/browser/js/Local.zig index 2fbc17c95..0dbc0ad11 100644 --- a/src/browser/js/Local.zig +++ b/src/browser/js/Local.zig @@ -913,13 +913,12 @@ fn jsValueToArrayBufferSlice(comptime T: type, any_view: bool, js_val: js.Value) byte_offset = 0; } - const backing_store_ptr = v8.v8__ArrayBuffer__GetBackingStore(array_buffer orelse return null); + const buffer = array_buffer orelse return null; if (byte_len == 0) { return &[_]T{}; } - const backing_store_handle = v8.std__shared_ptr__v8__BackingStore__get(&backing_store_ptr).?; - const data = v8.v8__BackingStore__Data(backing_store_handle); + const data = js.arrayBufferData(buffer); const base = @as([*]u8, @ptrCast(data)) + byte_offset; // 2. Validate alignment diff --git a/src/browser/js/Value.zig b/src/browser/js/Value.zig index 3e2f23676..f4738e100 100644 --- a/src/browser/js/Value.zig +++ b/src/browser/js/Value.zig @@ -256,13 +256,12 @@ pub fn toStringSmart(self: Value) ![]const u8 { return self.toStringSlice(); } - const backing_store_ptr = v8.v8__ArrayBuffer__GetBackingStore(array_buffer orelse return ""); + const buffer = array_buffer orelse return ""; if (byte_len == 0) { return &[_]u8{}; } - const backing_store_handle = v8.std__shared_ptr__v8__BackingStore__get(&backing_store_ptr) orelse return ""; - const data = v8.v8__BackingStore__Data(backing_store_handle) orelse return ""; + const data = js.arrayBufferData(buffer) orelse return ""; const base = @as([*]const u8, @ptrCast(data)) + byte_offset; return base[0..byte_len]; diff --git a/src/browser/js/js.zig b/src/browser/js/js.zig index 97163beae..66e6b819b 100644 --- a/src/browser/js/js.zig +++ b/src/browser/js/js.zig @@ -239,8 +239,7 @@ pub fn ArrayBufferRef(comptime kind: ArrayType) type { } else { const buffer_len = size * bits / 8; const backing_store = v8.v8__ArrayBuffer__NewBackingStore(isolate.handle, buffer_len).?; - const backing_store_ptr = v8.v8__BackingStore__TO_SHARED_PTR(backing_store); - array_buffer = v8.v8__ArrayBuffer__New2(isolate.handle, &backing_store_ptr).?; + array_buffer = newArrayBuffer(isolate, backing_store); } const handle: *const v8.Value = switch (comptime kind) { @@ -272,15 +271,26 @@ pub fn ArrayBufferRef(comptime kind: ArrayType) type { } const byte_offset = v8.v8__ArrayBufferView__ByteOffset(view); const array_buffer = v8.v8__ArrayBufferView__Buffer(view).?; - const backing_store_ptr = v8.v8__ArrayBuffer__GetBackingStore(array_buffer); - const backing_store = v8.std__shared_ptr__v8__BackingStore__get(&backing_store_ptr).?; - const data = v8.v8__BackingStore__Data(backing_store).?; + const data = arrayBufferData(array_buffer).?; const base = @as([*]u8, @ptrCast(data)) + byte_offset; return @as([*]BackingInt, @ptrCast(@alignCast(base)))[0 .. byte_len / @sizeOf(BackingInt)]; } }; } +fn newArrayBuffer(isolate: Isolate, backing_store: *v8.BackingStore) *const v8.ArrayBuffer { + var backing_store_ptr = v8.v8__BackingStore__TO_SHARED_PTR(backing_store); + defer v8.std__shared_ptr__v8__BackingStore__reset(&backing_store_ptr); + return v8.v8__ArrayBuffer__New2(isolate.handle, &backing_store_ptr).?; +} + +pub fn arrayBufferData(array_buffer: *const v8.ArrayBuffer) ?*anyopaque { + var backing_store_ptr = v8.v8__ArrayBuffer__GetBackingStore(array_buffer); + defer v8.std__shared_ptr__v8__BackingStore__reset(&backing_store_ptr); + const backing_store = v8.std__shared_ptr__v8__BackingStore__get(&backing_store_ptr) orelse return null; + return v8.v8__BackingStore__Data(backing_store); +} + // If a WebAPI takes a []const u8, then we'll coerce any JS value to that string // so null -> "null". But if a WebAPI takes an optional string, ?[]const u8, // how should we handle null? If the parameter _isn't_ passed, then it's obvious @@ -364,13 +374,12 @@ pub fn simpleZigValueToJs(isolate: Isolate, value: anytype, comptime fail: bool, ArrayBuffer => { const values = value.values; const len = values.len; - const backing_store = v8.v8__ArrayBuffer__NewBackingStore(isolate.handle, len); + const backing_store = v8.v8__ArrayBuffer__NewBackingStore(isolate.handle, len).?; if (len > 0) { const data: [*]u8 = @ptrCast(@alignCast(v8.v8__BackingStore__Data(backing_store))); @memcpy(data[0..len], @as([]const u8, @ptrCast(values))[0..len]); } - const backing_store_ptr = v8.v8__BackingStore__TO_SHARED_PTR(backing_store); - return @ptrCast(v8.v8__ArrayBuffer__New2(isolate.handle, &backing_store_ptr).?); + return @ptrCast(newArrayBuffer(isolate, backing_store)); }, // zig fmt: off TypedArray(u8), TypedArray(u16), TypedArray(u32), TypedArray(u64), @@ -395,8 +404,7 @@ pub fn simpleZigValueToJs(isolate: Isolate, value: anytype, comptime fail: bool, const backing_store = v8.v8__ArrayBuffer__NewBackingStore(isolate.handle, buffer_len).?; const data: [*]u8 = @ptrCast(@alignCast(v8.v8__BackingStore__Data(backing_store))); @memcpy(data[0..buffer_len], @as([]const u8, @ptrCast(values))[0..buffer_len]); - const backing_store_ptr = v8.v8__BackingStore__TO_SHARED_PTR(backing_store); - array_buffer = v8.v8__ArrayBuffer__New2(isolate.handle, &backing_store_ptr).?; + array_buffer = newArrayBuffer(isolate, backing_store); } switch (@typeInfo(value_type)) { @@ -486,6 +494,48 @@ test "TaggedAnyOpaque" { try std.testing.expectEqual(24, @sizeOf(TaggedOpaque)); } +test "js: ArrayBuffers crossing Zig don't keep a reference to their backing store" { + const frame = try testing.createFrame(); + defer testing.test_session.closeAllPages(); + + var ls: Local.Scope = undefined; + frame.js.localScope(&ls); + defer ls.deinit(); + const local = &ls.local; + + // Every buffer must end up owned by its ArrayBuffer alone, otherwise + // it's never freed. + const created = ArrayBufferRef(.uint8).init(local, 16); + try testing.expectEqual(1, backingStoreRefs(created.handle)); + _ = created.slice(); + try testing.expectEqual(1, backingStoreRefs(created.handle)); + + const typed = simpleZigValueToJs(local.isolate, TypedArray(u8){ .values = "abc" }, true, false); + try testing.expectEqual(1, backingStoreRefs(typed)); + + const buffer = simpleZigValueToJs(local.isolate, ArrayBuffer{ .values = "abc" }, true, false); + try testing.expectEqual(1, backingStoreRefs(buffer)); + + const from_js = try local.exec("new Uint8Array(8)", null); + _ = try from_js.toStringSmart(); + _ = try from_js.toZig(TypedArray(u8)); + try testing.expectEqual(1, backingStoreRefs(from_js.handle)); +} + +// References held on an ArrayBuffer's (or a view's) backing store, not +// counting the one taken here to ask. +fn backingStoreRefs(handle: *const v8.Value) i64 { + const array_buffer: *const v8.ArrayBuffer = if (v8.v8__Value__IsArrayBuffer(handle)) + @ptrCast(handle) + else + v8.v8__ArrayBufferView__Buffer(@ptrCast(handle)).?; + var backing_store_ptr = v8.v8__ArrayBuffer__GetBackingStore(array_buffer); + defer v8.std__shared_ptr__v8__BackingStore__reset(&backing_store_ptr); + return v8.std__shared_ptr__v8__BackingStore__use_count(&backing_store_ptr) - 1; +} + +const testing = @import("../../testing.zig"); + // Every finalizable instance of Zig gets 1 FinalizerCallback registered in the // Page. This is to ensure that, if v8 doesn't finalize the value, we can // release on Page teardown.