-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
feat: Add crash recovery on Windows #5012
Conversation
13a71e1
to
ff6804f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 25 out of 36. Check the log or trigger a new build to see more.
ff6804f
to
47b6dc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
8426335
to
453daf5
Compare
453daf5
to
35f2367
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shallow review
CMakeLists.txt
Outdated
@@ -244,4 +244,6 @@ if (BUILD_BENCHMARKS) | |||
add_subdirectory(benchmarks) | |||
endif () | |||
|
|||
add_subdirectory(auxiliary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need the auxiliary
subdir, could crash-handler
just not live in the repo root dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I chose a subdirectory is that I expect other tools to live here in the future and for those to not pollute the top level directory. For example, if plugins get some CLI or there's some qt plugin, this could live there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed, the crashpad project should be a submodule, and it will live under tools
. Neither of these things need to be actioned until this has been approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 25 out of 29. Check the log or trigger a new build to see more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
Description
This PR adds effectively re-enables the possibility for Chatterino to restart after a crash on Windows, which was removed in #4351. To restart, we build a custom crash handler that captures some info about the crash and restarts the app (with its arguments) after Crashpad is done processing the crash. Crash recovery is only enabled in release mode. Because crash recovery and set-up of Crashpad happens before the settings are initialized, settings for crash recovery are saved in a separate file (
<crashdump-dir>/chatterino-recovery.bin
).I put the handler into a new directory called
auxiliary
as I couldn't find/come up with anything better. The handler is only enabled on Windows.The dialog that shows after a crash happened now contains more information about the crash (exception and where it was saved).
As for the change in
.clang-tidy
:cppcoreguidelines-avoid-c-arrays
is already enabled and does the same.