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.

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

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

Merged
merged 17 commits into from
Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions filenames.gni
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions patches/node/.patches
Expand Up @@ -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
51 changes: 51 additions & 0 deletions patches/node/enable_crashpad_linux_node_processes.patch
@@ -0,0 +1,51 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: VerteDinde <keeleymhammond@gmail.com>
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') &&
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking on it, but what's the path forward for this patch? Is it something we're anticipating floating for the long-term?

Copy link
Member

Choose a reason for hiding this comment

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

I think there’s two approaches we could take in the long-term:

  1. Move this functionality out of our forked child_process.fork and into the new UtilityProcess API. If we did that, we could likely remove this patch, but I think the API needs additional feature work before that can be done.
  2. This becomes a long-running patch similar to our implementation of Breakpad in Node (see b10f243 )

I think the immediate plan is #2, but we’re looking at if #1 is feasible

+ (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.
54 changes: 45 additions & 9 deletions shell/app/node_main.cc
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
VerteDinde marked this conversation as resolved.
Show resolved Hide resolved
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);

Expand Down
39 changes: 39 additions & 0 deletions 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<v8::Object> exports,
v8::Local<v8::Value> unused,
v8::Local<v8::Context> 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)
1 change: 1 addition & 0 deletions shell/common/node_bindings.cc
Expand Up @@ -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) \
Expand Down
31 changes: 28 additions & 3 deletions spec/api-crash-reporter-spec.ts
Expand Up @@ -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();
Expand All @@ -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 () => {
Expand Down
6 changes: 6 additions & 0 deletions 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));
13 changes: 13 additions & 0 deletions spec/fixtures/apps/crash/main.js
Expand Up @@ -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);
Expand Down
9 changes: 9 additions & 0 deletions 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);