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 3 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
Original file line number Diff line number Diff line change
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: 0 additions & 1 deletion patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,3 @@ 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

This file was deleted.

45 changes: 18 additions & 27 deletions patches/node/enable_crashpad_linux_node_processes.patch
Original file line number Diff line number Diff line change
Expand Up @@ -8,53 +8,44 @@ 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
index 73c1dc769542865bdf7a2a03c16cef141d3f4b05..35a096dcff6f7072391bccd7952da7e8ea8980bb 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -159,7 +159,6 @@ function fork(modulePath, args = [], options) {
@@ -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 +612,22 @@ function normalizeSpawnArguments(file, args, options) {
@@ -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)) {
+ // On Linux, pass the file descriptor which crashpad server process
+ // uses to monitor the child process.
+ const fd = process.getCrashdumpSignalFD();
+ if (fd !== -1) {
+ // 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;
+ // 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()}`);
+ options.env.CRASHPAD_HANDLER_PID = pid;
Copy link
Member

Choose a reason for hiding this comment

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

if options / options.env can be null, do we need to make sure we initialize those as objects here?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that's a good callout, I'll add that in 🙇‍♀️

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in ecd98e8

Copy link
Member

Choose a reason for hiding this comment

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

As a heads up, I reverted initializing options.env here in d0e268c after seeing multiple node test failures. It looks like node expects the options object to not be extendable here, so we can't create an options.env at this point if it doesn't already exist. We can add properties to options.env if it exists though, which our check should gate for.

+ }
+ }
+ }
+
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;
+ }
+
setupDebugEnv();

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

#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 @@ -38,7 +35,11 @@
#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()
Expand Down Expand Up @@ -98,19 +99,6 @@ 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;

int GetCrashdumpSignalFD() {
return crashdump_signal_fd;
}

int GetCrashpadHandlerPID() {
return crashpad_handler_pid;
}
#endif

} // namespace

namespace electron {
Expand All @@ -132,48 +120,18 @@ int NodeMain(int argc, char* argv[]) {

#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();
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");
}
#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];
}
}

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

int exit_code = 1;
Expand All @@ -196,22 +154,14 @@ 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,
node::ProcessInitializationFlags::kNoInitializeNodeV8Platform});

for (const std::string& error : result->errors())
Expand All @@ -221,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 Expand Up @@ -258,12 +231,6 @@ 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);
#endif

// Setup process.crashReporter in child node processes
gin_helper::Dictionary reporter = gin::Dictionary::CreateEmpty(isolate);
Expand Down