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: avoid creating client_id file for empty DIR_CRASH_DUMPS #25296

Merged
merged 1 commit into from Sep 3, 2020

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Sep 3, 2020

Description of Change

Node child process don't set DIR_CRASH_DUMPS unless the undocumented BREAKPAD_DUMP_LOCATION is specified, in these situations client_id file will be generated under the working directory.

Refs microsoft/vscode#105743

Checklist

Release Notes

Notes: Fixed client_id file being generated in the working directory for Node.js child processes.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 3, 2020
nornagon
nornagon previously approved these changes Sep 3, 2020
@nornagon
Copy link
Member

nornagon commented Sep 3, 2020

hm... does this mean crashes in node child processes on linux won't come with a guid parameter?

Perhaps we should set BREAKPAD_DUMP_LOCATION for node child processes?

@nornagon nornagon dismissed their stale review September 3, 2020 18:01

Thought of some more questions

@deepak1556
Copy link
Member Author

deepak1556 commented Sep 3, 2020

Yeah thought about that, but I don't see a reasonable way to specify the value as base::PathService will be uninitialized in the node_main.cc which blocks us from extracting the DIR_USER_DATA. If we propagate the user data path via the process object then we can set the value here https://github.com/electron/electron/blob/master/patches/node/refactor_alter_child_process_fork_to_use_execute_script_with.patch or else we can just document BREAKPAD_DUMP_LOCATION for apps to specify.

@nornagon
Copy link
Member

nornagon commented Sep 3, 2020

I was imagining setting the env var in childProcess.fork() as in that patch. But I guess there are other ways that apps could launch a RUN_AS_NODE process...

@nornagon
Copy link
Member

nornagon commented Sep 3, 2020

I think the ultimate path forward here is to switch to crashpad on Linux, which we'll do eventually anyway.

@deepak1556
Copy link
Member Author

I think the ultimate path forward here is to switch to crashpad on Linux, which we'll do eventually anyway.

yep

FWIW crash reporting from node child process wasn't working at all on linux before the refactor, so I think we can leave this as such until the crashpad move is made.

@deepak1556 deepak1556 merged commit 03e60cc into master Sep 3, 2020
@release-clerk
Copy link

release-clerk bot commented Sep 3, 2020

Release Notes Persisted

Fixed client_id file being generated in the working directory for Node.js child processes.

@deepak1556 deepak1556 deleted the robo/fix_client_id branch September 3, 2020 18:52
@trop
Copy link
Contributor

trop bot commented Sep 3, 2020

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

@trop
Copy link
Contributor

trop bot commented Sep 3, 2020

I have automatically backported this PR to "10-x-y", please check out #25309

@trop
Copy link
Contributor

trop bot commented Sep 3, 2020

I have automatically backported this PR to "11-x-y", please check out #25310

@trop trop bot removed the target/11-x-y label Sep 3, 2020
@trop
Copy link
Contributor

trop bot commented Sep 3, 2020

@deepak1556 has manually backported this PR to "9-x-y", please check out #25316

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants