Fix the SIGQUIT handling system to deal with multiple ANRs properly. #1235
Conversation
There was a problem hiding this comment.
Please add a changelog entry.
Other nits mentioned below, but I'll leave the main functionality review for @fractalwrench as I'm not close-enough to the detail.
| struct sigaction handler; | ||
| sigemptyset(&handler.sa_mask); | ||
| handler.sa_sigaction = handle_sigquit; | ||
| handler.sa_flags = SA_SIGINFO; | ||
| if (sigaction(SIGQUIT, &handler, &original_sigquit_handler) != 0) { | ||
| if (sigaction(SIGQUIT, &handler, NULL) != 0) { |
There was a problem hiding this comment.
I think the fact that we're not restoring the original handler here should be supported by some comments to justify this for future readers.
There was a problem hiding this comment.
Added a comment here
There was a problem hiding this comment.
How will this work for environments such as Unity where ANR detection needs to be dynamically enabled/disabled via the Bugsnag.SetAutoNotify() api?
There was a problem hiding this comment.
When bugsnag is disabled, SIGQUIT is blocked and the default handler is never called. When bugsnag is enabled, our handler gets called. Either way, the default SIGQUIT handler never gets called in an Android app (and must never be called because it would terminate the app).
kstenerud
force-pushed
the
6477-fix-for-multiple-anrs
branch
2 times, most recently
from
80f44ec
to
cb0768b
Compare
kstenerud
force-pushed
the
6477-fix-for-multiple-anrs
branch
from
cb0768b
to
4c13f8d
Compare
| struct sigaction handler; | ||
| sigemptyset(&handler.sa_mask); | ||
| handler.sa_sigaction = handle_sigquit; | ||
| handler.sa_flags = SA_SIGINFO; | ||
| if (sigaction(SIGQUIT, &handler, &original_sigquit_handler) != 0) { | ||
| if (sigaction(SIGQUIT, &handler, NULL) != 0) { |
There was a problem hiding this comment.
How will this work for environments such as Unity where ANR detection needs to be dynamically enabled/disabled via the Bugsnag.SetAutoNotify() api?
Goal
Bugsnag wasn't handling things properly when multiple ANRs occur. This PR fixes it.
Design
The old handler would run once and exit, replacing the SIGQUIT handler with what was there before. This is the correct design for traditional signal handlers, but breaks in a Google ANR handler environment. Subsequent ANRs would trigger the default POSIX signal handler, which terminates the app.
Instead, we need the handler thread to run in a loop, blocking and unblocking SIGQUIT as necessary in order to coordinate with the Google ANR handler, and never setting the system to use the default handler.
Testing
These changes must be tested manually, which will happen on multiple devices at the beginning of the week. Do not merge until these tests are complete!