Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 1066341

Browse filesBrowse files
addaleaxaddaleax
committed
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: #32648 Refs: #30467 (comment) Backport-PR-URL: #35241 PR-URL: #32672 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent bd71cdf commit 1066341
Copy full SHA for 1066341

File tree

Expand file treeCollapse file tree

6 files changed

+85
-49
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+85
-49
lines changed
Open diff view settings
Collapse file

‎src/api/environment.cc‎

Copy file name to clipboardExpand all lines: src/api/environment.cc
+28-28Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,19 @@ void FreeIsolateData(IsolateData* isolate_data) {
317317
delete isolate_data;
318318
}
319319

320+
InspectorParentHandle::~InspectorParentHandle() {}
321+
322+
// Hide the internal handle class from the public API.
323+
#if HAVE_INSPECTOR
324+
struct InspectorParentHandleImpl : public InspectorParentHandle {
325+
std::unique_ptr<inspector::ParentInspectorHandle> impl;
326+
327+
explicit InspectorParentHandleImpl(
328+
std::unique_ptr<inspector::ParentInspectorHandle>&& impl)
329+
: impl(std::move(impl)) {}
330+
};
331+
#endif
332+
320333
Environment* CreateEnvironment(IsolateData* isolate_data,
321334
Local<Context> context,
322335
int argc,
@@ -335,7 +348,8 @@ Environment* CreateEnvironment(
335348
const std::vector<std::string>& args,
336349
const std::vector<std::string>& exec_args,
337350
EnvironmentFlags::Flags flags,
338-
ThreadId thread_id) {
351+
ThreadId thread_id,
352+
std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
339353
Isolate* isolate = context->GetIsolate();
340354
HandleScope handle_scope(isolate);
341355
Context::Scope context_scope(context);
@@ -352,6 +366,16 @@ Environment* CreateEnvironment(
352366
env->set_abort_on_uncaught_exception(false);
353367
}
354368

369+
#if HAVE_INSPECTOR
370+
if (inspector_parent_handle) {
371+
env->InitializeInspector(
372+
std::move(static_cast<InspectorParentHandleImpl*>(
373+
inspector_parent_handle.get())->impl));
374+
} else {
375+
env->InitializeInspector({});
376+
}
377+
#endif
378+
355379
if (env->RunBootstrapping().IsEmpty()) {
356380
FreeEnvironment(env);
357381
return nullptr;
@@ -381,19 +405,6 @@ void FreeEnvironment(Environment* env) {
381405
delete env;
382406
}
383407

384-
InspectorParentHandle::~InspectorParentHandle() {}
385-
386-
// Hide the internal handle class from the public API.
387-
#if HAVE_INSPECTOR
388-
struct InspectorParentHandleImpl : public InspectorParentHandle {
389-
std::unique_ptr<inspector::ParentInspectorHandle> impl;
390-
391-
explicit InspectorParentHandleImpl(
392-
std::unique_ptr<inspector::ParentInspectorHandle>&& impl)
393-
: impl(std::move(impl)) {}
394-
};
395-
#endif
396-
397408
NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
398409
Environment* env,
399410
ThreadId thread_id,
@@ -417,27 +428,17 @@ void LoadEnvironment(Environment* env) {
417428
MaybeLocal<Value> LoadEnvironment(
418429
Environment* env,
419430
StartExecutionCallback cb,
420-
std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
431+
std::unique_ptr<InspectorParentHandle> removeme) {
421432
env->InitializeLibuv(per_process::v8_is_profiling);
422433
env->InitializeDiagnostics();
423434

424-
#if HAVE_INSPECTOR
425-
if (inspector_parent_handle) {
426-
env->InitializeInspector(
427-
std::move(static_cast<InspectorParentHandleImpl*>(
428-
inspector_parent_handle.get())->impl));
429-
} else {
430-
env->InitializeInspector({});
431-
}
432-
#endif
433-
434435
return StartExecution(env, cb);
435436
}
436437

437438
MaybeLocal<Value> LoadEnvironment(
438439
Environment* env,
439440
const char* main_script_source_utf8,
440-
std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
441+
std::unique_ptr<InspectorParentHandle> removeme) {
441442
CHECK_NOT_NULL(main_script_source_utf8);
442443
return LoadEnvironment(
443444
env,
@@ -462,8 +463,7 @@ MaybeLocal<Value> LoadEnvironment(
462463
env->process_object(),
463464
env->native_module_require()};
464465
return ExecuteBootstrapper(env, name.c_str(), &params, &args);
465-
},
466-
std::move(inspector_parent_handle));
466+
});
467467
}
468468

469469
Environment* GetCurrentEnvironment(Local<Context> context) {
Collapse file

‎src/node.h‎

Copy file name to clipboardExpand all lines: src/node.h
+11-7Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,10 @@ enum Flags : uint64_t {
400400
};
401401
} // namespace EnvironmentFlags
402402

403+
struct InspectorParentHandle {
404+
virtual ~InspectorParentHandle();
405+
};
406+
403407
// TODO(addaleax): Maybe move per-Environment options parsing here.
404408
// Returns nullptr when the Environment cannot be created e.g. there are
405409
// pending JavaScript exceptions.
@@ -416,16 +420,14 @@ NODE_EXTERN Environment* CreateEnvironment(
416420
const std::vector<std::string>& args,
417421
const std::vector<std::string>& exec_args,
418422
EnvironmentFlags::Flags flags = EnvironmentFlags::kDefaultFlags,
419-
ThreadId thread_id = {} /* allocates a thread id automatically */);
423+
ThreadId thread_id = {} /* allocates a thread id automatically */,
424+
std::unique_ptr<InspectorParentHandle> inspector_parent_handle = {});
420425

421-
struct InspectorParentHandle {
422-
virtual ~InspectorParentHandle();
423-
};
424426
// Returns a handle that can be passed to `LoadEnvironment()`, making the
425427
// child Environment accessible to the inspector as if it were a Node.js Worker.
426428
// `child_thread_id` can be created using `AllocateEnvironmentThreadId()`
427429
// and then later passed on to `CreateEnvironment()` to create the child
428-
// Environment.
430+
// Environment, together with the inspector handle.
429431
// This method should not be called while the parent Environment is active
430432
// on another thread.
431433
NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
@@ -443,14 +445,16 @@ using StartExecutionCallback =
443445

444446
// TODO(addaleax): Deprecate this in favour of the MaybeLocal<> overload.
445447
NODE_EXTERN void LoadEnvironment(Environment* env);
448+
// The `InspectorParentHandle` arguments here are ignored and not used.
449+
// For passing `InspectorParentHandle`, use `CreateEnvironment()`.
446450
NODE_EXTERN v8::MaybeLocal<v8::Value> LoadEnvironment(
447451
Environment* env,
448452
StartExecutionCallback cb,
449-
std::unique_ptr<InspectorParentHandle> inspector_parent_handle = {});
453+
std::unique_ptr<InspectorParentHandle> ignored_donotuse_removeme = {});
450454
NODE_EXTERN v8::MaybeLocal<v8::Value> LoadEnvironment(
451455
Environment* env,
452456
const char* main_script_source_utf8,
453-
std::unique_ptr<InspectorParentHandle> inspector_parent_handle = {});
457+
std::unique_ptr<InspectorParentHandle> ignored_donotuse_removeme = {});
454458
NODE_EXTERN void FreeEnvironment(Environment* env);
455459

456460
// Set a callback that is called when process.exit() is called from JS,
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+3-6Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,8 @@ void Worker::Run() {
317317
std::move(argv_),
318318
std::move(exec_argv_),
319319
EnvironmentFlags::kNoFlags,
320-
thread_id_));
320+
thread_id_,
321+
std::move(inspector_parent_handle_)));
321322
if (is_stopped()) return;
322323
CHECK_NOT_NULL(env_);
323324
env_->set_env_vars(std::move(env_vars_));
@@ -335,12 +336,8 @@ void Worker::Run() {
335336
{
336337
CreateEnvMessagePort(env_.get());
337338
Debug(this, "Created message port for worker %llu", thread_id_.id);
338-
if (LoadEnvironment(env_.get(),
339-
StartExecutionCallback{},
340-
std::move(inspector_parent_handle_))
341-
.IsEmpty()) {
339+
if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty())
342340
return;
343-
}
344341

345342
Debug(this, "Loaded environment for worker %llu", thread_id_.id);
346343
}
Collapse file

‎test/cctest/node_test_fixture.h‎

Copy file name to clipboardExpand all lines: test/cctest/node_test_fixture.h
+9-3Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,10 @@ class EnvironmentTestFixture : public NodeTestFixture {
125125
public:
126126
class Env {
127127
public:
128-
Env(const v8::HandleScope& handle_scope, const Argv& argv) {
128+
Env(const v8::HandleScope& handle_scope,
129+
const Argv& argv,
130+
node::EnvironmentFlags::Flags flags =
131+
node::EnvironmentFlags::kDefaultFlags) {
129132
auto isolate = handle_scope.GetIsolate();
130133
context_ = node::NewContext(isolate);
131134
CHECK(!context_.IsEmpty());
@@ -135,10 +138,13 @@ class EnvironmentTestFixture : public NodeTestFixture {
135138
&NodeTestFixture::current_loop,
136139
platform.get());
137140
CHECK_NE(nullptr, isolate_data_);
141+
std::vector<std::string> args(*argv, *argv + 1);
142+
std::vector<std::string> exec_args(*argv, *argv + 1);
138143
environment_ = node::CreateEnvironment(isolate_data_,
139144
context_,
140-
1, *argv,
141-
argv.nr_args(), *argv);
145+
args,
146+
exec_args,
147+
flags);
142148
CHECK_NE(nullptr, environment_);
143149
}
144150

Collapse file

‎test/cctest/test_environment.cc‎

Copy file name to clipboardExpand all lines: test/cctest/test_environment.cc
+6-5Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,9 @@ TEST_F(EnvironmentTest, AtExitRunsJS) {
168168
TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
169169
const v8::HandleScope handle_scope(isolate_);
170170
const Argv argv;
171+
// Only one of the Environments can have default flags and own the inspector.
171172
Env env1 {handle_scope, argv};
172-
Env env2 {handle_scope, argv};
173+
Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags};
173174

174175
AtExit(*env1, at_exit_callback1);
175176
AtExit(*env2, at_exit_callback2);
@@ -334,7 +335,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) {
334335
" id: 1,\n"
335336
" method: 'Runtime.evaluate',\n"
336337
" params: {\n"
337-
" expression: 'global.variableFromParent = 42;'\n"
338+
" expression: 'globalThis.variableFromParent = 42;'\n"
338339
" }\n"
339340
" })\n"
340341
" });\n"
@@ -401,14 +402,14 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) {
401402
{ "dummy" },
402403
{},
403404
node::EnvironmentFlags::kNoFlags,
404-
data->thread_id);
405+
data->thread_id,
406+
std::move(data->inspector_parent_handle));
405407
CHECK_NOT_NULL(environment);
406408

407409
v8::Local<v8::Value> extracted_value = LoadEnvironment(
408410
environment,
409411
"while (!global.variableFromParent) {}\n"
410-
"return global.variableFromParent;",
411-
std::move(data->inspector_parent_handle)).ToLocalChecked();
412+
"return global.variableFromParent;").ToLocalChecked();
412413

413414
uv_run(&loop, UV_RUN_DEFAULT);
414415
CHECK(extracted_value->IsInt32());
Collapse file

‎test/parallel/test-inspector-inspect-brk-node.js‎

Copy file name to clipboard
+28Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
'use strict';
2+
const common = require('../common');
3+
4+
// Regression test for https://github.com/nodejs/node/issues/32648
5+
6+
common.skipIfInspectorDisabled();
7+
8+
const { NodeInstance } = require('../common/inspector-helper.js');
9+
10+
async function runTest() {
11+
const child = new NodeInstance(['--inspect-brk-node=0', '-p', '42']);
12+
const session = await child.connectInspectorSession();
13+
await session.send({ method: 'Runtime.enable' });
14+
await session.send({ method: 'Debugger.enable' });
15+
await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
16+
await session.waitForNotification((notification) => {
17+
// The main assertion here is that we do hit the loader script first.
18+
return notification.method === 'Debugger.scriptParsed' &&
19+
notification.params.url === 'internal/bootstrap/loaders.js';
20+
});
21+
22+
await session.waitForNotification('Debugger.paused');
23+
await session.send({ method: 'Debugger.resume' });
24+
await session.waitForNotification('Debugger.paused');
25+
await session.runToCompletion();
26+
}
27+
28+
runTest().then(common.mustCall());

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.