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: crash dump location on Linux #31668

Merged
merged 3 commits into from Nov 4, 2021
Merged

fix: crash dump location on Linux #31668

merged 3 commits into from Nov 4, 2021

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Nov 2, 2021

Description of Change

Closes #31643.
Closes #31565

Refs #30278 - now that Electron has switched to Crashpad by default on Linux DIR_CRASH_DUMPS should be the same across supported platforms.

Checklist

Release Notes

Notes: Fixed an issue where app.getPath('crashDumps') returned an incorrect path for Linux.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/16-x-y labels Nov 2, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 2, 2021
@deepak1556
Copy link
Member

LGTM with the directory alignment change but it does not solve the underlying issue reported which is incorrect path of DIR_USER_DATA

The problem arises from

std::string switch_value =
api::crash_reporter::GetClientId() + ",no_channel";

GetClientId touches DIR_USER_DATA for the first time via

bool GetClientIdPath(base::FilePath* path) {
if (base::PathService::Get(electron::DIR_CRASH_DUMPS, path)) {
*path = path->Append("client_id");
return true;
}
return false;
}

AppendExtraCommandLineSwitches will also get called for Zygote Process which will get launched before any JS runs which is were we first set the correct name for DIR_USER_DATA and having GetClientId called as part of the Zygote process leads to incorrect directory naming. Looking at the upstream code zygote process is not interested in switches::kEnableCrashReporter value, so we can just ignore this block for it to fix the root issue here.

@deepak1556
Copy link
Member

The fix should also be targeted to 15-x-y as the problematic path can be triggered when launching with --enable-crashpad

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 3, 2021
@deepak1556
Copy link
Member

@codebytere updated the PR addressing the root issue.

@jkleinsc jkleinsc merged commit 96a04c6 into main Nov 4, 2021
@jkleinsc jkleinsc deleted the crash-dumps-location-linux branch November 4, 2021 17:46
@release-clerk
Copy link

release-clerk bot commented Nov 4, 2021

Release Notes Persisted

Fixed an issue where app.getPath('crashDumps') returned an incorrect path for Linux.

@trop
Copy link
Contributor

trop bot commented Nov 4, 2021

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

@trop
Copy link
Contributor

trop bot commented Nov 4, 2021

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

@timfish
Copy link
Contributor

timfish commented Nov 7, 2021

The fix should also be targeted to 15-x-y as the problematic path can be triggered when launching with --enable-crashpad

I had already tested v15 with --enable-crashpad for @sentry/electron and didn't see any issues with app.getPath('crashDumps') 🤔

I only noticed it failing when trying v16 🤔

t57ser pushed a commit to t57ser/electron that referenced this pull request Jan 25, 2022
* fix: crash dump location on Linux

* fix: ignore client_id for Zygote process

* chore: update comment

Co-authored-by: deepak1556 <hop2deep@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: app.getPath('crashDumps') returns incorrect path on Linux [Bug]: Wrong value from getPath("userData")
4 participants