Fix memory leak when printing any error's source code. (#12831)

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>

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<JSC::StackFrame>&
             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<JSC::Exception*>(value)) {
         if (JSC::ErrorInstance* error = JSC::jsDynamicCast<JSC::ErrorInstance*>(jscException->value())) {
-            fromErrorInstance(exception, arg1, error, &jscException->stack(), value);
+            fromErrorInstance(exception, global, error, &jscException->stack(), value);
             return;
         }
     }
 
     if (JSC::ErrorInstance* error = JSC::jsDynamicCast<JSC::ErrorInstance*>(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<ProxyObject*>(JSValue::decode(value))->internalField((ProxyObject::Field)id).get());
 }
+
+CPP_DECL void JSC__SourceProvider__deref(JSC::SourceProvider* provider)
+{
+    provider->deref();
+}

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);
             }

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 {

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();
 

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,

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);