src: merge RunInThisContext() with RunInContext()

daeyeon · targos · commit 488742f2078b · 2022-07-20T09:14:01.000+02:00
This commit resolves a TODO in `RunInThisContext()` by merging `RunInThisContext()` with `RunInContext()`. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: #43225 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
diff --git a/lib/vm.js b/lib/vm.js
@@ -23,7 +23,6 @@
 
 const {
   ArrayPrototypeForEach,
-  ArrayPrototypeUnshift,
   Symbol,
   PromiseReject,
   ReflectApply,
@@ -123,17 +122,19 @@ class Script extends ContextifyScript {
   }
 
   runInThisContext(options) {
-    const { breakOnSigint, args } = getRunInContextArgs(options);
+    const { breakOnSigint, args } = getRunInContextArgs(null, options);
     if (breakOnSigint && process.listenerCount('SIGINT') > 0) {
-      return sigintHandlersWrap(super.runInThisContext, this, args);
+      return sigintHandlersWrap(super.runInContext, this, args);
     }
-    return ReflectApply(super.runInThisContext, this, args);
+    return ReflectApply(super.runInContext, this, args);
   }
 
   runInContext(contextifiedObject, options) {
     validateContext(contextifiedObject);
-    const { breakOnSigint, args } = getRunInContextArgs(options);
-    ArrayPrototypeUnshift(args, contextifiedObject);
+    const { breakOnSigint, args } = getRunInContextArgs(
+      contextifiedObject,
+      options,
+    );
     if (breakOnSigint && process.listenerCount('SIGINT') > 0) {
       return sigintHandlersWrap(super.runInContext, this, args);
     }
@@ -153,7 +154,7 @@ function validateContext(contextifiedObject) {
   }
 }
 
-function getRunInContextArgs(options = kEmptyObject) {
+function getRunInContextArgs(contextifiedObject, options = kEmptyObject) {
   validateObject(options, 'options');
 
   let timeout = options.timeout;
@@ -174,7 +175,13 @@ function getRunInContextArgs(options = kEmptyObject) {
 
   return {
     breakOnSigint,
-    args: [timeout, displayErrors, breakOnSigint, breakFirstLine]
+    args: [
+      contextifiedObject,
+      timeout,
+      displayErrors,
+      breakOnSigint,
+      breakFirstLine,
+    ],
   };
 }
 
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -671,7 +671,6 @@ void ContextifyScript::Init(Environment* env, Local<Object> target) {
   script_tmpl->SetClassName(class_name);
   env->SetProtoMethod(script_tmpl, "createCachedData", CreateCachedData);
   env->SetProtoMethod(script_tmpl, "runInContext", RunInContext);
-  env->SetProtoMethod(script_tmpl, "runInThisContext", RunInThisContext);
 
   Local<Context> context = env->context();
 
@@ -685,7 +684,6 @@ void ContextifyScript::RegisterExternalReferences(
   registry->Register(New);
   registry->Register(CreateCachedData);
   registry->Register(RunInContext);
-  registry->Register(RunInThisContext);
 }
 
 void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
@@ -852,59 +850,33 @@ void ContextifyScript::CreateCachedData(
   }
 }
 
-void ContextifyScript::RunInThisContext(
-    const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-
-  ContextifyScript* wrapped_script;
-  ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder());
-
-  TRACE_EVENT0(TRACING_CATEGORY_NODE2(vm, script), "RunInThisContext");
-
-  // TODO(addaleax): Use an options object or otherwise merge this with
-  // RunInContext().
-  CHECK_EQ(args.Length(), 4);
-
-  CHECK(args[0]->IsNumber());
-  int64_t timeout = args[0]->IntegerValue(env->context()).FromJust();
-
-  CHECK(args[1]->IsBoolean());
-  bool display_errors = args[1]->IsTrue();
-
-  CHECK(args[2]->IsBoolean());
-  bool break_on_sigint = args[2]->IsTrue();
-
-  CHECK(args[3]->IsBoolean());
-  bool break_on_first_line = args[3]->IsTrue();
-
-  // Do the eval within this context
-  EvalMachine(env,
-              timeout,
-              display_errors,
-              break_on_sigint,
-              break_on_first_line,
-              nullptr,  // microtask_queue
-              args);
-}
-
 void ContextifyScript::RunInContext(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
   ContextifyScript* wrapped_script;
   ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder());
 
   CHECK_EQ(args.Length(), 5);
+  CHECK(args[0]->IsObject() || args[0]->IsNull());
 
-  CHECK(args[0]->IsObject());
-  Local<Object> sandbox = args[0].As<Object>();
-  // Get the context from the sandbox
-  ContextifyContext* contextify_context =
-      ContextifyContext::ContextFromContextifiedSandbox(env, sandbox);
-  CHECK_NOT_NULL(contextify_context);
+  Local<Context> context;
+  std::shared_ptr<v8::MicrotaskQueue> microtask_queue;
 
-  Local<Context> context = contextify_context->context();
-  if (context.IsEmpty())
-    return;
+  if (args[0]->IsObject()) {
+    Local<Object> sandbox = args[0].As<Object>();
+    // Get the context from the sandbox
+    ContextifyContext* contextify_context =
+        ContextifyContext::ContextFromContextifiedSandbox(env, sandbox);
+    CHECK_NOT_NULL(contextify_context);
+    CHECK_EQ(contextify_context->env(), env);
+
+    context = contextify_context->context();
+    if (context.IsEmpty()) return;
+
+    microtask_queue = contextify_context->microtask_queue();
+  } else {
+    context = env->context();
+  }
 
   TRACE_EVENT0(TRACING_CATEGORY_NODE2(vm, script), "RunInContext");
 
@@ -922,12 +894,12 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo<Value>& args) {
 
   // Do the eval within the context
   Context::Scope context_scope(context);
-  EvalMachine(contextify_context->env(),
+  EvalMachine(env,
               timeout,
               display_errors,
               break_on_sigint,
               break_on_first_line,
-              contextify_context->microtask_queue(),
+              microtask_queue,
               args);
 }
 
diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -148,9 +148,7 @@ class ContextifyScript : public BaseObject {
   static void RegisterExternalReferences(ExternalReferenceRegistry* registry);
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static bool InstanceOf(Environment* env, const v8::Local<v8::Value>& args);
-  static void CreateCachedData(
-      const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void RunInThisContext(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void CreateCachedData(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void RunInContext(const v8::FunctionCallbackInfo<v8::Value>& args);
   static bool EvalMachine(Environment* env,
                           const int64_t timeout,
diff --git a/test/parallel/test-trace-events-vm.js b/test/parallel/test-trace-events-vm.js
@@ -8,7 +8,6 @@ const tmpdir = require('../common/tmpdir');
 
 const names = [
   'ContextifyScript::New',
-  'RunInThisContext',
   'RunInContext',
 ];
 
