Skip to content

Commit

Permalink
Fix a small memory leak when requiring CommonJS modules (#12984)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarred-Sumner
Jarred-Sumner authored Aug 1, 2024
1 parent 49ab4c1 commit dc620ea
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 75 deletions.
6 changes: 3 additions & 3 deletions src/bun.js/bindings/ModuleLoader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ extern "C" void Bun__onFulfillAsyncModule(
}
}

if (res->result.value.commonJSExportsLen) {
if (res->result.value.isCommonJSModule) {
auto created = Bun::createCommonJSModule(jsCast<Zig::GlobalObject*>(globalObject), specifierValue, res->result.value);
if (created.has_value()) {
JSSourceCode* code = JSSourceCode::create(vm, WTFMove(created.value()));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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()) {
Expand Down
6 changes: 1 addition & 5 deletions src/bun.js/bindings/exports.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down
3 changes: 1 addition & 2 deletions src/bun.js/bindings/headers-handwritten.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
68 changes: 8 additions & 60 deletions src/bun.js/module_loader.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
};
}
Expand Down Expand Up @@ -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);

Expand All @@ -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;
}
Expand All @@ -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,
};
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -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,
};
},
Expand Down
12 changes: 12 additions & 0 deletions src/bundler.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/js_ast.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 3 additions & 4 deletions src/js_parser.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}
}
Expand Down Expand Up @@ -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,

Expand Down
5 changes: 5 additions & 0 deletions src/js_printer.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit dc620ea

Please sign in to comment.