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.

Sign up for GitHub

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

Jump to bottom

fix: save crash reports locally when uploadToServer: false on linux #24778

Merged
merged 2 commits into from Jul 30, 2020
Merged

fix: save crash reports locally when uploadToServer: false on linux #24778

merged 2 commits into from Jul 30, 2020

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Jul 29, 2020 •
edited

Description of Change

Fixes #24714

Checklist

Release Notes

Notes: save crash reports locally when uploadToServer: false on linux

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 29, 2020
@deepak1556 deepak1556 requested a review from nornagon July 29, 2020 10:55
@deepak1556 deepak1556 mentioned this pull request Jul 29, 2020
Closed
deepak1556
deepak1556 commented Jul 29, 2020
View reviewed changes
spec-main/api-crash-reporter-spec.ts
// dump dir on linux.
ifdescribe(process.platform !== 'linux')(`when ${crashingProcess} crashes`, () => {
const processList = process.platform === 'linux' ? ['main', 'renderer', 'sandboxed-renderer']
: ['main', 'renderer', 'sandboxed-renderer', 'node'];
Copy link
Member Author

Choose a reason for hiding this comment

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

fix for node process on linux is non-trivial, filed #24779 for follow-up.

@deepak1556 deepak1556 changed the title fix: generate dumps under crashDumps folder in linux fix: ave crash reports locally when uploadToServer: false on linux Jul 29, 2020
@deepak1556 deepak1556 changed the title fix: ave crash reports locally when uploadToServer: false on linux fix: save crash reports locally when uploadToServer: false on linux Jul 29, 2020
nornagon
nornagon approved these changes Jul 29, 2020
View reviewed changes
shell/app/electron_crash_reporter_client.cc
@@ -159,7 +167,7 @@ bool ElectronCrashReporterClient::GetCrashMetricsLocation(
#endif // OS_MACOSX || OS_LINUX

bool ElectronCrashReporterClient::IsRunningUnattended() {
return false;
return !collect_stats_consent_;
Copy link
Member

Choose a reason for hiding this comment

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

Did you find this change to be necessary to get crashes to dump to disk? I'm surprised it would be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup the crash dumping to disk for the browser process relies on this https://source.chromium.org/chromium/chromium/src/+/master:components/crash/core/app/breakpad_linux.cc;l=2068

Copy link
Member

Choose a reason for hiding this comment

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

aha, this comment explains why the redundancy: https://source.chromium.org/chromium/chromium/src/+/master:components/crash/core/app/crashpad.cc;l=251-255;drc=e4696af2b851af0b99601ec463ef11209036c2bd

spec-main/api-crash-reporter-spec.ts Outdated Show resolved Hide resolved
@deepak1556 @nornagon
Update spec-main/api-crash-reporter-spec.ts
3f26150 
Co-authored-by: Jeremy Rose <jeremya@chromium.org>
@deepak1556
Copy link
Member Author

Failing tests are unrelated, merging.

@deepak1556 deepak1556 merged commit 0102343 into master Jul 30, 2020
@release-clerk
Copy link

release-clerk bot commented Jul 30, 2020

Release Notes Persisted

save crash reports locally when uploadToServer: false on linux

@deepak1556 deepak1556 deleted the robo/fix_local_dumps_linux branch July 30, 2020 00:45
@trop
Copy link
Contributor

trop bot commented Jul 30, 2020

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

@trop trop bot mentioned this pull request Jul 30, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Jul 30, 2020

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

deepak1556 added a commit that referenced this pull request Jul 30, 2020
@deepak1556 @nornagon
fix: save crash reports locally when uploadToServer: false on linux (#…
552b55b 
…24778)

* fix: generate dumps under crashDumps folder in linux

* Update spec-main/api-crash-reporter-spec.ts

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
@deepak1556 deepak1556 mentioned this pull request Jul 30, 2020
Merged
7 tasks
@trop
Copy link
Contributor

trop bot commented Jul 30, 2020

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

@trop trop bot added the in-flight/9-x-y label Jul 30, 2020
@codebytere codebytere removed the new-pr 🌱 PR opened in the last 24 hours label Jul 31, 2020
deepak1556 added a commit that referenced this pull request Jul 31, 2020
@deepak1556 @nornagon
fix: save crash reports locally when uploadToServer: false on linux (#…
0107cab 
…24778) (#24788)

* fix: generate dumps under crashDumps folder in linux

* Update spec-main/api-crash-reporter-spec.ts

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

crash reports are not saved locally when uploadToServer: false on linux
3 participants
@deepak1556 @nornagon @codebytere