Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: move options checks from C++ to JS #19822

Closed
wants to merge 2 commits into from
Closed

module: move options checks from C++ to JS #19822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0dd8e17
module: move options checks from C++ to JS
targos Apr 4, 2018
b50b638
add signature comment
targos Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/create_dynamic_module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const createDynamicModule = (exports, url = '', evaluate) => {
}));`;
const reflectiveModule = new ModuleWrap(src, `cjs-facade:${url}`);
reflectiveModule.instantiate();
const { setExecutor, reflect } = reflectiveModule.evaluate()();
const { setExecutor, reflect } = reflectiveModule.evaluate(-1, false)();
// public exposed ESM
const reexports = `
import {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/module_job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class ModuleJob {
async run() {
const module = await this.instantiate();
try {
module.evaluate();
module.evaluate(-1, false);
} catch (e) {
e.stack;
this.hadError = true;
Expand Down
80 changes: 54 additions & 26 deletions lib/internal/vm/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
const { internalBinding } = require('internal/bootstrap/loaders');
const { emitExperimentalWarning } = require('internal/util');
const { URL } = require('internal/url');
const { kParsingContext, isContext } = process.binding('contextify');
const { isContext } = process.binding('contextify');
const {
ERR_INVALID_ARG_TYPE,
ERR_OUT_OF_RANGE,
ERR_VM_MODULE_ALREADY_LINKED,
ERR_VM_MODULE_DIFFERENT_CONTEXT,
ERR_VM_MODULE_LINKING_ERRORED,
Expand Down Expand Up @@ -55,23 +56,26 @@ class Module {
if (typeof src !== 'string')
throw new ERR_INVALID_ARG_TYPE('src', 'string', src);
if (typeof options !== 'object' || options === null)
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);

let context;
if (options.context !== undefined) {
if (typeof options.context !== 'object' || options.context === null) {
throw new ERR_INVALID_ARG_TYPE('options.context', 'object',
options.context);
const {
context,
lineOffset = 0,
columnOffset = 0,
initializeImportMeta
} = options;

if (context !== undefined) {
if (typeof context !== 'object' || context === null) {
throw new ERR_INVALID_ARG_TYPE('options.context', 'Object', context);
}
if (isContext(options.context)) {
context = options.context;
} else {
throw new ERR_INVALID_ARG_TYPE('options.context',
'vm.Context', options.context);
if (!isContext(context)) {
throw new ERR_INVALID_ARG_TYPE('options.context', 'vm.Context',
context);
}
}

let url = options.url;
let { url } = options;
if (url !== undefined) {
if (typeof url !== 'string') {
throw new ERR_INVALID_ARG_TYPE('options.url', 'string', url);
Expand All @@ -88,22 +92,19 @@ class Module {
perContextModuleId.set(context, 1);
}

if (options.initializeImportMeta !== undefined) {
if (typeof options.initializeImportMeta === 'function') {
initImportMetaMap.set(this, options.initializeImportMeta);
validateInteger(lineOffset, 'options.lineOffset');
validateInteger(columnOffset, 'options.columnOffset');

if (initializeImportMeta !== undefined) {
if (typeof initializeImportMeta === 'function') {
initImportMetaMap.set(this, initializeImportMeta);
} else {
throw new ERR_INVALID_ARG_TYPE(
'options.initializeImportMeta', 'function',
options.initializeImportMeta);
'options.initializeImportMeta', 'function', initializeImportMeta);
}
}

const wrap = new ModuleWrap(src, url, {
[kParsingContext]: context,
lineOffset: options.lineOffset,
columnOffset: options.columnOffset
});

const wrap = new ModuleWrap(src, url, context, lineOffset, columnOffset);
wrapMap.set(this, wrap);
linkingStatusMap.set(this, 'unlinked');
wrapToModuleMap.set(wrap, this);
Expand Down Expand Up @@ -194,7 +195,25 @@ class Module {
wrap.instantiate();
}

async evaluate(options) {
async evaluate(options = {}) {
if (typeof options !== 'object' || options === null) {
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
}

let timeout = options.timeout;
if (timeout === undefined) {
timeout = -1;
} else if (!Number.isInteger(timeout) || timeout <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should timeout also run through validateInteger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is a bit different. timeout may not be negative, while validateInteger allows negative values.

throw new ERR_INVALID_ARG_TYPE('options.timeout', 'a positive integer',
timeout);
}

const { breakOnSigint = false } = options;
if (typeof breakOnSigint !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE('options.breakOnSigint', 'boolean',
breakOnSigint);
}

const wrap = wrapMap.get(this);
const status = wrap.getStatus();
if (status !== kInstantiated &&
Expand All @@ -204,7 +223,7 @@ class Module {
'must be one of instantiated, evaluated, and errored'
);
}
const result = wrap.evaluate(options);
const result = wrap.evaluate(timeout, breakOnSigint);
return { result, __proto__: null };
}

Expand All @@ -224,6 +243,15 @@ class Module {
}
}

function validateInteger(prop, propName) {
if (!Number.isInteger(prop)) {
throw new ERR_INVALID_ARG_TYPE(propName, 'integer', prop);
}
if ((prop >> 0) !== prop) {
throw new ERR_OUT_OF_RANGE(propName, '32-bit integer', prop);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not belong somewhere a little more generic perhaps? Somewhat surprising there isn't already something for this too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, especially since I copied this function from another file. I'll look into bringing together type validation functions in another PR if that's fine for you.


module.exports = {
Module,
initImportMetaMap,
Expand Down
130 changes: 57 additions & 73 deletions src/module_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
namespace node {
namespace loader {

using node::contextify::ContextifyContext;
using node::url::URL;
using node::url::URL_FLAGS_FAILED;
using v8::Array;
Expand Down Expand Up @@ -77,54 +78,58 @@ ModuleWrap* ModuleWrap::GetFromModule(Environment* env,

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

Isolate* isolate = args.GetIsolate();
CHECK(args.IsConstructCall());
Local<Object> that = args.This();

if (!args.IsConstructCall()) {
env->ThrowError("constructor must be called using new");
return;
}

if (!args[0]->IsString()) {
env->ThrowError("first argument is not a string");
return;
}
const int argc = args.Length();
CHECK_GE(argc, 2);

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

if (!args[1]->IsString()) {
env->ThrowError("second argument is not a string");
return;
}

CHECK(args[1]->IsString());
Local<String> url = args[1].As<String>();

Local<Object> that = args.This();
Local<Context> context;
Local<Integer> line_offset;
Local<Integer> column_offset;

Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
TryCatch try_catch(isolate);

Local<Value> options = args[2];
MaybeLocal<Integer> line_offset = contextify::GetLineOffsetArg(env, options);
MaybeLocal<Integer> column_offset =
contextify::GetColumnOffsetArg(env, options);
MaybeLocal<Context> maybe_context = contextify::GetContextArg(env, options);
if (argc == 5) {
// new ModuleWrap(source, url, context?, lineOffset, columnOffset)
if (args[2]->IsUndefined()) {
context = that->CreationContext();
} else {
CHECK(args[2]->IsObject());
ContextifyContext* sandbox =
ContextifyContext::ContextFromContextifiedSandbox(
env, args[2].As<Object>());
CHECK_NE(sandbox, nullptr);
context = sandbox->context();
}

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

if (try_catch.HasCaught()) {
no_abort_scope.Close();
try_catch.ReThrow();
return;
CHECK(args[4]->IsNumber());
column_offset = args[4].As<Integer>();
} else {
// new ModuleWrap(source, url)
context = that->CreationContext();
line_offset = Integer::New(isolate, 0);
column_offset = Integer::New(isolate, 0);
}

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

// compile
{
ScriptOrigin origin(url,
line_offset.ToLocalChecked(), // line offset
column_offset.ToLocalChecked(), // column offset
line_offset, // line offset
column_offset, // column offset
False(isolate), // is cross origin
Local<Integer>(), // script id
Local<Value>(), // source map URL
Expand Down Expand Up @@ -161,10 +166,9 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = args.GetIsolate();
if (!args[0]->IsFunction()) {
env->ThrowError("first argument is not a function");
return;
}

CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsFunction());

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

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

void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = args.GetIsolate();
Isolate* isolate = env->isolate();
ModuleWrap* obj;
ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());
Local<Context> context = obj->context_.Get(isolate);
Local<Module> module = obj->module_.Get(isolate);

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

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

int64_t timeout = maybe_timeout.ToChecked();
bool break_on_sigint = maybe_break_on_sigint.ToChecked();
CHECK(args[1]->IsBoolean());
bool break_on_sigint = args[1]->IsTrue();

Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
TryCatch try_catch(isolate);

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

if (args.IsConstructCall()) {
env->ThrowError("resolve() must not be called as a constructor");
return;
}
if (args.Length() != 2) {
env->ThrowError("resolve must have exactly 2 arguments (string, string)");
return;
}
// module.resolve(specifier, url)
CHECK_EQ(args.Length(), 2);

if (!args[0]->IsString()) {
env->ThrowError("first argument is not a string");
return;
}
CHECK(args[0]->IsString());
Utf8Value specifier_utf8(env->isolate(), args[0]);
std::string specifier_std(*specifier_utf8, specifier_utf8.length());

if (!args[1]->IsString()) {
env->ThrowError("second argument is not a string");
return;
}
CHECK(args[1]->IsString());
Utf8Value url_utf8(env->isolate(), args[1]);
URL url(*url_utf8, url_utf8.length());

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

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

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

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

Expand Down
Loading