From 1066341cd90ffbad77f3f207e09ee15c3e4da7f4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 5 Apr 2020 23:13:31 +0200 Subject: [PATCH] src: initialize inspector before RunBootstrapping() This is necessary for `--inspect-brk-node` to work, and for the inspector to be aware of scripts created before bootstrapping. Fixes: https://github.com/nodejs/node/issues/32648 Refs: https://github.com/nodejs/node/pull/30467#discussion_r396879908 Backport-PR-URL: https://github.com/nodejs/node/pull/35241 PR-URL: https://github.com/nodejs/node/pull/32672 Reviewed-By: Gus Caplan Reviewed-By: Eugene Ostroukhov Reviewed-By: Joyee Cheung Reviewed-By: David Carlier Reviewed-By: James M Snell --- src/api/environment.cc | 56 +++++++++---------- src/node.h | 18 +++--- src/node_worker.cc | 9 +-- test/cctest/node_test_fixture.h | 12 +++- test/cctest/test_environment.cc | 11 ++-- .../test-inspector-inspect-brk-node.js | 28 ++++++++++ 6 files changed, 85 insertions(+), 49 deletions(-) create mode 100644 test/parallel/test-inspector-inspect-brk-node.js diff --git a/src/api/environment.cc b/src/api/environment.cc index e47c5d42292615..ad980c0694a90a 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -317,6 +317,19 @@ void FreeIsolateData(IsolateData* isolate_data) { delete isolate_data; } +InspectorParentHandle::~InspectorParentHandle() {} + +// Hide the internal handle class from the public API. +#if HAVE_INSPECTOR +struct InspectorParentHandleImpl : public InspectorParentHandle { + std::unique_ptr impl; + + explicit InspectorParentHandleImpl( + std::unique_ptr&& impl) + : impl(std::move(impl)) {} +}; +#endif + Environment* CreateEnvironment(IsolateData* isolate_data, Local context, int argc, @@ -335,7 +348,8 @@ Environment* CreateEnvironment( const std::vector& args, const std::vector& exec_args, EnvironmentFlags::Flags flags, - ThreadId thread_id) { + ThreadId thread_id, + std::unique_ptr inspector_parent_handle) { Isolate* isolate = context->GetIsolate(); HandleScope handle_scope(isolate); Context::Scope context_scope(context); @@ -352,6 +366,16 @@ Environment* CreateEnvironment( env->set_abort_on_uncaught_exception(false); } +#if HAVE_INSPECTOR + if (inspector_parent_handle) { + env->InitializeInspector( + std::move(static_cast( + inspector_parent_handle.get())->impl)); + } else { + env->InitializeInspector({}); + } +#endif + if (env->RunBootstrapping().IsEmpty()) { FreeEnvironment(env); return nullptr; @@ -381,19 +405,6 @@ void FreeEnvironment(Environment* env) { delete env; } -InspectorParentHandle::~InspectorParentHandle() {} - -// Hide the internal handle class from the public API. -#if HAVE_INSPECTOR -struct InspectorParentHandleImpl : public InspectorParentHandle { - std::unique_ptr impl; - - explicit InspectorParentHandleImpl( - std::unique_ptr&& impl) - : impl(std::move(impl)) {} -}; -#endif - NODE_EXTERN std::unique_ptr GetInspectorParentHandle( Environment* env, ThreadId thread_id, @@ -417,27 +428,17 @@ void LoadEnvironment(Environment* env) { MaybeLocal LoadEnvironment( Environment* env, StartExecutionCallback cb, - std::unique_ptr inspector_parent_handle) { + std::unique_ptr removeme) { env->InitializeLibuv(per_process::v8_is_profiling); env->InitializeDiagnostics(); -#if HAVE_INSPECTOR - if (inspector_parent_handle) { - env->InitializeInspector( - std::move(static_cast( - inspector_parent_handle.get())->impl)); - } else { - env->InitializeInspector({}); - } -#endif - return StartExecution(env, cb); } MaybeLocal LoadEnvironment( Environment* env, const char* main_script_source_utf8, - std::unique_ptr inspector_parent_handle) { + std::unique_ptr removeme) { CHECK_NOT_NULL(main_script_source_utf8); return LoadEnvironment( env, @@ -462,8 +463,7 @@ MaybeLocal LoadEnvironment( env->process_object(), env->native_module_require()}; return ExecuteBootstrapper(env, name.c_str(), ¶ms, &args); - }, - std::move(inspector_parent_handle)); + }); } Environment* GetCurrentEnvironment(Local context) { diff --git a/src/node.h b/src/node.h index eea39ed44eda0c..fbe9c9e74061ec 100644 --- a/src/node.h +++ b/src/node.h @@ -400,6 +400,10 @@ enum Flags : uint64_t { }; } // namespace EnvironmentFlags +struct InspectorParentHandle { + virtual ~InspectorParentHandle(); +}; + // TODO(addaleax): Maybe move per-Environment options parsing here. // Returns nullptr when the Environment cannot be created e.g. there are // pending JavaScript exceptions. @@ -416,16 +420,14 @@ NODE_EXTERN Environment* CreateEnvironment( const std::vector& args, const std::vector& exec_args, EnvironmentFlags::Flags flags = EnvironmentFlags::kDefaultFlags, - ThreadId thread_id = {} /* allocates a thread id automatically */); + ThreadId thread_id = {} /* allocates a thread id automatically */, + std::unique_ptr inspector_parent_handle = {}); -struct InspectorParentHandle { - virtual ~InspectorParentHandle(); -}; // Returns a handle that can be passed to `LoadEnvironment()`, making the // child Environment accessible to the inspector as if it were a Node.js Worker. // `child_thread_id` can be created using `AllocateEnvironmentThreadId()` // and then later passed on to `CreateEnvironment()` to create the child -// Environment. +// Environment, together with the inspector handle. // This method should not be called while the parent Environment is active // on another thread. NODE_EXTERN std::unique_ptr GetInspectorParentHandle( @@ -443,14 +445,16 @@ using StartExecutionCallback = // TODO(addaleax): Deprecate this in favour of the MaybeLocal<> overload. NODE_EXTERN void LoadEnvironment(Environment* env); +// The `InspectorParentHandle` arguments here are ignored and not used. +// For passing `InspectorParentHandle`, use `CreateEnvironment()`. NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, StartExecutionCallback cb, - std::unique_ptr inspector_parent_handle = {}); + std::unique_ptr ignored_donotuse_removeme = {}); NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, const char* main_script_source_utf8, - std::unique_ptr inspector_parent_handle = {}); + std::unique_ptr ignored_donotuse_removeme = {}); NODE_EXTERN void FreeEnvironment(Environment* env); // Set a callback that is called when process.exit() is called from JS, diff --git a/src/node_worker.cc b/src/node_worker.cc index afa4a9a52d0192..bd43a9cfbab97f 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -317,7 +317,8 @@ void Worker::Run() { std::move(argv_), std::move(exec_argv_), EnvironmentFlags::kNoFlags, - thread_id_)); + thread_id_, + std::move(inspector_parent_handle_))); if (is_stopped()) return; CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); @@ -335,12 +336,8 @@ void Worker::Run() { { CreateEnvMessagePort(env_.get()); Debug(this, "Created message port for worker %llu", thread_id_.id); - if (LoadEnvironment(env_.get(), - StartExecutionCallback{}, - std::move(inspector_parent_handle_)) - .IsEmpty()) { + if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty()) return; - } Debug(this, "Loaded environment for worker %llu", thread_id_.id); } diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 2f7ab06c14334f..903900ac9c94f1 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -125,7 +125,10 @@ class EnvironmentTestFixture : public NodeTestFixture { public: class Env { public: - Env(const v8::HandleScope& handle_scope, const Argv& argv) { + Env(const v8::HandleScope& handle_scope, + const Argv& argv, + node::EnvironmentFlags::Flags flags = + node::EnvironmentFlags::kDefaultFlags) { auto isolate = handle_scope.GetIsolate(); context_ = node::NewContext(isolate); CHECK(!context_.IsEmpty()); @@ -135,10 +138,13 @@ class EnvironmentTestFixture : public NodeTestFixture { &NodeTestFixture::current_loop, platform.get()); CHECK_NE(nullptr, isolate_data_); + std::vector args(*argv, *argv + 1); + std::vector exec_args(*argv, *argv + 1); environment_ = node::CreateEnvironment(isolate_data_, context_, - 1, *argv, - argv.nr_args(), *argv); + args, + exec_args, + flags); CHECK_NE(nullptr, environment_); } diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 33361c12280d9d..d0182ca8620fac 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -168,8 +168,9 @@ TEST_F(EnvironmentTest, AtExitRunsJS) { TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { const v8::HandleScope handle_scope(isolate_); const Argv argv; + // Only one of the Environments can have default flags and own the inspector. Env env1 {handle_scope, argv}; - Env env2 {handle_scope, argv}; + Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags}; AtExit(*env1, at_exit_callback1); AtExit(*env2, at_exit_callback2); @@ -334,7 +335,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { " id: 1,\n" " method: 'Runtime.evaluate',\n" " params: {\n" - " expression: 'global.variableFromParent = 42;'\n" + " expression: 'globalThis.variableFromParent = 42;'\n" " }\n" " })\n" " });\n" @@ -401,14 +402,14 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { { "dummy" }, {}, node::EnvironmentFlags::kNoFlags, - data->thread_id); + data->thread_id, + std::move(data->inspector_parent_handle)); CHECK_NOT_NULL(environment); v8::Local extracted_value = LoadEnvironment( environment, "while (!global.variableFromParent) {}\n" - "return global.variableFromParent;", - std::move(data->inspector_parent_handle)).ToLocalChecked(); + "return global.variableFromParent;").ToLocalChecked(); uv_run(&loop, UV_RUN_DEFAULT); CHECK(extracted_value->IsInt32()); diff --git a/test/parallel/test-inspector-inspect-brk-node.js b/test/parallel/test-inspector-inspect-brk-node.js new file mode 100644 index 00000000000000..caf17b956dcd19 --- /dev/null +++ b/test/parallel/test-inspector-inspect-brk-node.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); + +// Regression test for https://github.com/nodejs/node/issues/32648 + +common.skipIfInspectorDisabled(); + +const { NodeInstance } = require('../common/inspector-helper.js'); + +async function runTest() { + const child = new NodeInstance(['--inspect-brk-node=0', '-p', '42']); + const session = await child.connectInspectorSession(); + await session.send({ method: 'Runtime.enable' }); + await session.send({ method: 'Debugger.enable' }); + await session.send({ method: 'Runtime.runIfWaitingForDebugger' }); + await session.waitForNotification((notification) => { + // The main assertion here is that we do hit the loader script first. + return notification.method === 'Debugger.scriptParsed' && + notification.params.url === 'internal/bootstrap/loaders.js'; + }); + + await session.waitForNotification('Debugger.paused'); + await session.send({ method: 'Debugger.resume' }); + await session.waitForNotification('Debugger.paused'); + await session.runToCompletion(); +} + +runTest().then(common.mustCall());