7 files changed, 114 insertions, 240 deletions

diff --git a/lib/internal/modules/esm/create_dynamic_module.js b/lib/internal/modules/esm/create_dynamic_module.js
index 7e9777af51..e0c9726363 100644
--- a/lib/internal/modules/esm/create_dynamic_module.js
+++ b/lib/internal/modules/esm/create_dynamic_module.js
@@ -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 {
diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
index d948252829..8dbeef78e6 100644
--- a/lib/internal/modules/esm/module_job.js
+++ b/lib/internal/modules/esm/module_job.js
@@ -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;
diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js
index a64ccfe3b2..7284c8bd61 100644
--- a/lib/internal/vm/module.js
+++ b/lib/internal/vm/module.js
@@ -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,
@@ -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);
@@ -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);
@@ -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) {
+      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 &&
@@ -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 };
   }
 
@@ -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);
+  }
+}
+
 module.exports = {
   Module,
   initImportMetaMap,
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
index 617bae8b60..7bf7b9dd08 100644
--- a/src/module_wrap.cc
+++ b/src/module_wrap.cc
@@ -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;
@@ -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
@@ -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();
 
@@ -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;
@@ -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());
 
@@ -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);
 
@@ -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);
 
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index c71e9b5789..ce23576501 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -47,7 +47,6 @@ using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Name;
 using v8::NamedPropertyHandlerConfiguration;
-using v8::Nothing;
 using v8::Object;
 using v8::ObjectTemplate;
 using v8::PropertyAttribute;
@@ -586,132 +585,6 @@ void ContextifyContext::IndexedPropertyDeleterCallback(
   args.GetReturnValue().Set(false);
 }
 
-Maybe<bool> GetBreakOnSigintArg(Environment* env,
-                                Local<Value> options) {
-  if (options->IsUndefined() || options->IsString()) {
-    return Just(false);
-  }
-  if (!options->IsObject()) {
-    env->ThrowTypeError("options must be an object");
-    return Nothing<bool>();
-  }
-
-  Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "breakOnSigint");
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(), key);
-  if (maybe_value.IsEmpty())
-    return Nothing<bool>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  return Just(value->IsTrue());
-}
-
-Maybe<int64_t> GetTimeoutArg(Environment* env, Local<Value> options) {
-  if (options->IsUndefined() || options->IsString()) {
-    return Just<int64_t>(-1);
-  }
-  if (!options->IsObject()) {
-    env->ThrowTypeError("options must be an object");
-    return Nothing<int64_t>();
-  }
-
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(), env->timeout_string());
-  if (maybe_value.IsEmpty())
-    return Nothing<int64_t>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  if (value->IsUndefined()) {
-    return Just<int64_t>(-1);
-  }
-
-  Maybe<int64_t> timeout = value->IntegerValue(env->context());
-
-  if (timeout.IsJust() && timeout.ToChecked() <= 0) {
-    env->ThrowRangeError("timeout must be a positive number");
-    return Nothing<int64_t>();
-  }
-
-  return timeout;
-}
-
-MaybeLocal<Integer> GetLineOffsetArg(Environment* env,
-                                     Local<Value> options) {
-  Local<Integer> defaultLineOffset = Integer::New(env->isolate(), 0);
-
-  if (!options->IsObject()) {
-    return defaultLineOffset;
-  }
-
-  Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset");
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(), key);
-  if (maybe_value.IsEmpty())
-    return MaybeLocal<Integer>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  if (value->IsUndefined())
-    return defaultLineOffset;
-
-  return value->ToInteger(env->context());
-}
-
-MaybeLocal<Integer> GetColumnOffsetArg(Environment* env,
-                                       Local<Value> options) {
-  Local<Integer> defaultColumnOffset = Integer::New(env->isolate(), 0);
-
-  if (!options->IsObject()) {
-    return defaultColumnOffset;
-  }
-
-  Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset");
-  MaybeLocal<Value> maybe_value =
-    options.As<Object>()->Get(env->context(), key);
-  if (maybe_value.IsEmpty())
-    return MaybeLocal<Integer>();
-
-  Local<Value> value = maybe_value.ToLocalChecked();
-  if (value->IsUndefined())
-    return defaultColumnOffset;
-
-  return value->ToInteger(env->context());
-}
-
-MaybeLocal<Context> GetContextArg(Environment* env,
-                                  Local<Value> options) {
-  if (!options->IsObject())
-    return MaybeLocal<Context>();
-
-  MaybeLocal<Value> maybe_value =
-      options.As<Object>()->Get(env->context(),
-                                env->vm_parsing_context_symbol());
-  Local<Value> value;
-  if (!maybe_value.ToLocal(&value))
-    return MaybeLocal<Context>();
-
-  if (!value->IsObject()) {
-    if (!value->IsNullOrUndefined()) {
-      env->ThrowTypeError(
-          "contextifiedSandbox argument must be an object.");
-    }
-    return MaybeLocal<Context>();
-  }
-
-  ContextifyContext* sandbox =
-      ContextifyContext::ContextFromContextifiedSandbox(
-          env, value.As<Object>());
-  if (!sandbox) {
-    env->ThrowTypeError(
-        "sandbox argument must have been converted to a context.");
-    return MaybeLocal<Context>();
-  }
-
-  Local<Context> context = sandbox->context();
-  if (context.IsEmpty())
-    return MaybeLocal<Context>();
-  return context;
-}
-
 class ContextifyScript : public BaseObject {
  private:
   Persistent<UnboundScript> script_;
diff --git a/src/node_contextify.h b/src/node_contextify.h
index 1075bc5c68..565b8ef856 100644
--- a/src/node_contextify.h
+++ b/src/node_contextify.h
@@ -98,17 +98,6 @@ class ContextifyContext {
       const v8::PropertyCallbackInfo<v8::Boolean>& args);
 };
 
-v8::Maybe<bool> GetBreakOnSigintArg(
-    Environment* env, v8::Local<v8::Value> options);
-v8::Maybe<int64_t> GetTimeoutArg(
-    Environment* env, v8::Local<v8::Value> options);
-v8::MaybeLocal<v8::Integer> GetLineOffsetArg(
-    Environment* env, v8::Local<v8::Value> options);
-v8::MaybeLocal<v8::Integer> GetColumnOffsetArg(
-    Environment* env, v8::Local<v8::Value> options);
-v8::MaybeLocal<v8::Context> GetContextArg(
-    Environment* env, v8::Local<v8::Value> options);
-
 }  // namespace contextify
 }  // namespace node
 
diff --git a/test/parallel/test-internal-module-wrap.js b/test/parallel/test-internal-module-wrap.js
index 66a17a0f34..bb4a648ef7 100644
--- a/test/parallel/test-internal-module-wrap.js
+++ b/test/parallel/test-internal-module-wrap.js
@@ -24,6 +24,6 @@ const bar = new ModuleWrap('export const five = 5', 'bar');
 
   foo.instantiate();
 
-  assert.strictEqual(await foo.evaluate(), 6);
+  assert.strictEqual(await foo.evaluate(-1, false), 6);
   assert.strictEqual(foo.namespace().five, 5);
 })();