vm: memory leak on vm.compileFunction with importModuleDynamically

[legendecas]

Version

v18.6.0

Platform

Darwin

Subsystem

vm

What steps will reproduce the bug?

Run node with node --experimental-vm-modules --max-heap-size=128 test.js, and "test.js" as:

const vm = require('vm');

function work() {
  const context = vm.createContext({});
  const fn = vm.compileFunction(`
    import('foo').then(() => {});
  `, [], {
    parsingContext: context,
    importModuleDynamically: async (specifier, fn, importAssertions) => {
      const m = new vm.SyntheticModule(['x'], () => {
        m.setExport('x', 1);
      }, {
        context,
      });

      await m.link(() => {});
      await m.evaluate();

      return m;
    },
  });

  fn();
}

(function main() {
  work()
  // yielding to give chance to the evaluation of promises.
  setTimeout(main, 1);
})();

How often does it reproduce? Is there a required condition?

always

What is the expected behavior?

Should not crash as OOM.

What do you see instead?

The program crashed with OOM shortly.

Additional information

Opening this issue to track the problem #44198 (comment).

[ywave620]

importModuleDynamically() callback is invoked everytime the import statement in the code paased to the vm is evaluated. The essential problem here is that it takes a vm.Script instance(created byvm.runInThisContext()) or a Function instance(created byvm.compileFunction()) as the second argument. Since we can not know when to evaluate the import statement(it may be scheduled to evaluate in one year...), we have to keep track the vm.Script instance or the Function instance forever.

However, I went through all JS files in /lib, and I didn't spot any usage of the second argument.
e.g.

Maybe we can fix this issue by disposing it. It's not backward compatible, but we have to admit this is a pitfall of the design.

[ywave620]

In case anyone who is confused, the dilemma we have now:

1. The lifetime of CompiledFnEntry(stored in Environment::id_to_function_map) is bounded to the Function created as a result of vm.compileFunction(), since that Function can be alive for a long time, the CompiledFnEntry is leaked!
2. In the scenario of vm.runInThisContext(), theContextifyScript(stored in Environment::id_to_script_map) is not strongly referenced at all, so it is a subject to GC. However, it is required when evaluating an import statement in the code paased to vm, hence, we might run into a segmentation fault in

   node/src/module_wrap.cc

   Lines 585 to 595 in 1531ef1

   	int type = options->Get(context, HostDefinedOptions::kType)
   	.As<Number>()
   	->Int32Value(context)
   	.ToChecked();
   	uint32_t id = options->Get(context, HostDefinedOptions::kID)
   	.As<Number>()
   	->Uint32Value(context)
   	.ToChecked();
   	if (type == ScriptType::kScript) {
   	contextify::ContextifyScript* wrap = env->id_to_script_map.find(id)->second;
   	object = wrap->object();

   if it is GC'ed prematurely. Check Segmentation fault when executing worker with eval: true containing dynamic import after await #43205 (comment) for how to trigger :).

The fix I proposed above works for either of these two issues.

[ywave620]

I would like to hear your ideas :) @legendecas