[Android] Integrate exit code handling into process_handler - #5455
Conversation
95569d8 to
b5f8edb
Compare
b5f8edb to
1d4dda2
Compare
8f902fa to
ac29a1f
Compare
ac29a1f to
34c7b78
Compare
Xeicker
left a comment
Xeicker
left a comment
There was a problem hiding this comment.
I don't quite get why it is an issue that the uptime is calculated incorrectly and how it is related to the returning codes issue.
Not that it shouldn't be fixed, just curious why to integrate it in the stacked PRs
| logs.warning(f'Activity Crashed with: {exit_info}') | ||
| return_code = exit_info.reason | ||
|
|
||
| elif android.constants.LOW_MEMORY_REGEX.search(output): |
There was a problem hiding this comment.
nit: I would use exit reason low memory. I'd consider that a bit more consistent
There was a problem hiding this comment.
Currently when memory on the device is low, CF performs a reset on the device and then continues the execution without marking the test case or the bad build check as a crash, returning a non 0 exit reason here would change that, so i don't think this one is feasable.
34c7b78 to
b6cfae0
Compare
Answering your 'uptime calculation question' When testing this changes i performed quite a ton of tests, and i saw that intermittently CF marked the test case as a failure because of this, even though i was seeing the test case execution directly on the avd's screen, and since the fix was a small pretty straight forward one i figured that it might fit into this small pr of changes in this stack |
a7adff5 to
15461cf
Compare
decoNR
left a comment
decoNR
left a comment
There was a problem hiding this comment.
Please resolve comments before merging.
| if android.constants.LOW_MEMORY_REGEX.search(output): | ||
| if android.util.activity_crashed(exit_info): | ||
| logs.warning(f'Activity Crashed with: {exit_info}') | ||
| return_code = exit_info.reason |
There was a problem hiding this comment.
Just to confirm, it seems that return_code can be overwritten, could this cause any issues?
There was a problem hiding this comment.
Other than bringing more clarity it shouldn't bring any issues, actually in this method we usually change the return_code to tell callers if the invoked process crashed, see example:
There was a problem hiding this comment.
What I meant is that the value assigned to return_code here can be overwritten later in the function.
I just wanted to flag this to confirm if it is not a problem.
There was a problem hiding this comment.
You are right, i see it can be overwritten to
return_code = 1
If there was a memory crash... this seems very delicate so I'll add a log there to note the dev that the return_code was modified midflight.
Thanks for catching that!
1b60828 to
0a7db63
Compare
4 participants
Bug: b/553141628
Overview
Since Android API level 30 (and apps targeting Android 11+), apps have scoped storage access.
am startprocess execution does not report exit codes on activity crashes, returning 0 even when native crashes occur.This PR integrates Android activity crash detection and reboot tracking into
process_handler.run_process(), ensuring process return codes accurately reflect native crashes (e.g. SIGSEGV, SIGABRT) or device reboots at test case executions. We also correct an intermittent issue in which the time since the last reboot vs time since activity launched was calculated incorrectly.Changes
src/clusterfuzz/_internal/system/process_handler.py: Integratedactivity_crashed_by_package()check and reboot validation inrun_process()for Android.src/clusterfuzz/_internal/tests/core/system/process_handler_test.py: Added unit tests for Android activity crash and clean exit handling inrun_process().Tests performed
When this happen now CF correctly determines that the app crashed due to runtime issues not related to memory errors.
PR stack
masterfeature/android-exit-code-constantsfeature/android-exit-code-corefeature/android-exit-code-process-handler👈feature/android-bad-build-check