Skip to content

Commit 77b52fd

Browse files
targostargos
committed
module: move options checks from C++ to JS
PR-URL: nodejs/node#19822 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 0ac6ced commit 77b52fd

File tree

7 files changed

+114
-240
lines changed

7 files changed

+114
-240
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ const createDynamicModule = (exports, url = '', evaluate) => {
3232
}));`;
3333
const reflectiveModule = new ModuleWrap(src, `cjs-facade:${url}`);
3434
reflectiveModule.instantiate();
35-
const { setExecutor, reflect } = reflectiveModule.evaluate()();
35+
const { setExecutor, reflect } = reflectiveModule.evaluate(-1, false)();
3636
// public exposed ESM
3737
const reexports = `
3838
import {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ class ModuleJob {
103103
async run() {
104104
const module = await this.instantiate();
105105
try {
106-
module.evaluate();
106+
module.evaluate(-1, false);
107107
} catch (e) {
108108
e.stack;
109109
this.hadError = true;

‎lib/internal/vm/module.js

Lines changed: 54 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@
33
const { internalBinding } = require('internal/bootstrap/loaders');
44
const { emitExperimentalWarning } = require('internal/util');
55
const { URL } = require('internal/url');
6-
const { kParsingContext, isContext } = process.binding('contextify');
6+
const { isContext } = process.binding('contextify');
77
const {
88
ERR_INVALID_ARG_TYPE,
9+
ERR_OUT_OF_RANGE,
910
ERR_VM_MODULE_ALREADY_LINKED,
1011
ERR_VM_MODULE_DIFFERENT_CONTEXT,
1112
ERR_VM_MODULE_LINKING_ERRORED,
@@ -55,23 +56,26 @@ class Module {
5556
if (typeof src !== 'string')
5657
throw new ERR_INVALID_ARG_TYPE('src', 'string', src);
5758
if (typeof options !== 'object' || options === null)
58-
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
59+
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
5960

60-
let context;
61-
if (options.context !== undefined) {
62-
if (typeof options.context !== 'object' || options.context === null) {
63-
throw new ERR_INVALID_ARG_TYPE('options.context', 'object',
64-
options.context);
61+
const {
62+
context,
63+
lineOffset = 0,
64+
columnOffset = 0,
65+
initializeImportMeta
66+
} = options;
67+
68+
if (context !== undefined) {
69+
if (typeof context !== 'object' || context === null) {
70+
throw new ERR_INVALID_ARG_TYPE('options.context', 'Object', context);
6571
}
66-
if (isContext(options.context)) {
67-
context = options.context;
68-
} else {
69-
throw new ERR_INVALID_ARG_TYPE('options.context',
70-
'vm.Context', options.context);
72+
if (!isContext(context)) {
73+
throw new ERR_INVALID_ARG_TYPE('options.context', 'vm.Context',
74+
context);
7175
}
7276
}
7377

74-
let url = options.url;
78+
let { url } = options;
7579
if (url !== undefined) {
7680
if (typeof url !== 'string') {
7781
throw new ERR_INVALID_ARG_TYPE('options.url', 'string', url);
@@ -88,22 +92,19 @@ class Module {
8892
perContextModuleId.set(context, 1);
8993
}
9094

91-
if (options.initializeImportMeta !== undefined) {
92-
if (typeof options.initializeImportMeta === 'function') {
93-
initImportMetaMap.set(this, options.initializeImportMeta);
95+
validateInteger(lineOffset, 'options.lineOffset');
96+
validateInteger(columnOffset, 'options.columnOffset');
97+
98+
if (initializeImportMeta !== undefined) {
99+
if (typeof initializeImportMeta === 'function') {
100+
initImportMetaMap.set(this, initializeImportMeta);
94101
} else {
95102
throw new ERR_INVALID_ARG_TYPE(
96-
'options.initializeImportMeta', 'function',
97-
options.initializeImportMeta);
103+
'options.initializeImportMeta', 'function', initializeImportMeta);
98104
}
99105
}
100106

101-
const wrap = new ModuleWrap(src, url, {
102-
[kParsingContext]: context,
103-
lineOffset: options.lineOffset,
104-
columnOffset: options.columnOffset
105-
});
106-
107+
const wrap = new ModuleWrap(src, url, context, lineOffset, columnOffset);
107108
wrapMap.set(this, wrap);
108109
linkingStatusMap.set(this, 'unlinked');
109110
wrapToModuleMap.set(wrap, this);
@@ -194,7 +195,25 @@ class Module {
194195
wrap.instantiate();
195196
}
196197

197-
async evaluate(options) {
198+
async evaluate(options = {}) {
199+
if (typeof options !== 'object' || options === null) {
200+
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
201+
}
202+
203+
let timeout = options.timeout;
204+
if (timeout === undefined) {
205+
timeout = -1;
206+
} else if (!Number.isInteger(timeout) || timeout <= 0) {
207+
throw new ERR_INVALID_ARG_TYPE('options.timeout', 'a positive integer',
208+
timeout);
209+
}
210+
211+
const { breakOnSigint = false } = options;
212+
if (typeof breakOnSigint !== 'boolean') {
213+
throw new ERR_INVALID_ARG_TYPE('options.breakOnSigint', 'boolean',
214+
breakOnSigint);
215+
}
216+
198217
const wrap = wrapMap.get(this);
199218
const status = wrap.getStatus();
200219
if (status !== kInstantiated &&
@@ -204,7 +223,7 @@ class Module {
204223
'must be one of instantiated, evaluated, and errored'
205224
);
206225
}
207-
const result = wrap.evaluate(options);
226+
const result = wrap.evaluate(timeout, breakOnSigint);
208227
return { result, __proto__: null };
209228
}
210229

@@ -224,6 +243,15 @@ class Module {
224243
}
225244
}
226245

246+
function validateInteger(prop, propName) {
247+
if (!Number.isInteger(prop)) {
248+
throw new ERR_INVALID_ARG_TYPE(propName, 'integer', prop);
249+
}
250+
if ((prop >> 0) !== prop) {
251+
throw new ERR_OUT_OF_RANGE(propName, '32-bit integer', prop);
252+
}
253+
}
254+
227255
module.exports = {
228256
Module,
229257
initImportMetaMap,

‎src/module_wrap.cc

Lines changed: 57 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
namespace node {
1414
namespace loader {
1515

16+
using node::contextify::ContextifyContext;
1617
using node::url::URL;
1718
using node::url::URL_FLAGS_FAILED;
1819
using v8::Array;
@@ -77,54 +78,58 @@ ModuleWrap* ModuleWrap::GetFromModule(Environment* env,
7778

7879
void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
7980
Environment* env = Environment::GetCurrent(args);
81+
Isolate* isolate = env->isolate();
8082

81-
Isolate* isolate = args.GetIsolate();
83+
CHECK(args.IsConstructCall());
84+
Local<Object> that = args.This();
8285

83-
if (!args.IsConstructCall()) {
84-
env->ThrowError("constructor must be called using new");
85-
return;
86-
}
87-
88-
if (!args[0]->IsString()) {
89-
env->ThrowError("first argument is not a string");
90-
return;
91-
}
86+
const int argc = args.Length();
87+
CHECK_GE(argc, 2);
9288

89+
CHECK(args[0]->IsString());
9390
Local<String> source_text = args[0].As<String>();
9491

95-
if (!args[1]->IsString()) {
96-
env->ThrowError("second argument is not a string");
97-
return;
98-
}
99-
92+
CHECK(args[1]->IsString());
10093
Local<String> url = args[1].As<String>();
10194

102-
Local<Object> that = args.This();
95+
Local<Context> context;
96+
Local<Integer> line_offset;
97+
Local<Integer> column_offset;
10398

104-
Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
105-
TryCatch try_catch(isolate);
106-
107-
Local<Value> options = args[2];
108-
MaybeLocal<Integer> line_offset = contextify::GetLineOffsetArg(env, options);
109-
MaybeLocal<Integer> column_offset =
110-
contextify::GetColumnOffsetArg(env, options);
111-
MaybeLocal<Context> maybe_context = contextify::GetContextArg(env, options);
99+
if (argc == 5) {
100+
// new ModuleWrap(source, url, context?, lineOffset, columnOffset)
101+
if (args[2]->IsUndefined()) {
102+
context = that->CreationContext();
103+
} else {
104+
CHECK(args[2]->IsObject());
105+
ContextifyContext* sandbox =
106+
ContextifyContext::ContextFromContextifiedSandbox(
107+
env, args[2].As<Object>());
108+
CHECK_NE(sandbox, nullptr);
109+
context = sandbox->context();
110+
}
112111

112+
CHECK(args[3]->IsNumber());
113+
line_offset = args[3].As<Integer>();
113114

114-
if (try_catch.HasCaught()) {
115-
no_abort_scope.Close();
116-
try_catch.ReThrow();
117-
return;
115+
CHECK(args[4]->IsNumber());
116+
column_offset = args[4].As<Integer>();
117+
} else {
118+
// new ModuleWrap(source, url)
119+
context = that->CreationContext();
120+
line_offset = Integer::New(isolate, 0);
121+
column_offset = Integer::New(isolate, 0);
118122
}
119123

120-
Local<Context> context = maybe_context.FromMaybe(that->CreationContext());
124+
Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
125+
TryCatch try_catch(isolate);
121126
Local<Module> module;
122127

123128
// compile
124129
{
125130
ScriptOrigin origin(url,
126-
line_offset.ToLocalChecked(), // line offset
127-
column_offset.ToLocalChecked(), // column offset
131+
line_offset, // line offset
132+
column_offset, // column offset
128133
False(isolate), // is cross origin
129134
Local<Integer>(), // script id
130135
Local<Value>(), // source map URL
@@ -161,10 +166,9 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
161166
void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
162167
Environment* env = Environment::GetCurrent(args);
163168
Isolate* isolate = args.GetIsolate();
164-
if (!args[0]->IsFunction()) {
165-
env->ThrowError("first argument is not a function");
166-
return;
167-
}
169+
170+
CHECK_EQ(args.Length(), 1);
171+
CHECK(args[0]->IsFunction());
168172

169173
Local<Object> that = args.This();
170174

@@ -239,27 +243,23 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
239243

240244
void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
241245
Environment* env = Environment::GetCurrent(args);
242-
Isolate* isolate = args.GetIsolate();
246+
Isolate* isolate = env->isolate();
243247
ModuleWrap* obj;
244248
ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());
245249
Local<Context> context = obj->context_.Get(isolate);
246250
Local<Module> module = obj->module_.Get(isolate);
247251

248-
Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
249-
TryCatch try_catch(isolate);
250-
Maybe<int64_t> maybe_timeout =
251-
contextify::GetTimeoutArg(env, args[0]);
252-
Maybe<bool> maybe_break_on_sigint =
253-
contextify::GetBreakOnSigintArg(env, args[0]);
252+
// module.evaluate(timeout, breakOnSigint)
253+
CHECK_EQ(args.Length(), 2);
254254

255-
if (try_catch.HasCaught()) {
256-
no_abort_scope.Close();
257-
try_catch.ReThrow();
258-
return;
259-
}
255+
CHECK(args[0]->IsNumber());
256+
int64_t timeout = args[0]->IntegerValue(env->context()).FromJust();
260257

261-
int64_t timeout = maybe_timeout.ToChecked();
262-
bool break_on_sigint = maybe_break_on_sigint.ToChecked();
258+
CHECK(args[1]->IsBoolean());
259+
bool break_on_sigint = args[1]->IsTrue();
260+
261+
Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
262+
TryCatch try_catch(isolate);
263263

264264
bool timed_out = false;
265265
bool received_signal = false;
@@ -665,26 +665,14 @@ Maybe<URL> Resolve(Environment* env,
665665
void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
666666
Environment* env = Environment::GetCurrent(args);
667667

668-
if (args.IsConstructCall()) {
669-
env->ThrowError("resolve() must not be called as a constructor");
670-
return;
671-
}
672-
if (args.Length() != 2) {
673-
env->ThrowError("resolve must have exactly 2 arguments (string, string)");
674-
return;
675-
}
668+
// module.resolve(specifier, url)
669+
CHECK_EQ(args.Length(), 2);
676670

677-
if (!args[0]->IsString()) {
678-
env->ThrowError("first argument is not a string");
679-
return;
680-
}
671+
CHECK(args[0]->IsString());
681672
Utf8Value specifier_utf8(env->isolate(), args[0]);
682673
std::string specifier_std(*specifier_utf8, specifier_utf8.length());
683674

684-
if (!args[1]->IsString()) {
685-
env->ThrowError("second argument is not a string");
686-
return;
687-
}
675+
CHECK(args[1]->IsString());
688676
Utf8Value url_utf8(env->isolate(), args[1]);
689677
URL url(*url_utf8, url_utf8.length());
690678

@@ -748,11 +736,9 @@ void ModuleWrap::SetImportModuleDynamicallyCallback(
748736
Isolate* iso = args.GetIsolate();
749737
Environment* env = Environment::GetCurrent(args);
750738
HandleScope handle_scope(iso);
751-
if (!args[0]->IsFunction()) {
752-
env->ThrowError("first argument is not a function");
753-
return;
754-
}
755739

740+
CHECK_EQ(args.Length(), 1);
741+
CHECK(args[0]->IsFunction());
756742
Local<Function> import_callback = args[0].As<Function>();
757743
env->set_host_import_module_dynamically_callback(import_callback);
758744

@@ -781,11 +767,9 @@ void ModuleWrap::SetInitializeImportMetaObjectCallback(
781767
const FunctionCallbackInfo<Value>& args) {
782768
Environment* env = Environment::GetCurrent(args);
783769
Isolate* isolate = env->isolate();
784-
if (!args[0]->IsFunction()) {
785-
env->ThrowError("first argument is not a function");
786-
return;
787-
}
788770

771+
CHECK_EQ(args.Length(), 1);
772+
CHECK(args[0]->IsFunction());
789773
Local<Function> import_meta_callback = args[0].As<Function>();
790774
env->set_host_initialize_import_meta_object_callback(import_meta_callback);
791775

0 commit comments

Comments
 (0)