From 2c723d7e84dfab5ad97fc0927426a0b2cd7a15b5 Mon Sep 17 00:00:00 2001 From: Robo Date: Wed, 30 Nov 2022 00:33:54 +0900 Subject: [PATCH] fix: enable crashpad for ELECTRON_RUN_AS_NODE processes (#36460) * wip: enable crashpad for node processes fix: add PID testing method wip: plumb fd into child_process in node * node::ProcessInitializationFlags::kNoDefaultSignalHandling * chore: clean up debug logging * chore: gate platform includes * test: clean up node process test * fix: pass pid in node_main * chore: cleanup impl * chore: fixup patch method definition * fix: expose bound methods to node_main * fix: remove bound methods * fix: crashpad connection for all ELECTRON_RUN_AS_NODE processes * chore: fix typo * chore: address review feedback * chore: delay crashpad initialization * chore: ensure options.env, code hygiene * chore: add argv test, check for process.env over {} * fix: fix test, return options.env immutability Co-authored-by: VerteDinde Co-authored-by: Jeremy Rose Co-authored-by: VerteDinde --- filenames.gni | 1 + patches/node/.patches | 1 + ...enable_crashpad_linux_node_processes.patch | 51 ++++++++++++++++++ shell/app/node_main.cc | 54 +++++++++++++++---- shell/common/api/crashpad_support.cc | 39 ++++++++++++++ shell/common/node_bindings.cc | 1 + spec/api-crash-reporter-spec.ts | 31 +++++++++-- spec/fixtures/apps/crash/fork.js | 6 +++ spec/fixtures/apps/crash/main.js | 13 +++++ spec/fixtures/apps/crash/node-extra-args.js | 9 ++++ 10 files changed, 194 insertions(+), 12 deletions(-) create mode 100644 patches/node/enable_crashpad_linux_node_processes.patch create mode 100644 shell/common/api/crashpad_support.cc create mode 100644 spec/fixtures/apps/crash/fork.js create mode 100644 spec/fixtures/apps/crash/node-extra-args.js diff --git a/filenames.gni b/filenames.gni index d68a04943b43..c1e90c0f563f 100644 --- a/filenames.gni +++ b/filenames.gni @@ -530,6 +530,7 @@ filenames = { "shell/browser/window_list_observer.h", "shell/browser/zoom_level_delegate.cc", "shell/browser/zoom_level_delegate.h", + "shell/common/api/crashpad_support.cc", "shell/common/api/electron_api_asar.cc", "shell/common/api/electron_api_clipboard.cc", "shell/common/api/electron_api_clipboard.h", diff --git a/patches/node/.patches b/patches/node/.patches index 8fc5d46fc62b..94f3804168c6 100644 --- a/patches/node/.patches +++ b/patches/node/.patches @@ -51,3 +51,4 @@ src_iwyu_in_cleanup_queue_cc.patch fix_expose_lookupandcompile_with_parameters.patch fix_prevent_changing_functiontemplateinfo_after_publish.patch chore_add_missing_algorithm_include.patch +enable_crashpad_linux_node_processes.patch diff --git a/patches/node/enable_crashpad_linux_node_processes.patch b/patches/node/enable_crashpad_linux_node_processes.patch new file mode 100644 index 000000000000..72b77ed517f5 --- /dev/null +++ b/patches/node/enable_crashpad_linux_node_processes.patch @@ -0,0 +1,51 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: VerteDinde +Date: Sun, 20 Nov 2022 21:45:20 -0800 +Subject: fix: enable crashpad for ELECTRON_RUN_AS_NODE linux processes + +Passes the crashpad handler PID and crashdump signal file descriptor +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..76151c75ef7ad420d2642c1cd11c8624b7d7e2a0 100644 +--- a/lib/child_process.js ++++ b/lib/child_process.js +@@ -59,6 +59,7 @@ let debug = require('internal/util/debuglog').debuglog( + ); + const { Buffer } = require('buffer'); + const { Pipe, constants: PipeConstants } = internalBinding('pipe_wrap'); ++const { getCrashdumpSignalFD, getCrashpadHandlerPID } = process._linkedBinding('electron_common_crashpad_support'); + + const { + AbortError, +@@ -159,7 +160,6 @@ function fork(modulePath, args = [], options) { + ArrayPrototypeSplice(execArgv, index - 1, 2); + } + } +- + args = [...execArgv, modulePath, ...args]; + + if (typeof options.stdio === 'string') { +@@ -613,6 +613,21 @@ function normalizeSpawnArguments(file, args, options) { + 'options.windowsVerbatimArguments'); + } + ++ if (process.platform === 'linux') { ++ if (ObjectPrototypeHasOwnProperty(options.env || process.env, 'ELECTRON_RUN_AS_NODE') && ++ (file === process.execPath)) { ++ // 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 ++ const fd = getCrashdumpSignalFD(); ++ const pid = getCrashpadHandlerPID(); ++ if (fd !== -1 && pid !== -1) { ++ options.env.CRASHDUMP_SIGNAL_FD = fd; ++ options.env.CRASHPAD_HANDLER_PID = pid; ++ } ++ } ++ } ++ + if (options.shell) { + const command = ArrayPrototypeJoin([file, ...args], ' '); + // Set the shell, switches, and commands. diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc index 998cf806c448..efadfd0d031b 100644 --- a/shell/app/node_main.cc +++ b/shell/app/node_main.cc @@ -34,6 +34,14 @@ #include "chrome/child/v8_crashpad_support_win.h" #endif +#if BUILDFLAG(IS_LINUX) +#include "base/environment.h" +#include "base/posix/global_descriptors.h" +#include "base/strings/string_number_conversions.h" +#include "components/crash/core/app/crash_switches.h" // nogncheck +#include "content/public/common/content_descriptors.h" +#endif + #if !IS_MAS_BUILD() #include "components/crash/core/app/crashpad.h" // nogncheck #include "shell/app/electron_crash_reporter_client.h" @@ -110,15 +118,20 @@ int NodeMain(int argc, char* argv[]) { v8_crashpad_support::SetUp(); #endif -// TODO(deepak1556): Enable crashpad support on linux for -// ELECTRON_RUN_AS_NODE processes. -// Refs https://github.com/electron/electron/issues/36030 -#if BUILDFLAG(IS_WIN) || (BUILDFLAG(IS_MAC) && !IS_MAS_BUILD()) - ElectronCrashReporterClient::Create(); - crash_reporter::InitializeCrashpad(false, "node"); - crash_keys::SetCrashKeysFromCommandLine( - *base::CommandLine::ForCurrentProcess()); - crash_keys::SetPlatformCrashKey(); +#if BUILDFLAG(IS_LINUX) + auto os_env = base::Environment::Create(); + 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 = -1, pid = -1; + DCHECK(base::StringToInt(fd_string, &fd)); + DCHECK(base::StringToInt(pid_string, &pid)); + base::GlobalDescriptors::GetInstance()->Set(kCrashDumpSignal, fd); + // Following API is unsafe in multi-threaded scenario, but at this point + // we are still single threaded. + os_env->UnSetVar("CRASHDUMP_SIGNAL_FD"); + os_env->UnSetVar("CRASHPAD_HANDLER_PID"); + } #endif int exit_code = 1; @@ -158,6 +171,29 @@ int NodeMain(int argc, char* argv[]) { return result->exit_code(); } +#if BUILDFLAG(IS_LINUX) + // 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(); + command_line->AppendSwitchASCII( + crash_reporter::switches::kCrashpadHandlerPid, pid_string); + ElectronCrashReporterClient::Create(); + crash_reporter::InitializeCrashpad(false, "node"); + crash_keys::SetCrashKeysFromCommandLine( + *base::CommandLine::ForCurrentProcess()); + crash_keys::SetPlatformCrashKey(); + // Ensure the flags and env variable does not propagate to userland. + command_line->RemoveSwitch(crash_reporter::switches::kCrashpadHandlerPid); + } +#elif BUILDFLAG(IS_WIN) || (BUILDFLAG(IS_MAC) && !IS_MAS_BUILD()) + ElectronCrashReporterClient::Create(); + crash_reporter::InitializeCrashpad(false, "node"); + crash_keys::SetCrashKeysFromCommandLine( + *base::CommandLine::ForCurrentProcess()); + crash_keys::SetPlatformCrashKey(); +#endif + gin::V8Initializer::LoadV8Snapshot( gin::V8SnapshotFileType::kWithAdditionalContext); diff --git a/shell/common/api/crashpad_support.cc b/shell/common/api/crashpad_support.cc new file mode 100644 index 000000000000..cf1cbdec72ca --- /dev/null +++ b/shell/common/api/crashpad_support.cc @@ -0,0 +1,39 @@ +// Copyright (c) 2022 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include "shell/common/gin_helper/dictionary.h" +#include "shell/common/node_includes.h" + +#if BUILDFLAG(IS_LINUX) +#include "components/crash/core/app/crashpad.h" // nogncheck +#endif + +namespace { + +#if BUILDFLAG(IS_LINUX) +int GetCrashdumpSignalFD() { + int fd; + return crash_reporter::GetHandlerSocket(&fd, nullptr) ? fd : -1; +} + +int GetCrashpadHandlerPID() { + int pid; + return crash_reporter::GetHandlerSocket(nullptr, &pid) ? pid : -1; +} +#endif + +void Initialize(v8::Local exports, + v8::Local unused, + v8::Local context, + void* priv) { + gin_helper::Dictionary dict(context->GetIsolate(), exports); +#if BUILDFLAG(IS_LINUX) + dict.SetMethod("getCrashdumpSignalFD", &GetCrashdumpSignalFD); + dict.SetMethod("getCrashpadHandlerPID", &GetCrashpadHandlerPID); +#endif +} + +} // namespace + +NODE_LINKED_MODULE_CONTEXT_AWARE(electron_common_crashpad_support, Initialize) diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 546e4658c1c6..1ee322ea5c2d 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -80,6 +80,7 @@ V(electron_common_asar) \ V(electron_common_clipboard) \ V(electron_common_command_line) \ + V(electron_common_crashpad_support) \ V(electron_common_environment) \ V(electron_common_features) \ V(electron_common_native_image) \ diff --git a/spec/api-crash-reporter-spec.ts b/spec/api-crash-reporter-spec.ts index 438fa6cad35b..c81d92d8ecdb 100644 --- a/spec/api-crash-reporter-spec.ts +++ b/spec/api-crash-reporter-spec.ts @@ -166,9 +166,7 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ expect(crash.mainProcessSpecific).to.equal('mps'); }); - // TODO(deepak1556): Re-enable this test once - // https://github.com/electron/electron/issues/36030 is resolved. - ifit(process.platform !== 'linux')('when a node process crashes', async () => { + ifit(!isLinuxOnArm)('when a node process crashes', async () => { const { port, waitForCrash } = await startServer(); runCrashApp('node', port); const crash = await waitForCrash(); @@ -177,6 +175,33 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ expect(crash.rendererSpecific).to.be.undefined(); }); + ifit(!isLinuxOnArm)('when a node process inside a node process crashes', async () => { + const { port, waitForCrash } = await startServer(); + runCrashApp('node-fork', port); + const crash = await waitForCrash(); + checkCrash('node', crash); + expect(crash.mainProcessSpecific).to.be.undefined(); + expect(crash.rendererSpecific).to.be.undefined(); + }); + + // Ensures that passing in crashpadHandlerPID flag for Linx child processes + // does not affect child proocess args. + ifit(process.platform === 'linux')('ensure linux child process args are not modified', async () => { + const { port, waitForCrash } = await startServer(); + let exitCode: number | null = null; + const appPath = path.join(__dirname, 'fixtures', 'apps', 'crash'); + const crashType = 'node-extra-args'; + const crashProcess = childProcess.spawn(process.execPath, [appPath, + `--crash-type=${crashType}`, + `--crash-reporter-url=http://127.0.0.1:${port}` + ], { stdio: 'inherit' }); + crashProcess.once('close', (code) => { + exitCode = code; + }); + await waitForCrash(); + expect(exitCode).to.equal(0); + }); + describe('with guid', () => { for (const processType of ['main', 'renderer', 'sandboxed-renderer']) { it(`when ${processType} crashes`, async () => { diff --git a/spec/fixtures/apps/crash/fork.js b/spec/fixtures/apps/crash/fork.js new file mode 100644 index 000000000000..888cfea5e7eb --- /dev/null +++ b/spec/fixtures/apps/crash/fork.js @@ -0,0 +1,6 @@ +const path = require('path'); +const childProcess = require('child_process'); + +const crashPath = path.join(__dirname, 'node-crash.js'); +const child = childProcess.fork(crashPath, { silent: true }); +child.on('exit', () => process.exit(0)); diff --git a/spec/fixtures/apps/crash/main.js b/spec/fixtures/apps/crash/main.js index 23f51362a78c..54037c678467 100644 --- a/spec/fixtures/apps/crash/main.js +++ b/spec/fixtures/apps/crash/main.js @@ -54,6 +54,19 @@ app.whenReady().then(() => { const crashPath = path.join(__dirname, 'node-crash.js'); const child = childProcess.fork(crashPath, { silent: true }); child.on('exit', () => process.exit(0)); + } else if (crashType === 'node-fork') { + const scriptPath = path.join(__dirname, 'fork.js'); + const child = childProcess.fork(scriptPath, { silent: true }); + child.on('exit', () => process.exit(0)); + } else if (crashType === 'node-extra-args') { + let exitcode = -1; + const crashPath = path.join(__dirname, 'node-extra-args.js'); + const child = childProcess.fork(crashPath, ['--enable-logging'], { silent: true }); + child.send('message'); + child.on('message', (forkedArgs) => { + if (JSON.stringify(forkedArgs) !== JSON.stringify(child.spawnargs)) { exitcode = 1; } else { exitcode = 0; } + process.exit(exitcode); + }); } else { console.error(`Unrecognized crash type: '${crashType}'`); process.exit(1); diff --git a/spec/fixtures/apps/crash/node-extra-args.js b/spec/fixtures/apps/crash/node-extra-args.js new file mode 100644 index 000000000000..3a1678585048 --- /dev/null +++ b/spec/fixtures/apps/crash/node-extra-args.js @@ -0,0 +1,9 @@ +const path = require('path'); +const childProcess = require('child_process'); + +process.on('message', function () { + process.send(process.argv); +}); + +// Allow time to send args, then crash the app. +setTimeout(() => process.nextTick(() => process.crash()), 10000);