diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index 01bf2de2c5..61f7f700e8 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -1,4 +1,3 @@ -#include "JSFFIFunction.h" #include "root.h" #include "JavaScriptCore/JSCast.h" #include "JavaScriptCore/JSType.h" @@ -8,6 +7,7 @@ #include "JavaScriptCore/JSPromiseConstructor.h" #include "JavaScriptCore/DeleteAllCodeEffort.h" #include "JavaScriptCore/BooleanObject.h" +#include "JSFFIFunction.h" #include "headers.h" #include "BunClientData.h" @@ -4053,7 +4053,7 @@ static void populateStackFrameMetadata(JSC::VM& vm, const JSC::StackFrame* stack static void populateStackFramePosition(const JSC::StackFrame* stackFrame, BunString* source_lines, OrdinalNumber* source_line_numbers, uint8_t source_lines_count, - ZigStackFramePosition* position) + ZigStackFramePosition* position, JSC::SourceProvider** referenced_source_provider) { auto code = stackFrame->codeBlock(); if (!code) @@ -4067,6 +4067,7 @@ static void populateStackFramePosition(const JSC::StackFrame* stackFrame, BunStr WTF::StringView sourceString = provider->source(); if (UNLIKELY(sourceString.isNull())) return; + if (!stackFrame->hasBytecodeIndex()) { if (stackFrame->hasLineAndColumnInfo()) { auto lineColumn = stackFrame->computeLineAndColumn(); @@ -4079,6 +4080,7 @@ static void populateStackFramePosition(const JSC::StackFrame* stackFrame, BunStr } auto location = Bun::getAdjustedPositionForBytecode(code, stackFrame->bytecodeIndex()); + *position = location; if (source_lines_count > 1 && source_lines != nullptr && sourceString.is8Bit()) { // Search for the beginning of the line @@ -4096,8 +4098,14 @@ static void populateStackFramePosition(const JSC::StackFrame* stackFrame, BunStr const unsigned char* bytes = sourceString.span8().data(); - // Most of the time, when you look at a stack trace, you want a couple lines above - source_lines[0] = Bun::toStringRef(sourceString.substring(lineStart, lineEnd - lineStart).toStringWithoutCopying()); + // Most of the time, when you look at a stack trace, you want a couple lines above. + + // It is key to not clone this data because source code strings are large. + // Usage of toStringView (non-owning) is safe as we ref the provider. + provider->ref(); + ASSERT(*referenced_source_provider == nullptr); + *referenced_source_provider = provider; + source_lines[0] = Bun::toStringView(sourceString.substring(lineStart, lineEnd - lineStart)); source_line_numbers[0] = location.line(); if (lineStart > 0) { @@ -4123,9 +4131,7 @@ static void populateStackFramePosition(const JSC::StackFrame* stackFrame, BunStr } // We are at the beginning of the line - source_lines[source_line_i] = Bun::toStringRef( - sourceString.substring(byte_offset_in_source_string, end_of_line_offset - byte_offset_in_source_string + 1) - .toStringWithoutCopying()); + source_lines[source_line_i] = Bun::toStringView(sourceString.substring(byte_offset_in_source_string, end_of_line_offset - byte_offset_in_source_string + 1)); source_line_numbers[source_line_i] = location.line().fromZeroBasedInt(location.line().zeroBasedInt() - source_line_i); source_line_i++; @@ -4136,17 +4142,15 @@ static void populateStackFramePosition(const JSC::StackFrame* stackFrame, BunStr } } } - - *position = location; } static void populateStackFrame(JSC::VM& vm, ZigStackTrace* trace, const JSC::StackFrame* stackFrame, - ZigStackFrame* frame, bool is_top) + ZigStackFrame* frame, bool is_top, JSC::SourceProvider** referenced_source_provider) { populateStackFrameMetadata(vm, stackFrame, frame); populateStackFramePosition(stackFrame, is_top ? trace->source_lines_ptr : nullptr, is_top ? trace->source_lines_numbers : nullptr, - is_top ? trace->source_lines_to_collect : 0, &frame->position); + is_top ? trace->source_lines_to_collect : 0, &frame->position, referenced_source_provider); } class V8StackTraceIterator { @@ -4332,7 +4336,7 @@ static void populateStackTrace(JSC::VM& vm, const WTF::Vector& break; ZigStackFrame* frame = &trace->frames_ptr[frame_i]; - populateStackFrame(vm, trace, &frames[stack_frame_i], frame, frame_i == 0); + populateStackFrame(vm, trace, &frames[stack_frame_i], frame, frame_i == 0, &trace->referenced_source_provider); stack_frame_i++; frame_i++; } @@ -4754,10 +4758,9 @@ JSC__JSValue JSC__JSValue__toError_(JSC__JSValue JSValue0) return JSC::JSValue::encode({}); } -void JSC__JSValue__toZigException(JSC__JSValue JSValue0, JSC__JSGlobalObject* arg1, - ZigException* exception) +void JSC__JSValue__toZigException(JSC__JSValue jsException, JSC__JSGlobalObject* global, ZigException* exception) { - JSC::JSValue value = JSC::JSValue::decode(JSValue0); + JSC::JSValue value = JSC::JSValue::decode(jsException); if (value == JSC::JSValue {}) { exception->code = JSErrorCodeError; exception->name = Bun::toStringRef("Error"_s); @@ -4767,17 +4770,17 @@ void JSC__JSValue__toZigException(JSC__JSValue JSValue0, JSC__JSGlobalObject* ar if (JSC::Exception* jscException = JSC::jsDynamicCast(value)) { if (JSC::ErrorInstance* error = JSC::jsDynamicCast(jscException->value())) { - fromErrorInstance(exception, arg1, error, &jscException->stack(), value); + fromErrorInstance(exception, global, error, &jscException->stack(), value); return; } } if (JSC::ErrorInstance* error = JSC::jsDynamicCast(value)) { - fromErrorInstance(exception, arg1, error, nullptr, value); + fromErrorInstance(exception, global, error, nullptr, value); return; } - exceptionFromString(exception, value, arg1); + exceptionFromString(exception, value, global); } void JSC__Exception__getStackTrace(JSC__Exception* arg0, ZigStackTrace* trace) @@ -5775,3 +5778,8 @@ CPP_DECL JSC__JSValue Bun__ProxyObject__getInternalField(JSC__JSValue value, uin { return JSValue::encode(jsCast(JSValue::decode(value))->internalField((ProxyObject::Field)id).get()); } + +CPP_DECL void JSC__SourceProvider__deref(JSC::SourceProvider* provider) +{ + provider->deref(); +} diff --git a/src/bun.js/bindings/exports.zig b/src/bun.js/bindings/exports.zig index 5fe49a0a91..3fd039729b 100644 --- a/src/bun.js/bindings/exports.zig +++ b/src/bun.js/bindings/exports.zig @@ -223,6 +223,13 @@ pub const ResolvedSource = extern struct { pub const Tag = @import("ResolvedSourceTag").ResolvedSourceTag; }; +pub const SourceProvider = opaque { + extern fn JSC__SourceProvider__deref(*SourceProvider) void; + pub fn deref(provider: *SourceProvider) void { + JSC__SourceProvider__deref(provider); + } +}; + const Mimalloc = @import("../../allocators/mimalloc.zig"); export fn ZigString__free(raw: [*]const u8, len: usize, allocator_: ?*anyopaque) void { @@ -426,6 +433,10 @@ pub const ZigStackTrace = extern struct { frames_ptr: [*]ZigStackFrame, frames_len: u8, + /// Non-null if `source_lines_*` points into data owned by a JSC::SourceProvider. + /// If so, then .deref must be called on it to release the memory. + referenced_source_provider: ?*JSC.SourceProvider = null, + pub fn toAPI( this: *const ZigStackTrace, allocator: std.mem.Allocator, @@ -786,6 +797,10 @@ pub const ZigException = extern struct { for (this.stack.frames_ptr[0..this.stack.frames_len]) |*frame| { frame.deinit(); } + + if (this.stack.referenced_source_provider) |source| { + source.deref(); + } } pub const shim = Shimmer("Zig", "Exception", @This()); @@ -828,7 +843,9 @@ pub const ZigException = extern struct { } pub fn deinit(this: *Holder, vm: *JSC.VirtualMachine) void { - this.zigException().deinit(); + if (this.loaded) { + this.zig_exception.deinit(); + } if (this.need_to_clear_parser_arena_on_deinit) { vm.module_loader.resetArena(vm); } diff --git a/src/bun.js/bindings/headers-handwritten.h b/src/bun.js/bindings/headers-handwritten.h index 330f21b178..ce5287eb5f 100644 --- a/src/bun.js/bindings/headers-handwritten.h +++ b/src/bun.js/bindings/headers-handwritten.h @@ -169,6 +169,7 @@ typedef struct ZigStackTrace { uint8_t source_lines_to_collect; ZigStackFrame* frames_ptr; uint8_t frames_len; + JSC::SourceProvider* referenced_source_provider; } ZigStackTrace; typedef struct ZigException { diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index 814cd4e804..2b6ab6c36d 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -3246,6 +3246,9 @@ pub const VirtualMachine = struct { var exception = exception_holder.zigException(); defer exception_holder.deinit(this); + // The ZigException structure stores substrings of the source code, in + // which we need the lifetime of this data to outlive the inner call to + // remapZigException, but still get freed. var source_code_slice: ?ZigString.Slice = null; defer if (source_code_slice) |slice| slice.deinit(); diff --git a/src/string.zig b/src/string.zig index 8e65e83ead..31df35dcdd 100644 --- a/src/string.zig +++ b/src/string.zig @@ -277,7 +277,7 @@ pub const Tag = enum(u8) { /// into a WTF::String. /// Can be in either `utf8` or `utf16le` encodings. ZigString = 2, - /// Static memory that is guarenteed to never be freed. When converted to WTF::String, + /// Static memory that is guaranteed to never be freed. When converted to WTF::String, /// the memory is not cloned, but instead referenced with WTF::ExternalStringImpl. /// Can be in either `utf8` or `utf16le` encodings. StaticZigString = 3, diff --git a/test/js/bun/util/inspect-error-leak.test.js b/test/js/bun/util/inspect-error-leak.test.js new file mode 100644 index 0000000000..49df1a3315 --- /dev/null +++ b/test/js/bun/util/inspect-error-leak.test.js @@ -0,0 +1,23 @@ +import { test, expect } from "bun:test"; + +const perBatch = 2000; +const repeat = 50; +test("Printing errors does not leak", () => { + function batch() { + for (let i = 0; i < perBatch; i++) { + Bun.inspect(new Error("leak")); + } + Bun.gc(true); + } + + batch(); + const baseline = Math.floor(process.memoryUsage.rss() / 1024); + for (let i = 0; i < repeat; i++) { + batch(); + } + + const after = Math.floor(process.memoryUsage.rss() / 1024); + const diff = ((after - baseline) / 1024) | 0; + console.log(`RSS increased by ${diff} MB`); + expect(diff, `RSS grew by ${diff} MB after ${perBatch * repeat} iterations`).toBeLessThan(10); +}, 10_000);