From 7f29993808c28f22ab3f6040022a4e45e3f27db7 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Mon, 11 Jul 2022 17:08:41 +0900 Subject: [PATCH] src: merge RunInThisContext() with RunInContext() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit resolves a TODO in `RunInThisContext()` by merging `RunInThisContext()` with `RunInContext()`. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: https://github.com/nodejs/node/pull/43225 Reviewed-By: Anna Henningsen Reviewed-By: Darshan Sen Reviewed-By: Minwoo Jung Reviewed-By: Chengzhong Wu Reviewed-By: Tobias Nießen Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel --- lib/vm.js | 23 +++++---- src/node_contextify.cc | 68 ++++++++------------------- src/node_contextify.h | 4 +- test/parallel/test-trace-events-vm.js | 1 - 4 files changed, 36 insertions(+), 60 deletions(-) diff --git a/lib/vm.js b/lib/vm.js index 7bcef6e5f4ff9a..1bb199363afbe1 100644 --- 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 index e7a4a4dd142cb2..2460fa9361b56b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -663,7 +663,6 @@ void ContextifyScript::Init(Environment* env, Local 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 = env->context(); @@ -677,7 +676,6 @@ void ContextifyScript::RegisterExternalReferences( registry->Register(New); registry->Register(CreateCachedData); registry->Register(RunInContext); - registry->Register(RunInThisContext); } void ContextifyScript::New(const FunctionCallbackInfo& args) { @@ -844,41 +842,6 @@ void ContextifyScript::CreateCachedData( } } -void ContextifyScript::RunInThisContext( - const FunctionCallbackInfo& 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& args) { Environment* env = Environment::GetCurrent(args); @@ -886,17 +849,26 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo& args) { 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 sandbox = args[0].As(); - // Get the context from the sandbox - ContextifyContext* contextify_context = - ContextifyContext::ContextFromContextifiedSandbox(env, sandbox); - CHECK_NOT_NULL(contextify_context); + Local context; + std::shared_ptr microtask_queue; - Local context = contextify_context->context(); - if (context.IsEmpty()) - return; + if (args[0]->IsObject()) { + Local sandbox = args[0].As(); + // 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"); @@ -914,12 +886,12 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo& 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 index c9ba78b8a5e185..d45b73b36e295d 100644 --- 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& args); static bool InstanceOf(Environment* env, const v8::Local& args); - static void CreateCachedData( - const v8::FunctionCallbackInfo& args); - static void RunInThisContext(const v8::FunctionCallbackInfo& args); + static void CreateCachedData(const v8::FunctionCallbackInfo& args); static void RunInContext(const v8::FunctionCallbackInfo& 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 index b3d3a403bec711..e7c5fb0bc0e416 100644 --- 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', ];