Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

Merged
merged 17 commits into from
Nov 29, 2022
Merged

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a177d0d
wip: enable crashpad for node processes
VerteDinde Nov 16, 2022
5d263b0
node::ProcessInitializationFlags::kNoDefaultSignalHandling
nornagon Nov 23, 2022
bae57a8
chore: clean up debug logging
VerteDinde Nov 24, 2022
2626021
chore: gate platform includes
VerteDinde Nov 25, 2022
530fdfb
test: clean up node process test
VerteDinde Nov 25, 2022
ad4f0d4
fix: pass pid in node_main
VerteDinde Nov 28, 2022
49e1519
chore: cleanup impl
deepak1556 Nov 28, 2022
cd77422
chore: fixup patch method definition
VerteDinde Nov 28, 2022
ac8b23f
fix: expose bound methods to node_main
VerteDinde Nov 28, 2022
f644a62
fix: remove bound methods
VerteDinde Nov 28, 2022
d00181c
fix: crashpad connection for all ELECTRON_RUN_AS_NODE processes
deepak1556 Nov 28, 2022
1665dd5
chore: fix typo
deepak1556 Nov 28, 2022
21b0e1f
chore: address review feedback
deepak1556 Nov 28, 2022
be15662
chore: delay crashpad initialization
deepak1556 Nov 28, 2022
ecd98e8
chore: ensure options.env, code hygiene
VerteDinde Nov 28, 2022
9463804
chore: add argv test, check for process.env over {}
VerteDinde Nov 28, 2022
d0e268c
fix: fix test, return options.env immutability
VerteDinde Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions patches/node/enable_crashpad_linux_node_processes.patch
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ to child processes spawned with `ELECTRON_RUN_AS_NODE` which is used
by the crashpad client to connect with the handler process.

diff --git a/lib/child_process.js b/lib/child_process.js
index 73c1dc769542865bdf7a2a03c16cef141d3f4b05..35a096dcff6f7072391bccd7952da7e8ea8980bb 100644
index 73c1dc769542865bdf7a2a03c16cef141d3f4b05..485f2236f8238e747d4422ad1b81648006a91028 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -59,6 +59,7 @@ let debug = require('internal/util/debuglog').debuglog(
Expand All @@ -27,13 +27,14 @@ index 73c1dc769542865bdf7a2a03c16cef141d3f4b05..35a096dcff6f7072391bccd7952da7e8
args = [...execArgv, modulePath, ...args];

if (typeof options.stdio === 'string') {
@@ -613,6 +613,21 @@ function normalizeSpawnArguments(file, args, options) {
@@ -613,6 +613,22 @@ function normalizeSpawnArguments(file, args, options) {
'options.windowsVerbatimArguments');
}

+ if (process.platform === 'linux') {
+ if (ObjectPrototypeHasOwnProperty(options.env || {}, 'ELECTRON_RUN_AS_NODE') &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since options.env isn't necessarily set here, how does this work...? should this be options.env || process.env?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I think so? Made the change in 9463804

+ (file === process.execPath)) {
+ options.env = Object.create(options.env || process.env);
+ // On Linux, pass the file descriptor which crashpad handler process
+ // uses to monitor the child process and PID of the handler process.
+ // https://source.chromium.org/chromium/chromium/src/+/110.0.5415.0:components/crash/core/app/crashpad_linux.cc;l=199-206
Expand Down
4 changes: 2 additions & 2 deletions shell/app/node_main.cc
View file
Open in desktop
Expand Up @@ -123,7 +123,7 @@ int NodeMain(int argc, char* argv[]) {
std::string fd_string, pid_string;
if (os_env->GetVar("CRASHDUMP_SIGNAL_FD", &fd_string) &&
os_env->GetVar("CRASHPAD_HANDLER_PID", &pid_string)) {
int fd, pid = -1;
int fd = -1, pid = -1;
DCHECK(base::StringToInt(fd_string, &fd));
DCHECK(base::StringToInt(pid_string, &pid));
base::GlobalDescriptors::GetInstance()->Set(kCrashDumpSignal, fd);
Expand Down Expand Up @@ -172,7 +172,7 @@ int NodeMain(int argc, char* argv[]) {
}

#if BUILDFLAG(IS_LINUX)
// On Posix, initialize crashpad after Nodejs init phase so that
// On Linux, initialize crashpad after Nodejs init phase so that
// crash and termination signal handlers can be set by the crashpad client.
if (!pid_string.empty()) {
auto* command_line = base::CommandLine::ForCurrentProcess();
Expand Down