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 11 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 patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,4 @@ build_allow_electron_to_use_exec_script.patch
build_only_use_the_mas_build_config_in_the_required_components.patch
chore_introduce_blocking_api_for_electron.patch
chore_patch_out_partition_attribute_dcheck_for_webviews.patch
chore_fix_file_decriptor_extraction_for_node_processes.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: deepak1556 <hop2deep@gmail.com>
Date: Mon, 28 Nov 2022 20:07:17 +0900
Subject: chore: fix file decriptor extraction for node processes

For node processes, extract the crashdump signal file decriptor From
environment variable which is set by
//electron/patches/node/enable_crashpad_linux_node_processes.patch,
File decriptor is inherited by default via fork() performed by libuv.

diff --git a/components/crash/core/app/crashpad.cc b/components/crash/core/app/crashpad.cc
index 55ae5145e81a4719f33e052e00f606a577b0cf2e..0e9918602488482bfd07cb6232b5e9a8085fa65e 100644
--- a/components/crash/core/app/crashpad.cc
+++ b/components/crash/core/app/crashpad.cc
@@ -85,6 +85,7 @@ bool InitializeCrashpadImpl(bool initial_client,
initialized = true;

const bool browser_process = process_type.empty();
+ const bool node_process = (process_type == "node");

if (initial_client) {
#if BUILDFLAG(IS_APPLE)
@@ -112,7 +113,7 @@ bool InitializeCrashpadImpl(bool initial_client,
base::FilePath database_path;
if (!internal::PlatformCrashpadInitialization(
initial_client, browser_process, embedded_handler, user_data_dir,
- exe_path, initial_arguments, &database_path)) {
+ exe_path, initial_arguments, &database_path, node_process)) {
return false;
}

diff --git a/components/crash/core/app/crashpad.h b/components/crash/core/app/crashpad.h
index 813bbb4e3232c3e80a6f346535e3d16008316909..239449dac5fca4638973877fada2cbee92e7dc1f 100644
--- a/components/crash/core/app/crashpad.h
+++ b/components/crash/core/app/crashpad.h
@@ -271,7 +271,8 @@ bool PlatformCrashpadInitialization(
const std::string& user_data_dir,
const base::FilePath& exe_path,
const std::vector<std::string>& initial_arguments,
- base::FilePath* database_path);
+ base::FilePath* database_path,
+ bool node_process);

// Returns the current crash report database object, or null if it has not
// been initialized yet.
diff --git a/components/crash/core/app/crashpad_linux.cc b/components/crash/core/app/crashpad_linux.cc
index e55df93f17560a566e1dd2a63c560054edd772a5..97293a934f6c9a8034639cabbb20b35392c27a75 100644
--- a/components/crash/core/app/crashpad_linux.cc
+++ b/components/crash/core/app/crashpad_linux.cc
@@ -11,6 +11,7 @@

#include "base/base_switches.h"
#include "base/command_line.h"
+#include "base/environment.h"
#include "base/linux_util.h"
#include "base/logging.h"
#include "base/path_service.h"
@@ -77,7 +78,8 @@ bool PlatformCrashpadInitialization(
const std::string& user_data_dir,
const base::FilePath& exe_path,
const std::vector<std::string>& initial_arguments,
- base::FilePath* database_path) {
+ base::FilePath* database_path,
+ bool node_process) {
DCHECK_EQ(initial_client, browser_process);
DCHECK(initial_arguments.empty());

@@ -202,7 +204,17 @@ bool PlatformCrashpadInitialization(
CHECK(client.StartHandler(handler_path, *database_path, metrics_path, url,
annotations, arguments, false, false));
} else {
- int fd = base::GlobalDescriptors::GetInstance()->Get(kCrashDumpSignal);
+ int fd;
+ if (node_process) {
+ auto env = base::Environment::Create();
+ std::string fd_string;
+ if (env->GetVar("CRASHDUMP_SIGNAL_FD", &fd_string)) {
+ bool parsed = base::StringToInt(fd_string, &fd);
+ DCHECK(parsed);
+ }
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved
+ } else {
+ fd = base::GlobalDescriptors::GetInstance()->Get(kCrashDumpSignal);
+ }

pid_t pid = 0;
if (!sandbox::NamespaceSandbox::InNewUserNamespace()) {
diff --git a/components/crash/core/app/crashpad_mac.mm b/components/crash/core/app/crashpad_mac.mm
index 46935fcbf84cf1472214904cf96b03aa0e57be8f..b62da307431bebcd52d49d5bd720089860085575 100644
--- a/components/crash/core/app/crashpad_mac.mm
+++ b/components/crash/core/app/crashpad_mac.mm
@@ -135,7 +135,8 @@ bool PlatformCrashpadInitialization(
const std::string& user_data_dir,
const base::FilePath& exe_path,
const std::vector<std::string>& initial_arguments,
- base::FilePath* database_path) {
+ base::FilePath* database_path,
+ bool node_process) {
base::FilePath metrics_path; // Only valid in the browser process.
DCHECK(!embedded_handler); // This is not used on Mac.
DCHECK(exe_path.empty()); // This is not used on Mac.
diff --git a/components/crash/core/app/crashpad_win.cc b/components/crash/core/app/crashpad_win.cc
index 11ae602ccc58cb2728911b28d6637079d2dcb359..c38d79f44d5eef438d521e6c0528a31a2e38b4e8 100644
--- a/components/crash/core/app/crashpad_win.cc
+++ b/components/crash/core/app/crashpad_win.cc
@@ -67,7 +67,8 @@ bool PlatformCrashpadInitialization(
const std::string& user_data_dir,
const base::FilePath& exe_path,
const std::vector<std::string>& initial_arguments,
- base::FilePath* database_path) {
+ base::FilePath* database_path,
+ bool node_process) {
base::FilePath metrics_path; // Only valid in the browser process.

const char kPipeNameVar[] = "CHROME_CRASHPAD_PIPE_NAME";
1 change: 1 addition & 0 deletions patches/node/.patches
Original file line number Diff line number Diff line change
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
60 changes: 60 additions & 0 deletions patches/node/enable_crashpad_linux_node_processes.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
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..4c725a991b803c2bfde58b674df15b328102450b 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -159,7 +159,6 @@ function fork(modulePath, args = [], options) {
ArrayPrototypeSplice(execArgv, index - 1, 2);
}
}
-
args = [...execArgv, modulePath, ...args];

if (typeof options.stdio === 'string') {
@@ -613,6 +612,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)) {
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved
+ // On Linux, pass the file descriptor which crashpad server process
+ // uses to monitor the child process.
+ const fd = process.getCrashdumpSignalFD();
+ if (fd !== -1) {
+ options.env.CRASHDUMP_SIGNAL_FD = fd;
+ // On Linux, pass the PID of the crashpad handler process to the child process
+ // which is needed by the crashpad client running in the child process.
+ // https://source.chromium.org/chromium/chromium/src/+/110.0.5415.0:components/crash/core/app/crashpad_linux.cc;l=199-206
+ ArrayPrototypePush(args, `--crashpad-handler-pid=${process.getCrashpadHandlerPID()}`);
nornagon marked this conversation as resolved.
Show resolved Hide resolved
+ }
+ }
+ }
+
if (options.shell) {
const command = ArrayPrototypeJoin([file, ...args], ' ');
// Set the shell, switches, and commands.
diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js
index 6659f03e9aa45c35d355399597f533ad20232575..c1460d9f12607ecb26b602a15a516f013047f57d 100644
--- a/lib/internal/process/pre_execution.js
+++ b/lib/internal/process/pre_execution.js
@@ -59,6 +59,11 @@ function prepareMainThreadExecution(expandArgv1 = false,
setupCoverageHooks(process.env.NODE_V8_COVERAGE);
}

+ if (process.env.CRASHDUMP_SIGNAL_FD) {
+ // Make sure it's not accidentally inherited by child processes.
+ delete process.env.CRASHDUMP_SIGNAL_FD;
+ }
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved
+
setupDebugEnv();

// Print stack trace on `SIGINT` if option `--trace-sigint` presents.
77 changes: 73 additions & 4 deletions shell/app/node_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@

#include "base/base_switches.h"
#include "base/command_line.h"
#include "base/environment.h"
#include "base/feature_list.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_split.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/thread_pool/thread_pool_instance.h"
Expand All @@ -34,6 +37,10 @@
#include "chrome/child/v8_crashpad_support_win.h"
#endif

#if BUILDFLAG(IS_LINUX)
#include "components/crash/core/app/crash_switches.h" // nogncheck
#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 @@ -91,6 +98,19 @@ void SetCrashKeyStub(const std::string& key, const std::string& value) {}
void ClearCrashKeyStub(const std::string& key) {}
#endif

#if BUILDFLAG(IS_LINUX)
int crashpad_handler_pid = -1;
int crashdump_signal_fd = -1;
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved

int GetCrashdumpSignalFD() {
return crashdump_signal_fd;
}

int GetCrashpadHandlerPID() {
return crashpad_handler_pid;
}
#endif

} // namespace

namespace electron {
Expand All @@ -110,17 +130,52 @@ 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())
#if BUILDFLAG(IS_LINUX)
auto os_env = base::Environment::Create();
std::string fd_string;
if (os_env->GetVar("CRASHDUMP_SIGNAL_FD", &fd_string)) {
base::StringToInt(fd_string, &crashdump_signal_fd);
ElectronCrashReporterClient::Create();
crash_reporter::InitializeCrashpad(false, "node");
crash_keys::SetCrashKeysFromCommandLine(
*base::CommandLine::ForCurrentProcess());
crash_keys::SetPlatformCrashKey();
}
#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

#if BUILDFLAG(IS_LINUX)
// Shrink arguments without `--crashpad-handler-pid` to ensure that
// process.argv does not carry the flag.
int count = 0;
const std::string search_str =
std::string("--").append(crash_reporter::switches::kCrashpadHandlerPid);
for (int i = 0; i < argc; i++) {
if (base::StartsWith(argv[i], search_str)) {
auto results = base::SplitStringUsingSubstr(
argv[i], "=", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
base::StringToInt(results[1], &crashpad_handler_pid);
argv[i] = nullptr;
count += 1;
}
}

int new_argc = argc - count;
char** new_argv = new char*[new_argc];
for (int i = 0, j = 0; i < argc; i++) {
if (argv[i] != nullptr) {
new_argv[j++] = argv[i];
}
}
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved

base::CommandLine::ForCurrentProcess()->InitFromArgv(new_argc, new_argv);
#endif

int exit_code = 1;
{
// Feed gin::PerIsolateData with a task runner.
Expand All @@ -141,14 +196,22 @@ int NodeMain(int argc, char* argv[]) {
if (flags_exit_code != 0)
exit(flags_exit_code);

#if BUILDFLAG(IS_LINUX)
// Hack around with the argv pointer. Used for process.title = "blah".
argv = uv_setup_args(new_argc, new_argv);

std::vector<std::string> args(new_argv, new_argv + new_argc);
#else
// Hack around with the argv pointer. Used for process.title = "blah".
argv = uv_setup_args(argc, argv);

std::vector<std::string> args(argv, argv + argc);
#endif
std::unique_ptr<node::InitializationResult> result =
node::InitializeOncePerProcess(
args,
{node::ProcessInitializationFlags::kNoInitializeV8,
node::ProcessInitializationFlags::kNoDefaultSignalHandling,
VerteDinde marked this conversation as resolved.
Show resolved Hide resolved
node::ProcessInitializationFlags::kNoInitializeNodeV8Platform});

for (const std::string& error : result->errors())
Expand Down Expand Up @@ -195,6 +258,12 @@ int NodeMain(int argc, char* argv[]) {

gin_helper::Dictionary process(isolate, env->process_object());
process.SetMethod("crash", &ElectronBindings::Crash);
#if BUILDFLAG(IS_LINUX)
// These are needed so that process forked from this child process
// can connect to the crashpad handler process.
process.SetMethod("getCrashdumpSignalFD", &GetCrashdumpSignalFD);
process.SetMethod("getCrashpadHandlerPID", &GetCrashpadHandlerPID);
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved
#endif

// Setup process.crashReporter in child node processes
gin_helper::Dictionary reporter = gin::Dictionary::CreateEmpty(isolate);
Expand Down
20 changes: 20 additions & 0 deletions shell/common/api/electron_bindings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
#include "shell/common/thread_restrictions.h"
#include "third_party/blink/renderer/platform/heap/process_heap.h" // nogncheck

#if BUILDFLAG(IS_LINUX)
#include "components/crash/core/app/crashpad.h" // nogncheck
#endif

namespace electron {

ElectronBindings::ElectronBindings(uv_loop_t* loop) {
Expand Down Expand Up @@ -60,6 +64,10 @@ void ElectronBindings::BindProcess(v8::Isolate* isolate,
process->SetMethod("getCPUUsage",
base::BindRepeating(&ElectronBindings::GetCPUUsage,
base::Unretained(metrics)));
#if BUILDFLAG(IS_LINUX)
process->SetMethod("getCrashdumpSignalFD", &GetCrashdumpSignalFD);
process->SetMethod("getCrashpadHandlerPID", &GetCrashpadHandlerPID);
#endif

#if IS_MAS_BUILD()
process->SetReadOnly("mas", true);
Expand Down Expand Up @@ -122,6 +130,18 @@ void ElectronBindings::OnCallNextTick(uv_async_t* handle) {
self->pending_next_ticks_.clear();
}

#if BUILDFLAG(IS_LINUX)
int ElectronBindings::GetCrashdumpSignalFD() {
int fd;
return crash_reporter::GetHandlerSocket(&fd, nullptr) ? fd : -1;
}

int ElectronBindings::GetCrashpadHandlerPID() {
int pid;
return crash_reporter::GetHandlerSocket(nullptr, &pid) ? pid : -1;
}
#endif

// static
void ElectronBindings::Crash() {
volatile int* zero = nullptr;
Expand Down
4 changes: 4 additions & 0 deletions shell/common/api/electron_bindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ class ElectronBindings {
base::ProcessMetrics* metrics);

static void Crash();
#if BUILDFLAG(IS_LINUX)
static int GetCrashdumpSignalFD();
static int GetCrashpadHandlerPID();
#endif

static void DidReceiveMemoryDump(
v8::Global<v8::Context> context,
Expand Down
13 changes: 10 additions & 3 deletions spec/api-crash-reporter-spec.ts
Original file line number Diff line number Diff line change
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