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 14 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..35a096dcff6f7072391bccd7952da7e8ea8980bb 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 || {}, '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)) {
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved
+ // 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, pid = -1;
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved
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 Posix, initialize crashpad after Nodejs init phase so that
VerteDinde marked this conversation as resolved.
Show resolved Hide resolved
// 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
13 changes: 10 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,15 @@ 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();
});

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));
4 changes: 4 additions & 0 deletions spec/fixtures/apps/crash/main.js
Expand Up @@ -54,6 +54,10 @@ 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 {
console.error(`Unrecognized crash type: '${crashType}'`);
process.exit(1);
Expand Down