Skip to content

Commit 6255572

Browse files
author
Vishal Ranaut
committed
vm: fix SourceTextModule memory leak
This commit fixes a memory leak in vm.SourceTextModule where the module namespace and all objects reachable from it were kept alive after evaluation. The issue was caused by an unconditional registration in the \moduleRegistries\ WeakMap, which was kept alive by the module's \idSymbol\ as long as the V8 Module was retained (e.g. by an AsyncContextFrame in the microtask queue). The fix explicitly unregisters the module from the registry after instantiation, unless \importModuleDynamically\ is provided (which inherently requires keeping the referrer alive to support dynamic imports at any time). Fixes: #63186
1 parent 9e58d9d commit 6255572

2 files changed

Lines changed: 62 additions & 0 deletions

File tree

lib/internal/vm/module.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,14 @@ class SourceTextModule extends Module {
405405
throw new ERR_VM_MODULE_STATUS('must be unlinked');
406406
}
407407
this[kWrap].instantiate();
408+
409+
if (this.#importModuleDynamically === undefined) {
410+
const { moduleRegistries } = require('internal/modules/esm/utils');
411+
const idSymbol = this[kWrap][host_defined_option_symbol];
412+
if (idSymbol) {
413+
moduleRegistries.delete(idSymbol);
414+
}
415+
}
408416
}
409417

410418
get dependencySpecifiers() {
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
'use strict';
2+
// Flags: --experimental-vm-modules --expose-gc
3+
4+
require('../common');
5+
const assert = require('assert');
6+
const { SourceTextModule } = require('vm');
7+
const v8 = require('v8');
8+
9+
async function run() {
10+
let initialMemory = 0;
11+
12+
// Run a few times to warm up the VM
13+
for (let i = 0; i < 5; i++) {
14+
const m = new SourceTextModule('import.meta.url; const x = new Array(1000).fill(1);', { identifier: `file://warmup${i}.js` });
15+
await m.link(() => {});
16+
await m.evaluate();
17+
}
18+
global.gc();
19+
initialMemory = process.memoryUsage().heapUsed;
20+
21+
process.on('unhandledRejection', () => {});
22+
const ctx = require('vm').createContext({});
23+
for (let i = 0; i < 1000; i++) {
24+
const m = new SourceTextModule(`
25+
import.meta.url;
26+
const x = new Array(1024 * 1024).fill(1); // some memory
27+
await new Promise(r => setTimeout(r, 0));
28+
throw new Error('foo');
29+
`, {
30+
context: ctx,
31+
identifier: `file://test${i}.js`,
32+
initializeImportMeta(meta) { meta.url = `file://test${i}.js`; }
33+
});
34+
await m.link(() => {});
35+
m.evaluate(); // Don't catch!
36+
}
37+
38+
// Wait for all microtasks to run
39+
await new Promise(r => setImmediate(r));
40+
41+
delete globalThis.importMetaUrl;
42+
global.gc();
43+
44+
const finalMemory = process.memoryUsage().heapUsed;
45+
const diffMB = (finalMemory - initialMemory) / 1024 / 1024;
46+
47+
// The leak was ~80MB for 1000 iterations.
48+
// We expect the diff to be small (e.g. < 5MB).
49+
assert(diffMB < 10, `Memory leaked: ${diffMB.toFixed(2)}MB`);
50+
}
51+
52+
run().then(() => {
53+
console.log('Test passed');
54+
});

0 commit comments

Comments
 (0)