fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

deepak1556 · 2022-11-28T11:23:30Z

Description of Change

Fixes #36030

Co-authored by: @VerteDinde , @nornagon

Release Notes

Notes: Enable crashpad for ELECTRON_RUN_AS_NODE processes.

fix: add PID testing method wip: plumb fd into child_process in node

patches/chromium/chore_fix_file_decriptor_extraction_for_node_processes.patch

patches/node/enable_crashpad_linux_node_processes.patch

shell/app/node_main.cc

nornagon · 2022-11-28T21:21:31Z

patches/node/enable_crashpad_linux_node_processes.patch

 +        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;


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

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

Addressed in ecd98e8

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.

shell/app/node_main.cc

nornagon · 2022-11-28T22:21:40Z

patches/node/enable_crashpad_linux_node_processes.patch

@@ -27,13 +27,14 @@ index 73c1dc769542865bdf7a2a03c16cef141d3f4b05..35a096dcff6f7072391bccd7952da7e8
   args = [...execArgv, modulePath, ...args];

   if (typeof options.stdio === 'string') {
-@@ -613,6 +613,21 @@ function normalizeSpawnArguments(file, args, options) {
+@@ -613,6 +613,22 @@ function normalizeSpawnArguments(file, args, options) {
                     'options.windowsVerbatimArguments');
   }

 +  if (process.platform === 'linux') {
 +    if (ObjectPrototypeHasOwnProperty(options.env || {}, 'ELECTRON_RUN_AS_NODE') &&


since options.env isn't necessarily set here, how does this work...? should this be options.env || process.env?

Good question, I think so? Made the change in 9463804

nornagon

test needs a little work!

spec/api-crash-reporter-spec.ts

spec/fixtures/apps/crash/node-extra-args.js

codebytere · 2022-11-29T13:08:47Z

patches/node/enable_crashpad_linux_node_processes.patch

+   }
+
+  if (process.platform === 'linux') {
+    if (ObjectPrototypeHasOwnProperty(options.env || process.env, 'ELECTRON_RUN_AS_NODE') &&


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

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

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.

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

release-clerk · 2022-11-29T15:33:57Z

Release Notes Persisted

Enable crashpad for ELECTRON_RUN_AS_NODE processes.

trop · 2022-11-29T15:34:12Z

I was unable to backport this PR to "22-x-y" cleanly;
you will need to perform this backport manually.

trop · 2022-11-29T16:06:55Z

@VerteDinde has manually backported this PR to "22-x-y", please check out #36483

* 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 <keeleymhammond@gmail.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org> Co-authored-by: VerteDinde <vertedinde@electronjs.org>

VerteDinde and others added 7 commits November 23, 2022 18:52

wip: enable crashpad for node processes

a177d0d

fix: add PID testing method wip: plumb fd into child_process in node

node::ProcessInitializationFlags::kNoDefaultSignalHandling

5d263b0

chore: clean up debug logging

bae57a8

chore: gate platform includes

2626021

test: clean up node process test

530fdfb

fix: pass pid in node_main

ad4f0d4

chore: cleanup impl

49e1519

deepak1556 requested a review from a team as a code owner November 28, 2022 11:23

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 28, 2022

deepak1556 added target/22-x-y PR should also be added to the "22-x-y" branch. semver/patch backwards-compatible bug fixes labels Nov 28, 2022

VerteDinde and others added 4 commits November 28, 2022 05:25

chore: fixup patch method definition

cd77422

fix: expose bound methods to node_main

ac8b23f

fix: remove bound methods

f644a62

fix: crashpad connection for all ELECTRON_RUN_AS_NODE processes

d00181c

deepak1556 changed the title ~~fix: enable crashpad for processes created with child_process.fork~~ fix: enable crashpad for ELECTRON_RUN_AS_NODE processes Nov 28, 2022

chore: fix typo

1665dd5

nornagon requested changes Nov 28, 2022

View reviewed changes

deepak1556 added 2 commits November 29, 2022 05:04

chore: address review feedback

21b0e1f

chore: delay crashpad initialization

be15662

VerteDinde requested a review from nornagon November 28, 2022 21:20

nornagon reviewed Nov 28, 2022

View reviewed changes

shell/app/node_main.cc Outdated Show resolved Hide resolved

nornagon reviewed Nov 28, 2022

View reviewed changes

shell/app/node_main.cc Outdated Show resolved Hide resolved

nornagon reviewed Nov 28, 2022

View reviewed changes

shell/app/node_main.cc Show resolved Hide resolved

chore: ensure options.env, code hygiene

ecd98e8

nornagon reviewed Nov 28, 2022

View reviewed changes

VerteDinde force-pushed the enable-crashpad-node-processes branch from b449baf to bfb32aa Compare November 28, 2022 23:47

VerteDinde requested a review from nornagon November 28, 2022 23:47

nornagon reviewed Nov 29, 2022

View reviewed changes

spec/api-crash-reporter-spec.ts Outdated Show resolved Hide resolved

spec/fixtures/apps/crash/node-extra-args.js Outdated Show resolved Hide resolved

chore: add argv test, check for process.env over {}

9463804

VerteDinde force-pushed the enable-crashpad-node-processes branch from bfb32aa to 9463804 Compare November 29, 2022 00:47

fix: fix test, return options.env immutability

d0e268c

VerteDinde force-pushed the enable-crashpad-node-processes branch from 9d3bedb to d0e268c Compare November 29, 2022 05:25

VerteDinde requested review from nornagon and VerteDinde November 29, 2022 06:54

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 29, 2022

codebytere approved these changes Nov 29, 2022

View reviewed changes

VerteDinde removed their request for review November 29, 2022 15:06

jkleinsc approved these changes Nov 29, 2022

View reviewed changes

jkleinsc merged commit 2c723d7 into main Nov 29, 2022

jkleinsc deleted the enable-crashpad-node-processes branch November 29, 2022 15:33

trop bot added needs-manual-bp/22-x-y and removed target/22-x-y PR should also be added to the "22-x-y" branch. labels Nov 29, 2022

VerteDinde mentioned this pull request Nov 29, 2022

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36483

Merged

trop bot added in-flight/22-x-y merged/22-x-y PR was merged to the "22-x-y" branch. and removed needs-manual-bp/22-x-y in-flight/22-x-y labels Nov 29, 2022

commit-lint bot mentioned this pull request Mar 10, 2023

[pull] main from electron:main Blaquez-home/electron#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

deepak1556 commented Nov 28, 2022 •

edited

nornagon Nov 28, 2022

VerteDinde Nov 28, 2022

VerteDinde Nov 28, 2022

VerteDinde Nov 29, 2022 •

edited

nornagon Nov 28, 2022

VerteDinde Nov 28, 2022 •

edited

nornagon left a comment

codebytere Nov 29, 2022

VerteDinde Nov 29, 2022

release-clerk bot commented Nov 29, 2022

trop bot commented Nov 29, 2022

trop bot commented Nov 29, 2022

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

fix: enable crashpad for ELECTRON_RUN_AS_NODE processes #36460

Conversation

deepak1556 commented Nov 28, 2022 • edited

Description of Change

Release Notes

nornagon Nov 28, 2022

Choose a reason for hiding this comment

VerteDinde Nov 28, 2022

Choose a reason for hiding this comment

VerteDinde Nov 28, 2022

Choose a reason for hiding this comment

VerteDinde Nov 29, 2022 • edited

Choose a reason for hiding this comment

nornagon Nov 28, 2022

Choose a reason for hiding this comment

VerteDinde Nov 28, 2022 • edited

Choose a reason for hiding this comment

nornagon left a comment

Choose a reason for hiding this comment

codebytere Nov 29, 2022

Choose a reason for hiding this comment

VerteDinde Nov 29, 2022

Choose a reason for hiding this comment

release-clerk bot commented Nov 29, 2022

trop bot commented Nov 29, 2022

trop bot commented Nov 29, 2022

deepak1556 commented Nov 28, 2022 •

edited

VerteDinde Nov 29, 2022 •

edited

VerteDinde Nov 28, 2022 •

edited