Crashlytics: report previous-run ANRs in didCrashOnPreviousExecution()#8250
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates didCrashOnPreviousExecution() to return true if the previous run ended with an Application Not Responding (ANR) event on Android API level 30 or above. Feedback on these changes highlights a potential race condition where asynchronous background initialization might cause didCrashOnPreviousExecution() to return a false negative during early app startup; performing the ANR check synchronously on the background worker thread is suggested as a remedy. Additionally, it is recommended to replace Thread.sleep() in the newly added unit tests with robust synchronization (such as awaiting a Future or using a CountDownLatch) to avoid test flakiness.
Replaced "e.g." and "i.e." with "for example" and "that is" throughout the documentation comments to improve readability and style. NO_RELEASE_CHANGE
mrober
left a comment
mrober
left a comment
There was a problem hiding this comment.
LGTM, just 1 question. But please merge this into feature/didCrashAnr branch because this is a change to the behaviour of a public api
|
|
||
| // Set during initialization's session finalization when the previous session ended with an ANR, | ||
| // so didCrashOnPreviousExecution() reports ANRs alongside JVM and native crashes. | ||
| private volatile boolean didPreviousExecutionEndWithAnr = false; |
There was a problem hiding this comment.
Does this need to be volatile?
There was a problem hiding this comment.
Written on background worker (finalizeSessions), read on caller/main thread via public didCrashOnPreviousExecution(), so volatile is needed for visibility
Replace the abbreviation "e.g." with "for example" in the Javadoc for the experimentInfoMap parameter to improve readability. NO_RELEASE_CHANGE
Detect ANRs during the existing non-blocking finalizeSessions flow and fold the result into didCrashOnPreviousExecution(), so it returns true for ANRs alongside JVM and native crashes (API 30+). Implements firebase#4201.
jrodiz
force-pushed
the
feature/jrc--Issue4201-2.API.to.check.previous.ANR
branch
from
472ec56 to
e6689fd
Compare
3 participants
Report previous-run ANRs in didCrashOnPreviousExecution()
Implements #4201.
FirebaseCrashlytics.didCrashOnPreviousExecution()now returnstruewhen the previous run ended with an ANR, in addition to JVM and native crashes. ANR detection requires API 30+ (ApplicationExitInfo); on older devices the ANR contribution is alwaysfalse. No new public API.How
On the next launch, Crashlytics already runs
finalizeSessions()→writeApplicationExitInfoEventIfRelevant()on a background worker to attach any ANR from the previous session. This records that result in adidPreviousExecutionEndWithAnrflag (via the newSessionReportingCoordinator.didRelevantAnrOccur(...)) and ORs it intodidCrashOnPreviousExecution()— no main-thread blocking and no extra system query.Tests
Unit (Robolectric):
didRelevantAnrOccuracross four branches;finalizeSessionssets / leaves the flag; no-ANRonPreExecute→didCrashOnPreviousExecution()isfalse.Verified on a physical device (Galaxy S25 Ultra, API 36): clean launch →
false; after a real ANR (OS recordsreason=6 (ANR)), relaunch →true. Reproducible end-to-end with the sample app at jrodiz/repro4201 (tap-to-ANR, then relaunch).