From dc620ea8377cfc458547ba9ca867ae6e4ee3ef1a Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 31 Jul 2024 22:30:01 -0700 Subject: [PATCH] Fix a small memory leak when requiring CommonJS modules (#12984) --- src/bun.js/bindings/ModuleLoader.cpp | 6 +- src/bun.js/bindings/exports.zig | 6 +- src/bun.js/bindings/headers-handwritten.h | 3 +- src/bun.js/module_loader.zig | 68 +++-------------------- src/bundler.zig | 12 ++++ src/js_ast.zig | 2 +- src/js_parser.zig | 7 +-- src/js_printer.zig | 5 ++ 8 files changed, 34 insertions(+), 75 deletions(-) diff --git a/src/bun.js/bindings/ModuleLoader.cpp b/src/bun.js/bindings/ModuleLoader.cpp index 63655b948e2452..beb3e320ee5ee3 100644 --- a/src/bun.js/bindings/ModuleLoader.cpp +++ b/src/bun.js/bindings/ModuleLoader.cpp @@ -430,7 +430,7 @@ extern "C" void Bun__onFulfillAsyncModule( } } - if (res->result.value.commonJSExportsLen) { + if (res->result.value.isCommonJSModule) { auto created = Bun::createCommonJSModule(jsCast(globalObject), specifierValue, res->result.value); if (created.has_value()) { JSSourceCode* code = JSSourceCode::create(vm, WTFMove(created.value())); @@ -602,7 +602,7 @@ JSValue fetchCommonJSModule( Bun__transpileFile(bunVM, globalObject, specifier, referrer, typeAttribute, res, false); getSourceCodeStringForDeref(); - if (res->success && res->result.value.commonJSExportsLen) { + if (res->success && res->result.value.isCommonJSModule) { target->evaluate(globalObject, specifier->toWTFString(BunString::ZeroCopy), res->result.value); RETURN_IF_EXCEPTION(scope, {}); RELEASE_AND_RETURN(scope, target); @@ -774,7 +774,7 @@ static JSValue fetchESMSourceCode( getSourceCodeStringForDeref(); } - if (res->success && res->result.value.commonJSExportsLen) { + if (res->success && res->result.value.isCommonJSModule) { auto created = Bun::createCommonJSModule(globalObject, specifierJS, res->result.value); if (created.has_value()) { diff --git a/src/bun.js/bindings/exports.zig b/src/bun.js/bindings/exports.zig index 3fd039729beabe..75e74262d18d3d 100644 --- a/src/bun.js/bindings/exports.zig +++ b/src/bun.js/bindings/exports.zig @@ -202,11 +202,7 @@ pub const ResolvedSource = extern struct { /// source_url is eventually deref'd on success source_url: bun.String = bun.String.empty, - // this pointer is unused and shouldn't exist - commonjs_exports: ?[*]ZigString = null, - - // This field is used to indicate whether it's a CommonJS module or ESM - commonjs_exports_len: u32 = 0, + is_commonjs_module: bool = false, hash: u32 = 0, diff --git a/src/bun.js/bindings/headers-handwritten.h b/src/bun.js/bindings/headers-handwritten.h index 73a461e20858a8..615780ea6b781e 100644 --- a/src/bun.js/bindings/headers-handwritten.h +++ b/src/bun.js/bindings/headers-handwritten.h @@ -94,8 +94,7 @@ typedef struct ResolvedSource { BunString specifier; BunString source_code; BunString source_url; - ZigString* commonJSExports; - uint32_t commonJSExportsLen; + bool isCommonJSModule; uint32_t hash; void* allocator; JSC::EncodedJSValue jsvalue_for_export; diff --git a/src/bun.js/module_loader.zig b/src/bun.js/module_loader.zig index cb0fd1d4ca293a..b7970046a7f63a 100644 --- a/src/bun.js/module_loader.zig +++ b/src/bun.js/module_loader.zig @@ -393,7 +393,7 @@ pub const RuntimeTranspilerStore = struct { }; resolved_source.tag = brk: { - if (resolved_source.commonjs_exports_len > 0) { + if (resolved_source.is_commonjs_module) { const actual_package_json: *PackageJSON = brk2: { // this should already be cached virtually always so it's fine to do this const dir_info = (vm.bundler.resolver.readDirInfo(this.path.name.dir) catch null) orelse @@ -617,7 +617,7 @@ pub const RuntimeTranspilerStore = struct { .specifier = duped, .source_url = duped.createIfDifferent(path.text), .hash = 0, - .commonjs_exports_len = if (entry.metadata.module_type == .cjs) std.math.maxInt(u32) else 0, + .is_commonjs_module = entry.metadata.module_type == .cjs, }; return; @@ -731,11 +731,7 @@ pub const RuntimeTranspilerStore = struct { .source_code = source_code, .specifier = duped, .source_url = duped.createIfDifferent(path.text), - .commonjs_exports = null, - .commonjs_exports_len = if (parse_result.ast.exports_kind == .cjs) - std.math.maxInt(u32) - else - 0, + .is_commonjs_module = parse_result.ast.has_commonjs_export_names or parse_result.ast.exports_kind == .cjs, .hash = 0, }; } @@ -1471,11 +1467,6 @@ pub const ModuleLoader = struct { dumpSource(jsc_vm, specifier, &printer); } - const commonjs_exports = try bun.default_allocator.alloc(ZigString, parse_result.ast.commonjs_export_names.len); - for (parse_result.ast.commonjs_export_names, commonjs_exports) |name, *out| { - out.* = ZigString.fromUTF8(name); - } - if (jsc_vm.isWatcherEnabled()) { var resolved_source = jsc_vm.refCountedResolvedSource(printer.ctx.written, bun.String.init(specifier), path.text, null, false); @@ -1493,16 +1484,7 @@ pub const ModuleLoader = struct { } } - resolved_source.commonjs_exports = if (commonjs_exports.len > 0) - commonjs_exports.ptr - else - null; - resolved_source.commonjs_exports_len = if (commonjs_exports.len > 0) - @as(u32, @truncate(commonjs_exports.len)) - else if (parse_result.ast.exports_kind == .cjs) - std.math.maxInt(u32) - else - 0; + resolved_source.is_commonjs_module = parse_result.ast.has_commonjs_export_names or parse_result.ast.exports_kind == .cjs; return resolved_source; } @@ -1512,16 +1494,7 @@ pub const ModuleLoader = struct { .source_code = bun.String.createLatin1(printer.ctx.getWritten()), .specifier = String.init(specifier), .source_url = String.init(path.text), - .commonjs_exports = if (commonjs_exports.len > 0) - commonjs_exports.ptr - else - null, - .commonjs_exports_len = if (commonjs_exports.len > 0) - @as(u32, @truncate(commonjs_exports.len)) - else if (parse_result.ast.exports_kind == .cjs) - std.math.maxInt(u32) - else - 0, + .is_commonjs_module = parse_result.ast.has_commonjs_export_names or parse_result.ast.exports_kind == .cjs, .hash = 0, }; @@ -1861,7 +1834,7 @@ pub const ModuleLoader = struct { .specifier = input_specifier, .source_url = input_specifier.createIfDifferent(path.text), .hash = 0, - .commonjs_exports_len = if (entry.metadata.module_type == .cjs) std.math.maxInt(u32) else 0, + .is_commonjs_module = entry.metadata.module_type == .cjs, .tag = brk: { if (entry.metadata.module_type == .cjs and parse_result.source.path.isFile()) { const actual_package_json: *PackageJSON = package_json orelse brk2: { @@ -1949,11 +1922,6 @@ pub const ModuleLoader = struct { dumpSource(jsc_vm, specifier, &printer); } - const commonjs_exports = try bun.default_allocator.alloc(ZigString, parse_result.ast.commonjs_export_names.len); - for (parse_result.ast.commonjs_export_names, commonjs_exports) |name, *out| { - out.* = ZigString.fromUTF8(name); - } - defer { if (is_main) { jsc_vm.has_loaded = true; @@ -1962,17 +1930,7 @@ pub const ModuleLoader = struct { if (jsc_vm.isWatcherEnabled()) { var resolved_source = jsc_vm.refCountedResolvedSource(printer.ctx.written, input_specifier, path.text, null, false); - - resolved_source.commonjs_exports = if (commonjs_exports.len > 0) - commonjs_exports.ptr - else - null; - resolved_source.commonjs_exports_len = if (commonjs_exports.len > 0) - @as(u32, @truncate(commonjs_exports.len)) - else if (parse_result.ast.exports_kind == .cjs) - std.math.maxInt(u32) - else - 0; + resolved_source.is_commonjs_module = parse_result.ast.has_commonjs_export_names or parse_result.ast.exports_kind == .cjs; return resolved_source; } @@ -2010,18 +1968,8 @@ pub const ModuleLoader = struct { }, .specifier = input_specifier, .source_url = input_specifier.createIfDifferent(path.text), - .commonjs_exports = if (commonjs_exports.len > 0) - commonjs_exports.ptr - else - null, - .commonjs_exports_len = if (commonjs_exports.len > 0) - @as(u32, @truncate(commonjs_exports.len)) - else if (parse_result.ast.exports_kind == .cjs) - std.math.maxInt(u32) - else - 0, + .is_commonjs_module = parse_result.ast.has_commonjs_export_names or parse_result.ast.exports_kind == .cjs, .hash = 0, - .tag = tag, }; }, diff --git a/src/bundler.zig b/src/bundler.zig index 3f7de364e37c23..ac6a81203c9823 100644 --- a/src/bundler.zig +++ b/src/bundler.zig @@ -1234,6 +1234,18 @@ pub const Bundler = struct { comptime format: js_printer.Format, handler: js_printer.SourceMapHandler, ) !usize { + if (bun.getRuntimeFeatureFlag("BUN_FEATURE_FLAG_DISABLE_SOURCE_MAPS")) { + return bundler.printWithSourceMapMaybe( + result.ast, + &result.source, + Writer, + writer, + format, + false, + handler, + result.runtime_transpiler_cache, + ); + } return bundler.printWithSourceMapMaybe( result.ast, &result.source, diff --git a/src/js_ast.zig b/src/js_ast.zig index 323ccbda76582b..f0f3f93ae96b24 100644 --- a/src/js_ast.zig +++ b/src/js_ast.zig @@ -6636,7 +6636,7 @@ pub const Ast = struct { /// Not to be confused with `commonjs_named_exports` /// This is a list of named exports that may exist in a CommonJS module /// We use this with `commonjs_at_runtime` to re-export CommonJS - commonjs_export_names: []string = &([_]string{}), + has_commonjs_export_names: bool = false, import_meta_ref: Ref = Ref.None, pub const CommonJSNamedExport = struct { diff --git a/src/js_parser.zig b/src/js_parser.zig index 4a23c0323808cb..e7322655bde21a 100644 --- a/src/js_parser.zig +++ b/src/js_parser.zig @@ -5050,7 +5050,7 @@ fn NewParser_( parse_pass_symbol_uses: ParsePassSymbolUsageType = undefined, /// Used by commonjs_at_runtime - commonjs_export_names: bun.StringArrayHashMapUnmanaged(void) = .{}, + has_commonjs_export_names: bool = false, /// When this flag is enabled, we attempt to fold all expressions that /// TypeScript would consider to be "constant expressions". This flag is @@ -18896,8 +18896,7 @@ fn NewParser_( name_loc, ); } else if (p.options.features.commonjs_at_runtime and identifier_opts.assign_target != .none) { - // Record this CommonJS export name for use later. - _ = p.commonjs_export_names.getOrPut(p.allocator, name) catch unreachable; + p.has_commonjs_export_names = true; } } } @@ -23910,7 +23909,7 @@ fn NewParser_( false, // .top_Level_await_keyword = p.top_level_await_keyword, .commonjs_named_exports = p.commonjs_named_exports, - .commonjs_export_names = p.commonjs_export_names.keys(), + .has_commonjs_export_names = p.has_commonjs_export_names, .hashbang = hashbang, diff --git a/src/js_printer.zig b/src/js_printer.zig index 821177fe0423e6..76fb306a622b3f 100644 --- a/src/js_printer.zig +++ b/src/js_printer.zig @@ -6131,6 +6131,11 @@ pub fn printAst( renamer, getSourceMapBuilder(if (generate_source_map) .lazy else .disable, ascii_only, opts, source, &tree), ); + defer { + if (comptime generate_source_map) { + printer.source_map_builder.line_offset_tables.deinit(opts.allocator); + } + } var bin_stack_heap = std.heap.stackFallback(1024, bun.default_allocator); printer.binary_expression_stack = std.ArrayList(PrinterType.BinaryExpressionVisitor).init(bin_stack_heap.get()); defer printer.binary_expression_stack.clearAndFree();