From ef91b0e26273a3faed431b642eec2832b85adc4f Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 28 May 2022 03:10:57 +0900 Subject: [PATCH 1/4] src: merge RunInThisContext() with RunInContext() This commit resolves a TODO in `RunInThisContext()` by merging `RunInThisContext()` with `RunInContext()`. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/vm.js | 23 +++++++++----- src/node_contextify.cc | 70 +++++++++++++----------------------------- 2 files changed, 37 insertions(+), 56 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..33dd6e3df97b2b 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,28 @@ 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; + Environment* eval_env = nullptr; + 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); + + context = contextify_context->context(); + if (context.IsEmpty()) return; + + eval_env = contextify_context->env(); + microtask_queue = contextify_context->microtask_queue(); + } else { + eval_env = env; + context = env->context(); + } TRACE_EVENT0(TRACING_CATEGORY_NODE2(vm, script), "RunInContext"); @@ -914,12 +888,12 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo& args) { // Do the eval within the context Context::Scope context_scope(context); - EvalMachine(contextify_context->env(), + EvalMachine(eval_env, timeout, display_errors, break_on_sigint, break_on_first_line, - contextify_context->microtask_queue(), + microtask_queue, args); } From 21963feeba39d1c6a02cfd4bdbf083107a2fa128 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Wed, 1 Jun 2022 10:14:18 +0900 Subject: [PATCH 2/4] fixup: remove the trace event name Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- test/parallel/test-trace-events-vm.js | 1 - 1 file changed, 1 deletion(-) 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', ]; From ff52370c4a3b9ceb7c2c4d1ad1923831402112a0 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Wed, 1 Jun 2022 10:16:00 +0900 Subject: [PATCH 3/4] fixup: remove the function declaration Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- src/node_contextify.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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, From 06a97fd604d8dfbd74a9b9795287bd5ad5cc4bad Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Mon, 6 Jun 2022 14:28:27 +0900 Subject: [PATCH 4/4] fixup: merge eval_env with env Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- src/node_contextify.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 33dd6e3df97b2b..2460fa9361b56b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -852,7 +852,6 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo& args) { CHECK(args[0]->IsObject() || args[0]->IsNull()); Local context; - Environment* eval_env = nullptr; std::shared_ptr microtask_queue; if (args[0]->IsObject()) { @@ -861,14 +860,13 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo& args) { 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; - eval_env = contextify_context->env(); microtask_queue = contextify_context->microtask_queue(); } else { - eval_env = env; context = env->context(); } @@ -888,7 +886,7 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo& args) { // Do the eval within the context Context::Scope context_scope(context); - EvalMachine(eval_env, + EvalMachine(env, timeout, display_errors, break_on_sigint,