Visitar URL original
vm: unify host-defined option generation in vm.compileFunction · nodejs/node@a54179f · GitHub
Skip to content

Commit a54179f

Browse files
joyeecheungtargos
joyeecheung
authored and
targos
committed
vm: unify host-defined option generation in vm.compileFunction
Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent b6021ab commit a54179f

4 files changed

Lines changed: 46 additions & 24 deletions

File tree

‎lib/internal/vm.js‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,16 @@
22

33
const {
44
ArrayPrototypeForEach,
5+
Symbol,
56
} = primordials;
67

78
const {
89
compileFunction,
910
isContext: _isContext,
1011
} = internalBinding('contextify');
12+
const {
13+
default_host_defined_options,
14+
} = internalBinding('symbols');
1115
const {
1216
validateArray,
1317
validateBoolean,
@@ -30,12 +34,27 @@ function isContext(object) {
3034
return _isContext(object);
3135
}
3236

37+
function getHostDefinedOptionId(importModuleDynamically, filename) {
38+
if (importModuleDynamically !== undefined) {
39+
// Check that it's either undefined or a function before we pass
40+
// it into the native constructor.
41+
validateFunction(importModuleDynamically,
42+
'options.importModuleDynamically');
43+
}
44+
if (importModuleDynamically === undefined) {
45+
// We need a default host defined options that are the same for all
46+
// scripts not needing custom module callbacks so that the isolate
47+
// compilation cache can be hit.
48+
return default_host_defined_options;
49+
}
50+
return Symbol(filename);
51+
}
52+
3353
function internalCompileFunction(code, params, options) {
3454
validateString(code, 'code');
3555
if (params !== undefined) {
3656
validateStringArray(params, 'params');
3757
}
38-
3958
const {
4059
filename = '',
4160
columnOffset = 0,
@@ -72,6 +91,8 @@ function internalCompileFunction(code, params, options) {
7291
validateObject(extension, name, kValidateObjectAllowNullable);
7392
});
7493

94+
const hostDefinedOptionId =
95+
getHostDefinedOptionId(importModuleDynamically, filename);
7596
const result = compileFunction(
7697
code,
7798
filename,
@@ -82,6 +103,7 @@ function internalCompileFunction(code, params, options) {
82103
parsingContext,
83104
contextExtensions,
84105
params,
106+
hostDefinedOptionId,
85107
);
86108

87109
if (produceCachedData) {
@@ -113,6 +135,7 @@ function internalCompileFunction(code, params, options) {
113135
}
114136

115137
module.exports = {
138+
getHostDefinedOptionId,
116139
internalCompileFunction,
117140
isContext,
118141
};

‎lib/vm.js‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ const {
4141
const {
4242
validateBoolean,
4343
validateBuffer,
44-
validateFunction,
4544
validateInt32,
4645
validateObject,
4746
validateOneOf,
@@ -54,6 +53,7 @@ const {
5453
kVmBreakFirstLineSymbol,
5554
} = require('internal/util');
5655
const {
56+
getHostDefinedOptionId,
5757
internalCompileFunction,
5858
isContext,
5959
} = require('internal/vm');
@@ -86,12 +86,8 @@ class Script extends ContextifyScript {
8686
}
8787
validateBoolean(produceCachedData, 'options.produceCachedData');
8888

89-
if (importModuleDynamically !== undefined) {
90-
// Check that it's either undefined or a function before we pass
91-
// it into the native constructor.
92-
validateFunction(importModuleDynamically,
93-
'options.importModuleDynamically');
94-
}
89+
const hostDefinedOptionId =
90+
getHostDefinedOptionId(importModuleDynamically, filename);
9591
// Calling `ReThrow()` on a native TryCatch does not generate a new
9692
// abort-on-uncaught-exception check. A dummy try/catch in JS land
9793
// protects against that.
@@ -103,7 +99,7 @@ class Script extends ContextifyScript {
10399
cachedData,
104100
produceCachedData,
105101
parsingContext,
106-
importModuleDynamically !== undefined);
102+
hostDefinedOptionId);
107103
} catch (e) {
108104
throw e; /* node-do-not-add-exception-line */
109105
}

‎src/node_contextify.cc‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -771,11 +771,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
771771
bool produce_cached_data = false;
772772
Local<Context> parsing_context = context;
773773

774-
bool needs_custom_host_defined_options = false;
774+
Local<Symbol> id_symbol;
775775
if (argc > 2) {
776776
// new ContextifyScript(code, filename, lineOffset, columnOffset,
777777
// cachedData, produceCachedData, parsingContext,
778-
// needsCustomHostDefinedOptions)
778+
// hostDefinedOptionId)
779779
CHECK_EQ(argc, 8);
780780
CHECK(args[2]->IsNumber());
781781
line_offset = args[2].As<Int32>()->Value();
@@ -795,9 +795,8 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
795795
CHECK_NOT_NULL(sandbox);
796796
parsing_context = sandbox->context();
797797
}
798-
if (args[7]->IsTrue()) {
799-
needs_custom_host_defined_options = true;
800-
}
798+
CHECK(args[7]->IsSymbol());
799+
id_symbol = args[7].As<Symbol>();
801800
}
802801

803802
ContextifyScript* contextify_script =
@@ -821,12 +820,6 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
821820

822821
Local<PrimitiveArray> host_defined_options =
823822
PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
824-
// We need a default host defined options that's the same for all scripts
825-
// not needing custom module callbacks for so that the isolate compilation
826-
// cache can be hit.
827-
Local<Symbol> id_symbol = needs_custom_host_defined_options
828-
? Symbol::New(isolate, filename)
829-
: env->default_host_defined_options();
830823
host_defined_options->Set(
831824
isolate, loader::HostDefinedOptions::kID, id_symbol);
832825

@@ -1199,6 +1192,10 @@ void ContextifyContext::CompileFunction(
11991192
params_buf = args[8].As<Array>();
12001193
}
12011194

1195+
// Argument 10: host-defined option symbol
1196+
CHECK(args[9]->IsSymbol());
1197+
Local<Symbol> id_symbol = args[9].As<Symbol>();
1198+
12021199
// Read cache from cached data buffer
12031200
ScriptCompiler::CachedData* cached_data = nullptr;
12041201
if (!cached_data_buf.IsEmpty()) {
@@ -1210,7 +1207,6 @@ void ContextifyContext::CompileFunction(
12101207
// Set host_defined_options
12111208
Local<PrimitiveArray> host_defined_options =
12121209
PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
1213-
Local<Symbol> id_symbol = Symbol::New(isolate, filename);
12141210
host_defined_options->Set(
12151211
isolate, loader::HostDefinedOptions::kID, id_symbol);
12161212

‎test/parallel/test-vm-no-dynamic-import-callback.js‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
'use strict';
22

3-
require('../common');
4-
const { Script } = require('vm');
3+
const common = require('../common');
4+
const { Script, compileFunction } = require('vm');
55
const assert = require('assert');
66

77
assert.rejects(async () => {
@@ -10,4 +10,11 @@ assert.rejects(async () => {
1010
await imported;
1111
}, {
1212
code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING'
13-
});
13+
}).then(common.mustCall());
14+
15+
assert.rejects(async () => {
16+
const imported = compileFunction('return import("fs")')();
17+
await imported;
18+
}, {
19+
code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING'
20+
}).then(common.mustCall());

0 commit comments

Comments
 (0)