Skip to content
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

Alter In foreground calculation #466

Merged
merged 5 commits into from
Apr 23, 2019
Merged

Alter In foreground calculation #466

merged 5 commits into from
Apr 23, 2019

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Apr 11, 2019

Goal

We should update the collected value for app.inForeground to use the process importance rather than lifecycle callbacks. This is because if an ANR occurs on application startup, the activity may not enter the STARTED state due to its main thread being blocked, but the app process would still be considered in the foreground.

It's important to note that the inForeground value is still only updated in the NDK layer on initialisation, and when an activity lifecycle event occurs. If we wished to inform the NDK layer of each time the process importance altered, using something like ProcessLifecycleOwner would be a suitable API for doing so. However, this would require a migration to androidx and would add another dependency to our SDK, so has not been implemented in this PR.

Changeset

  • Updated SessionTracker to calculate inForeground on initialisation, so that the value is accurate if an ANR occurs immediately on startup
  • Created ForegroundDetector class which encapsulates the inForeground checks for process importance
  • Updated BlockedThreadDetector to use ForegroundDetector exclusively
  • Updated SessionTracker to use ForegroundDetector primarily and also the existing implementation as a secondary source of information

Tests

  • Triggered a crash (JVM and native) in the foreground and background, and confirmed that inForeground was the correct value for each.
  • Updated existing unit tests
  • Ran in foreground mazerunner feature

if (info != null) {
return info.importance <= ActivityManager.RunningAppProcessInfo.IMPORTANCE_VISIBLE;
} else { // prefer a potential false positive if process info not available
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why default rather than falling back to the old implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We made the decision when implementing ANR detection that we should prefer a false positive when we're unable to determine process importance. This is because otherwise the notifier might discard an actual error, since the old implementation can be inaccurate if the main thread is blocked and lifecycle callbacks cannot be invoked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. To rephrase, there are actually three states for whether the app is in the foreground, yes, no, and unknown. In the unknown case, ANR errors should still be reported, so this uses yes instead of unknown or an implementation based on main thread activity. Is that a fair assessment?

It would be clearer (and delineate responsibilities better) to actually return a value which indicates that the state is currently unknown and let the ANR reporter handle that appropriately, but barring that, the comment should state for future travelers why a potential false positive is preferable and how it affects reporting. "If this was false, some ANRs might get ignored" was not what I expected, and I can see how a future PR to tweak to how this value is computed could unintentionally break this.

@fractalwrench fractalwrench merged commit cc65be6 into next Apr 23, 2019
@fractalwrench fractalwrench deleted the in-foreground branch April 23, 2019 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants