Skip to content

Commit 464ab31

Browse files
joyeecheungaduh95
joyeecheung
authored and
aduh95
committed
src: do not enable the inspector during snapshot building
V8 no longer allows attaching a debuger or setting a debug context ID during snapshot building. Avoid enabling the inspector in this case, and reject the inspector options when the user-land --build-snapshot is used. Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8679045 Signed-off-by: Joyee Cheung <[email protected]> PR-URL: #67154 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Richard Lau <[email protected]>
1 parent 823549f commit 464ab31

7 files changed

Lines changed: 58 additions & 22 deletions

File tree

‎doc/api/cli.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -721,6 +721,10 @@ an additional module during the snapshot building process is needed,
721721
please file a request in the [Node.js issue tracker][] and link to it in the
722722
[tracking issue for user-land snapshots][].
723723

724+
During snapshot building, a debugger cannot be attached to the process.
725+
`--build-snapshot` cannot be used together with `--inspect`, `--inspect-brk` or
726+
`--inspect-wait`.
727+
724728
### `--build-snapshot-config`
725729

726730
<!-- YAML

‎doc/node.1‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,9 @@ runtime, using either \fBv8.startupSnapshot.setDeserializeMainFunction()\fR or
447447
an additional module during the snapshot building process is needed,
448448
please file a request in the Node.js issue tracker and link to it in the
449449
tracking issue for user-land snapshots.
450+
During snapshot building, a debugger cannot be attached to the process.
451+
\fB--build-snapshot\fR cannot be used together with \fB--inspect\fR, \fB--inspect-brk\fR or
452+
\fB--inspect-wait\fR.
450453
.
451454
.It Fl -build-snapshot-config
452455
Specifies the path to a JSON configuration file which configures snapshot

‎src/inspector_agent.cc‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,11 @@ class NodeInspectorClient : public V8InspectorClient {
528528
is_main_ ? GetHumanReadableProcessName() : GetWorkerLabel(env);
529529
ContextInfo info(name);
530530
info.is_default = true;
531-
contextCreated(env->context(), info);
531+
// V8 does not allow a debugger to be attached while a startup or
532+
// a debug context ID to be set during snapshot building.
533+
if (!env->isolate_data()->is_building_snapshot()) {
534+
contextCreated(env->context(), info);
535+
}
532536
}
533537

534538
void runMessageLoopOnPause(int context_group_id) override {
@@ -923,6 +927,12 @@ bool Agent::StartIoThread() {
923927

924928
CHECK_NOT_NULL(client_);
925929

930+
if (parent_env_->isolate_data()->is_building_snapshot()) {
931+
fprintf(stderr,
932+
"The inspector cannot be activated while building a snapshot.\n");
933+
return false;
934+
}
935+
926936
io_ = InspectorIo::Start(client_->getThreadHandle(),
927937
path_,
928938
host_port_,

‎src/node_options.cc‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -203,23 +203,16 @@ void PerProcessOptions::CheckProcessTimeoutOptions(
203203
// make the timeout fire while the debugger is in control, so the two cannot
204204
// be combined. Runtime activation of the inspector is rejected separately.
205205
const DebugOptions& debug_options = per_isolate->per_env->debug_options();
206-
const char* debug_option = nullptr;
207-
if (debug_options.break_node_first_line) {
208-
debug_option = "--inspect-brk-node";
209-
} else if (debug_options.break_first_line) {
210-
debug_option = "--inspect-brk";
211-
} else if (debug_options.inspect_wait) {
212-
debug_option = "--inspect-wait";
213-
} else if (debug_options.inspector_enabled) {
214-
debug_option = "--inspect";
215-
} else if (debug_options.host_port.host() != "127.0.0.1" ||
216-
debug_options.host_port.port() !=
217-
DebugOptions::kDefaultInspectorPort) {
218-
// We can't catch the case where the value passed is the default value,
219-
// then the option just becomes a noop which is fine.
220-
debug_option = "--inspect-port";
221-
} else if (debug_options.inspect_publish_uid_string != "stderr,http") {
222-
debug_option = "--inspect-publish-uid";
206+
const char* debug_option = debug_options.inspector_option_name();
207+
if (debug_option == nullptr) {
208+
if (debug_options.host_port.host() != "127.0.0.1" ||
209+
debug_options.host_port.port() != DebugOptions::kDefaultInspectorPort) {
210+
// We can't catch the case where the value passed is the default value,
211+
// then the option just becomes a noop which is fine.
212+
debug_option = "--inspect-port";
213+
} else if (debug_options.inspect_publish_uid_string != "stderr,http") {
214+
debug_option = "--inspect-publish-uid";
215+
}
223216
}
224217
if (debug_option != nullptr) {
225218
errors->push_back(std::string("either --process-timeout or ") +
@@ -297,6 +290,16 @@ void PerIsolateOptions::CheckOptions(std::vector<std::string>* errors,
297290
HandleMaxOldSpaceSizePercentage(errors, &max_old_space_size_percentage);
298291
}
299292

293+
if (build_snapshot) {
294+
// V8 does not allow a debugger to be attached during snapshot building.
295+
const char* debug_option = per_env->debug_options().inspector_option_name();
296+
if (debug_option != nullptr) {
297+
errors->push_back(
298+
std::string("--build-snapshot cannot be used together with ") +
299+
debug_option);
300+
}
301+
}
302+
300303
per_env->CheckOptions(errors, argv);
301304
}
302305

‎src/node_options.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,14 @@ class DebugOptions : public Options {
112112
return break_first_line || break_node_first_line;
113113
}
114114

115+
const char* inspector_option_name() const {
116+
if (break_node_first_line) return "--inspect-brk-node";
117+
if (break_first_line) return "--inspect-brk";
118+
if (inspect_wait) return "--inspect-wait";
119+
if (inspector_enabled) return "--inspect";
120+
return nullptr;
121+
}
122+
115123
void CheckOptions(std::vector<std::string>* errors,
116124
std::vector<std::string>* argv) override;
117125
};

‎test/parallel/test-inspect-address-in-use.js‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,14 @@ function testChildProcess(getArgs, exitCode, options) {
4242

4343
tmpdir.refresh();
4444

45-
testChildProcess(
46-
(port) => [`--inspect=${port}`, '--build-snapshot', entry], 0,
47-
{ cwd: tmpdir.path });
45+
// V8 does not allow a debugger to be attached during snapshot building.
46+
spawnSyncAndExit(process.execPath, ['--inspect=0', '--build-snapshot', entry],
47+
{ cwd: tmpdir.path },
48+
{
49+
status: 9,
50+
signal: null,
51+
stderr: /--build-snapshot cannot be used together with --inspect/,
52+
});
4853

4954
testChildProcess(
5055
(port) => [`--inspect=${port}`, entry], 0);

‎test/parallel/test-snapshot-console.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@ const entry = fixtures.path('snapshot', 'console.js');
3030
console.log(child.stderr.toString());
3131
assert.strictEqual(child.status, 0);
3232
}
33-
assert.deepStrictEqual(Object.keys(console), JSON.parse(stdout));
33+
// console.createTask is not available during snapshot buildling.
34+
assert.deepStrictEqual(
35+
Object.keys(console).filter((key) => key !== 'createTask'),
36+
JSON.parse(stdout));
3437
const stats = fs.statSync(tmpdir.resolve('snapshot.blob'));
3538
assert(stats.isFile());
3639
}

0 commit comments

Comments
 (0)