Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit a86a2e1

Browse filesBrowse files
joyeecheungrichardlau
joyeecheung
authored and
richardlau
committed
module: use symbol in WeakMap to manage host defined options
Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in #46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: #48510 Backport-PR-URL: #51004 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
1 parent 2c2892b commit a86a2e1
Copy full SHA for a86a2e1

File tree

Expand file treeCollapse file tree

16 files changed

+185
-191
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

16 files changed

+185
-191
lines changed
Open diff view settings
Collapse file

‎lib/internal/modules/esm/create_dynamic_module.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/create_dynamic_module.js
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,9 @@ import.meta.done();
6969
if (imports.length) {
7070
reflect.imports = { __proto__: null };
7171
}
72-
const { setCallbackForWrap } = require('internal/modules/esm/utils');
73-
setCallbackForWrap(m, {
72+
const { registerModule } = require('internal/modules/esm/utils');
73+
registerModule(m, {
74+
__proto__: null,
7475
initializeImportMeta: (meta, wrap) => {
7576
meta.exports = reflect.exports;
7677
if (reflect.imports) {
Collapse file

‎lib/internal/modules/esm/loader.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,9 +210,10 @@ class ModuleLoader {
210210
) {
211211
const evalInstance = (url) => {
212212
const { ModuleWrap } = internalBinding('module_wrap');
213-
const { setCallbackForWrap } = require('internal/modules/esm/utils');
213+
const { registerModule } = require('internal/modules/esm/utils');
214214
const module = new ModuleWrap(url, undefined, source, 0, 0);
215-
setCallbackForWrap(module, {
215+
registerModule(module, {
216+
__proto__: null,
216217
initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }),
217218
importModuleDynamically: (specifier, { url }, importAttributes) => {
218219
return this.import(specifier, url, importAttributes);
Collapse file

‎lib/internal/modules/esm/translators.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/translators.js
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,8 +150,9 @@ translators.set('module', async function moduleStrategy(url, source, isMain) {
150150
maybeCacheSourceMap(url, source);
151151
debug(`Translating StandardModule ${url}`);
152152
const module = new ModuleWrap(url, undefined, source, 0, 0);
153-
const { setCallbackForWrap } = require('internal/modules/esm/utils');
154-
setCallbackForWrap(module, {
153+
const { registerModule } = require('internal/modules/esm/utils');
154+
registerModule(module, {
155+
__proto__: null,
155156
initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }),
156157
importModuleDynamically,
157158
});
Collapse file

‎lib/internal/modules/esm/utils.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/utils.js
+69-20Lines changed: 69 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ const {
77
ObjectFreeze,
88
} = primordials;
99

10+
const {
11+
privateSymbols: {
12+
host_defined_option_symbol,
13+
},
14+
} = internalBinding('util');
1015
const {
1116
ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING,
1217
ERR_INVALID_ARG_VALUE,
@@ -21,16 +26,8 @@ const {
2126
setImportModuleDynamicallyCallback,
2227
setInitializeImportMetaObjectCallback,
2328
} = internalBinding('module_wrap');
24-
const {
25-
getModuleFromWrap,
26-
} = require('internal/vm/module');
2729
const assert = require('internal/assert');
2830

29-
const callbackMap = new SafeWeakMap();
30-
function setCallbackForWrap(wrap, data) {
31-
callbackMap.set(wrap, data);
32-
}
33-
3431
let defaultConditions;
3532
/**
3633
* Returns the default conditions for ES module loading.
@@ -83,34 +80,86 @@ function getConditionsSet(conditions) {
8380
return getDefaultConditionsSet();
8481
}
8582

83+
/**
84+
* @callback ImportModuleDynamicallyCallback
85+
* @param {string} specifier
86+
* @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
87+
* @param {object} attributes
88+
* @returns { Promise<void> }
89+
*/
90+
91+
/**
92+
* @callback InitializeImportMetaCallback
93+
* @param {object} meta
94+
* @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
95+
*/
96+
97+
/**
98+
* @typedef {{
99+
* callbackReferrer: ModuleWrap|ContextifyScript|Function|vm.Module
100+
* initializeImportMeta? : InitializeImportMetaCallback,
101+
* importModuleDynamically? : ImportModuleDynamicallyCallback
102+
* }} ModuleRegistry
103+
*/
104+
105+
/**
106+
* @type {WeakMap<symbol, ModuleRegistry>}
107+
*/
108+
const moduleRegistries = new SafeWeakMap();
109+
110+
/**
111+
* V8 would make sure that as long as import() can still be initiated from
112+
* the referrer, the symbol referenced by |host_defined_option_symbol| should
113+
* be alive, which in term would keep the settings object alive through the
114+
* WeakMap, and in turn that keeps the referrer object alive, which would be
115+
* passed into the callbacks.
116+
* The reference goes like this:
117+
* [v8::internal::Script] (via host defined options) ----1--> [idSymbol]
118+
* [callbackReferrer] (via host_defined_option_symbol) ------2------^ |
119+
* ^----------3---- (via WeakMap)------
120+
* 1+3 makes sure that as long as import() can still be initiated, the
121+
* referrer wrap is still around and can be passed into the callbacks.
122+
* 2 is only there so that we can get the id symbol to configure the
123+
* weak map.
124+
* @param {ModuleWrap|ContextifyScript|Function} referrer The referrer to
125+
* get the id symbol from. This is different from callbackReferrer which
126+
* could be set by the caller.
127+
* @param {ModuleRegistry} registry
128+
*/
129+
function registerModule(referrer, registry) {
130+
const idSymbol = referrer[host_defined_option_symbol];
131+
// To prevent it from being GC'ed.
132+
registry.callbackReferrer ??= referrer;
133+
moduleRegistries.set(idSymbol, registry);
134+
}
135+
86136
/**
87137
* Defines the `import.meta` object for a given module.
88-
* @param {object} wrap - Reference to the module.
138+
* @param {symbol} symbol - Reference to the module.
89139
* @param {Record<string, string | Function>} meta - The import.meta object to initialize.
90140
*/
91-
function initializeImportMetaObject(wrap, meta) {
92-
if (callbackMap.has(wrap)) {
93-
const { initializeImportMeta } = callbackMap.get(wrap);
141+
function initializeImportMetaObject(symbol, meta) {
142+
if (moduleRegistries.has(symbol)) {
143+
const { initializeImportMeta, callbackReferrer } = moduleRegistries.get(symbol);
94144
if (initializeImportMeta !== undefined) {
95-
meta = initializeImportMeta(meta, getModuleFromWrap(wrap) || wrap);
145+
meta = initializeImportMeta(meta, callbackReferrer);
96146
}
97147
}
98148
}
99149

100150
/**
101151
* Asynchronously imports a module dynamically using a callback function. The native callback.
102-
* @param {object} wrap - Reference to the module.
152+
* @param {symbol} symbol - Reference to the module.
103153
* @param {string} specifier - The module specifier string.
104154
* @param {Record<string, string>} attributes - The import attributes object.
105155
* @returns {Promise<import('internal/modules/esm/loader.js').ModuleExports>} - The imported module object.
106156
* @throws {ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING} - If the callback function is missing.
107157
*/
108-
async function importModuleDynamicallyCallback(wrap, specifier, attributes) {
109-
if (callbackMap.has(wrap)) {
110-
const { importModuleDynamically } = callbackMap.get(wrap);
158+
async function importModuleDynamicallyCallback(symbol, specifier, attributes) {
159+
if (moduleRegistries.has(symbol)) {
160+
const { importModuleDynamically, callbackReferrer } = moduleRegistries.get(symbol);
111161
if (importModuleDynamically !== undefined) {
112-
return importModuleDynamically(
113-
specifier, getModuleFromWrap(wrap) || wrap, attributes);
162+
return importModuleDynamically(specifier, callbackReferrer, attributes);
114163
}
115164
}
116165
throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING();
@@ -176,7 +225,7 @@ async function initializeHooks() {
176225
}
177226

178227
module.exports = {
179-
setCallbackForWrap,
228+
registerModule,
180229
initializeESM,
181230
initializeHooks,
182231
getDefaultConditions,
Collapse file

‎lib/internal/vm.js‎

Copy file name to clipboardExpand all lines: lib/internal/vm.js
+4-3Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,10 @@ function internalCompileFunction(code, params, options) {
100100
const { importModuleDynamicallyWrap } = require('internal/vm/module');
101101
const wrapped = importModuleDynamicallyWrap(importModuleDynamically);
102102
const func = result.function;
103-
const { setCallbackForWrap } = require('internal/modules/esm/utils');
104-
setCallbackForWrap(result.cacheKey, {
105-
importModuleDynamically: (s, _k, i) => wrapped(s, func, i),
103+
const { registerModule } = require('internal/modules/esm/utils');
104+
registerModule(func, {
105+
__proto__: null,
106+
importModuleDynamically: wrapped,
106107
});
107108
}
108109

Collapse file

‎lib/internal/vm/module.js‎

Copy file name to clipboardExpand all lines: lib/internal/vm/module.js
+9-7Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ const {
1212
ObjectSetPrototypeOf,
1313
ReflectApply,
1414
SafePromiseAllReturnVoid,
15-
SafeWeakMap,
1615
Symbol,
1716
SymbolToStringTag,
1817
TypeError,
@@ -70,7 +69,6 @@ const STATUS_MAP = {
7069

7170
let globalModuleId = 0;
7271
const defaultModuleName = 'vm:module';
73-
const wrapToModuleMap = new SafeWeakMap();
7472

7573
const kWrap = Symbol('kWrap');
7674
const kContext = Symbol('kContext');
@@ -121,25 +119,30 @@ class Module {
121119
});
122120
}
123121

122+
let registry = { __proto__: null };
124123
if (sourceText !== undefined) {
125124
this[kWrap] = new ModuleWrap(identifier, context, sourceText,
126125
options.lineOffset, options.columnOffset,
127126
options.cachedData);
128-
const { setCallbackForWrap } = require('internal/modules/esm/utils');
129-
setCallbackForWrap(this[kWrap], {
127+
registry = {
128+
__proto__: null,
130129
initializeImportMeta: options.initializeImportMeta,
131130
importModuleDynamically: options.importModuleDynamically ?
132131
importModuleDynamicallyWrap(options.importModuleDynamically) :
133132
undefined,
134-
});
133+
};
135134
} else {
136135
assert(syntheticEvaluationSteps);
137136
this[kWrap] = new ModuleWrap(identifier, context,
138137
syntheticExportNames,
139138
syntheticEvaluationSteps);
140139
}
141140

142-
wrapToModuleMap.set(this[kWrap], this);
141+
// This will take precedence over the referrer as the object being
142+
// passed into the callbacks.
143+
registry.callbackReferrer = this;
144+
const { registerModule } = require('internal/modules/esm/utils');
145+
registerModule(this[kWrap], registry);
143146

144147
this[kContext] = context;
145148
}
@@ -446,5 +449,4 @@ module.exports = {
446449
SourceTextModule,
447450
SyntheticModule,
448451
importModuleDynamicallyWrap,
449-
getModuleFromWrap: (wrap) => wrapToModuleMap.get(wrap),
450452
};
Collapse file

‎lib/vm.js‎

Copy file name to clipboardExpand all lines: lib/vm.js
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,9 @@ class Script extends ContextifyScript {
106106
validateFunction(importModuleDynamically,
107107
'options.importModuleDynamically');
108108
const { importModuleDynamicallyWrap } = require('internal/vm/module');
109-
const { setCallbackForWrap } = require('internal/modules/esm/utils');
110-
setCallbackForWrap(this, {
109+
const { registerModule } = require('internal/modules/esm/utils');
110+
registerModule(this, {
111+
__proto__: null,
111112
importModuleDynamically:
112113
importModuleDynamicallyWrap(importModuleDynamically),
113114
});
Collapse file

‎src/env-inl.h‎

Copy file name to clipboardExpand all lines: src/env-inl.h
-10Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -345,16 +345,6 @@ inline AliasedInt32Array& Environment::stream_base_state() {
345345
return stream_base_state_;
346346
}
347347

348-
inline uint32_t Environment::get_next_module_id() {
349-
return module_id_counter_++;
350-
}
351-
inline uint32_t Environment::get_next_script_id() {
352-
return script_id_counter_++;
353-
}
354-
inline uint32_t Environment::get_next_function_id() {
355-
return function_id_counter_++;
356-
}
357-
358348
ShouldNotAbortOnUncaughtScope::ShouldNotAbortOnUncaughtScope(
359349
Environment* env)
360350
: env_(env) {
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
-8Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -698,14 +698,6 @@ class Environment : public MemoryRetainer {
698698
builtins::BuiltinLoader* builtin_loader();
699699

700700
std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
701-
std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
702-
std::unordered_map<uint32_t, contextify::ContextifyScript*>
703-
id_to_script_map;
704-
std::unordered_map<uint32_t, contextify::CompiledFnEntry*> id_to_function_map;
705-
706-
inline uint32_t get_next_module_id();
707-
inline uint32_t get_next_script_id();
708-
inline uint32_t get_next_function_id();
709701

710702
EnabledDebugList* enabled_debug_list() { return &enabled_debug_list_; }
711703

Collapse file

‎src/env_properties.h‎

Copy file name to clipboardExpand all lines: src/env_properties.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
V(arrow_message_private_symbol, "node:arrowMessage") \
2222
V(contextify_context_private_symbol, "node:contextify:context") \
2323
V(decorated_private_symbol, "node:decorated") \
24+
V(host_defined_option_symbol, "node:host_defined_option_symbol") \
2425
V(napi_type_tag, "node:napi:type_tag") \
2526
V(napi_wrapper, "node:napi:wrapper") \
2627
V(untransferable_object_private_symbol, "node:untransferableObject") \
@@ -337,7 +338,6 @@
337338
V(blocklist_constructor_template, v8::FunctionTemplate) \
338339
V(contextify_global_template, v8::ObjectTemplate) \
339340
V(contextify_wrapper_template, v8::ObjectTemplate) \
340-
V(compiled_fn_entry_template, v8::ObjectTemplate) \
341341
V(crypto_key_object_handle_constructor, v8::FunctionTemplate) \
342342
V(env_proxy_template, v8::ObjectTemplate) \
343343
V(env_proxy_ctor_template, v8::FunctionTemplate) \

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.