From 76f5ce922bd76aaf329bd41b6e7cf2aaa28d081b Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
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.
---
 src/module_wrap.cc                            |  1 +
 .../test-vm-synthetic-module-leak.js          | 24 +++++++++++++++++++
 .../test-vm-source-text-module-leak.js        | 23 ++++++++++++++++++
 3 files changed, 48 insertions(+)
 create mode 100644 test/es-module/test-vm-synthetic-module-leak.js
 create mode 100644 test/known_issues/test-vm-source-text-module-leak.js

diff --git a/src/module_wrap.cc b/src/module_wrap.cc
index f164d801b65864..6f9fc3286deacc 100644
--- a/src/module_wrap.cc
+++ b/src/module_wrap.cc
@@ -58,6 +58,7 @@ ModuleWrap::ModuleWrap(Environment* env,
   object->SetInternalField(kURLSlot, url);
   object->SetInternalField(kSyntheticEvaluationStepsSlot, undefined);
   object->SetInternalField(kContextObjectSlot, undefined);
+  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..34cfc92501994d
--- /dev/null
+++ b/test/es-module/test-vm-synthetic-module-leak.js
@@ -0,0 +1,24 @@
+'use strict';
+
+// Flags: --experimental-vm-modules --max-heap-size=20
+// 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() {
+  const m = new vm.SyntheticModule(['bar'], () => {
+    m.setExport('bar', new Date().toISOString().repeat(1e6));
+  });
+  await m.link(() => {});
+  await m.evaluate();
+  count++;
+  if (count < 30000) {
+    setImmediate(createModule);
+  }
+  return m;
+}
+
+createModule();
diff --git a/test/known_issues/test-vm-source-text-module-leak.js b/test/known_issues/test-vm-source-text-module-leak.js
new file mode 100644
index 00000000000000..0404e04f7df0f8
--- /dev/null
+++ b/test/known_issues/test-vm-source-text-module-leak.js
@@ -0,0 +1,23 @@
+'use strict';
+
+// Flags: --experimental-vm-modules --max-heap-size=20
+// This leaks because of the strong persistent v8::Module references
+// from ModuleWrap. We need replace that with a GC-aware ModuleWrap ->
+// v8::Module reference to fix this.
+
+require('../common');
+const vm = require('vm');
+
+let count = 0;
+async function createModule() {
+  const m = new vm.SourceTextModule('export default "hello".repeat(1e5)');
+  await m.link(() => {});
+  await m.evaluate();
+  count++;
+  if (count < 30000) {
+    setImmediate(createModule);
+  }
+  return m;
+}
+
+createModule();