Repository navigation
Improve the mechanism used to suspend threads #28
Description
Suspending the target process threads is currently an all-or-nothing operation that prevents minidump generation upon failure. The system we currently use is to iterate over the threads we've enumerated during initialization. This is an inherently racy operation as new threads can be created in the meantime and existing ones can quit. This is partially handled in that we tolerate being unable to suspend certain threads assuming that we stop at least a single one.
There should be ways to improve this but they're not clear-cut and probably needs some experimentation. One possibility is to send a SIGSTOP signal to the target process and wait for it to settle down before ptrace()-ing it. SIGSTOP is a big hammer that needs to be used with caution so I don't know what kind of issues we might encounter, but it could make the process more reliable by getting rid of some races (or at least leaving the kernel to handle them).
Additionally we might consider dumping threads we couldn't suspend. The data we'll get might be garbage but I don't really mind, we get garbage all the time in crash reports, it could also be useful. Let's say it would certainly be better than nothing. The reason why we never tried to do it with Breakpad is simple enough: handling errors is a chore! But here it's a lot easier. We could factorize the code for writing a single thread in thread_list_stream::write() and ignore individual failures (or even better, record them!).
Activity
cpeterso commentedon May 24, 2022
This work is related to Firefox Android's "no crashing thread identified" bug where many crash reports are missing thread stack traces:
https://bugzilla.mozilla.org/show_bug.cgi?id=1644486
Send SIGSTOP to linux/android processes before doing other procfs/ptrace
Send SIGSTOP to linux/android processes before doing other procfs/ptr…
afranchuk commentedon Apr 1, 2024
I've just tested this in the android emulator by running all test suites there.
I guess the easier way to do this might have been to install rust in the emulator, but instead I messed around with cargo configuration to set up the NDK linker and a runner script which pushed files with adb. There was an additional code change needed: adding support for an env variable to specify the
testhelper binary which is used to create child processes. The test suites previously usedcargo runorCARGO_BIN_EXE_testenv vars. I added a bit of code to use aTEST_HELPERenv var if set. There's also an extra step of runningcargo run --bin testto get it copied to the emulator sincecargo testwon't do that. I can upstream this if we think it useful, otherwise compiling and running in the emulator is very likely less setup/friction.All tests pass in the emulator! Which gives a bit more confidence that there's nothing weird around
SIGSTOPon android.Jake-Shadle commentedon Apr 2, 2024
I'd very much appreciate android tests being run in this repo's CI if you can get it working reliably
afranchuk commentedon Apr 5, 2024
@Jake-Shadle I see in the CI that
crossis already being used, but the test lines are commented out. Was that due to flakiness? I was considering possibly using https://github.com/ReactiveCircus/android-emulator-runner.Jake-Shadle commentedon Apr 5, 2024
Cross uses qemu for running test binaries, which doesn't support
ptraceat all, unfortunately.afranchuk commentedon Apr 5, 2024
Oh I see. I'll try out this other action, it seems to make it a bit easier than manually writing all the stuff to fetch and start the emulator.