From 05778c4c0f9ef15b8bc50d3614c2872d1ef7f89f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Jun 2023 18:04:35 +0200 Subject: [PATCH] module: fix leak of vm.SyntheticModule Previously we maintain a strong persistent reference to the ModuleWrap to retrieve the ID-to-ModuleWrap mapping from the HostImportModuleDynamicallyCallback using the number ID stored in the host-defined options. As a result the ModuleWrap would be kept alive until the Environment is shut down, which would be a leak for user code. With the new symbol-based host-defined option we can just get the ModuleWrap from the JS-land WeakMap so there's now no need to maintain this strong reference. This would at least fix the leak for vm.SyntheticModule. vm.SourceTextModule is still leaking due to the strong persistent reference to the v8::Module. PR-URL: https://github.com/nodejs/node/pull/48510 Refs: https://github.com/nodejs/node/issues/44211 Refs: https://github.com/nodejs/node/issues/42080 Refs: https://github.com/nodejs/node/issues/47096 Refs: https://github.com/nodejs/node/issues/43205 Refs: https://github.com/nodejs/node/issues/38695 Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Stephen Belanger --- src/module_wrap.cc | 1 + .../test-vm-synthetic-module-leak.js | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 test/es-module/test-vm-synthetic-module-leak.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 576cc3123a8138..b1fbbee5257400 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -64,6 +64,7 @@ ModuleWrap::ModuleWrap(Environment* env, if (!synthetic_evaluation_step->IsUndefined()) { synthetic_ = true; } + MakeWeak(); } ModuleWrap::~ModuleWrap() { diff --git a/test/es-module/test-vm-synthetic-module-leak.js b/test/es-module/test-vm-synthetic-module-leak.js new file mode 100644 index 00000000000000..9de02cb22f1128 --- /dev/null +++ b/test/es-module/test-vm-synthetic-module-leak.js @@ -0,0 +1,23 @@ +// Flags: --experimental-vm-modules --max-old-space-size=16 +'use strict'; + +// This tests that vm.SyntheticModule does not leak. +// See https://github.com/nodejs/node/issues/44211 +require('../common'); +const vm = require('vm'); + +let count = 0; +async function createModule() { + // Try to reach the maximum old space size. + const m = new vm.SyntheticModule(['bar'], () => { + m.setExport('bar', new Array(512).fill('----')); + }); + await m.link(() => {}); + await m.evaluate(); + if (count++ < 4 * 1024) { + setTimeout(createModule, 1); + } + return m; +} + +createModule();