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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# Changelog

## TBD
## 4.X.X (TBD)

### Enhancements

* Alter In foreground calculation
[#466](https://github.com/bugsnag/bugsnag-android/pull/466)

### Bug fixes

Expand Down
4 changes: 2 additions & 2 deletions sdk/src/androidTest/java/com/bugsnag/android/AppDataTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public void testDurationInForeground() {

@Test
public void testInForeground() {
assertFalse((Boolean) appData.get("inForeground"));
assertTrue((Boolean) appData.get("inForeground"));
}

@Test
Expand All @@ -85,7 +85,7 @@ public void testJsonSerialisation() throws JSONException {
assertNotNull(appDataJson.get("buildUUID"));
assertNotNull(appDataJson.get("duration"));
assertNotNull(appDataJson.get("durationInForeground"));
assertFalse(appDataJson.getBoolean("inForeground"));
assertTrue(appDataJson.getBoolean("inForeground"));
}

@Test
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

Expand Down Expand Up @@ -97,7 +98,7 @@ public void testIncrementCounts() throws Exception {

@Test
public void testBasicInForeground() throws Exception {
assertFalse(sessionTracker.isInForeground());
Assert.assertTrue(sessionTracker.isInForeground());
assertNull(sessionTracker.getCurrentSession());
assertNull(sessionTracker.getContextActivity());

Expand All @@ -114,11 +115,6 @@ public void testBasicInForeground() throws Exception {
sessionTracker.updateForegroundTracker("other", false, System.currentTimeMillis());
assertTrue(sessionTracker.isInForeground());
assertEquals(ACTIVITY_NAME, sessionTracker.getContextActivity());

sessionTracker.updateForegroundTracker(ACTIVITY_NAME, false, System.currentTimeMillis());
assertFalse(sessionTracker.isInForeground());
assertEquals(firstSession, sessionTracker.getCurrentSession());
assertNull(sessionTracker.getContextActivity());
}

@Test
Expand All @@ -138,9 +134,6 @@ public void testInForegroundDuration() throws Exception {

sessionTracker.updateForegroundTracker(ACTIVITY_NAME, false, now);
assertEquals(200, sessionTracker.getDurationInForegroundMs(now + 200));

sessionTracker.updateForegroundTracker(ACTIVITY_NAME, false, now);
assertEquals(0, sessionTracker.getDurationInForegroundMs(now + 300));
}

@Test
Expand Down
61 changes: 6 additions & 55 deletions sdk/src/main/java/com/bugsnag/android/BlockedThreadDetector.java
Original file line number Diff line number Diff line change
@@ -1,15 +1,9 @@
package com.bugsnag.android;

import android.app.ActivityManager;
import android.os.Build;
import android.os.Handler;
import android.os.HandlerThread;
import android.os.Looper;
import android.os.Process;
import android.os.SystemClock;
import android.support.annotation.Nullable;

import java.util.List;

/**
* Detects whether a given thread is blocked by continuously posting a {@link Runnable} to it
Expand Down Expand Up @@ -38,22 +32,22 @@ interface Delegate {
final Handler watchdogHandler;
private final HandlerThread watchdogHandlerThread;
final Delegate delegate;
final ActivityManager activityManager;
final ForegroundDetector foregroundDetector;

volatile long lastUpdateMs;
volatile boolean isAlreadyBlocked = false;

BlockedThreadDetector(long blockedThresholdMs,
Looper looper,
ActivityManager activityManager,
ForegroundDetector foregroundDetector,
Delegate delegate) {
this(blockedThresholdMs, MIN_CHECK_INTERVAL_MS, looper, activityManager, delegate);
this(blockedThresholdMs, MIN_CHECK_INTERVAL_MS, looper, foregroundDetector, delegate);
}

BlockedThreadDetector(long blockedThresholdMs,
long checkIntervalMs,
Looper looper,
ActivityManager activityManager,
ForegroundDetector foregroundDetector,
Delegate delegate) {
if ((blockedThresholdMs <= 0 || checkIntervalMs <= 0
|| looper == null || delegate == null)) {
Expand All @@ -64,7 +58,7 @@ interface Delegate {
this.looper = looper;
this.delegate = delegate;
this.uiHandler = new Handler(looper);
this.activityManager = activityManager;
this.foregroundDetector = foregroundDetector;

watchdogHandlerThread = new HandlerThread("bugsnag-anr-watchdog");
watchdogHandlerThread.start();
Expand Down Expand Up @@ -104,7 +98,7 @@ long calculateNextCheckIn() {

void checkIfThreadBlocked() {
long delta = SystemClock.uptimeMillis() - lastUpdateMs;
boolean inForeground = isInForeground();
boolean inForeground = foregroundDetector.isInForeground();

if (inForeground && delta > blockedThresholdMs) {
if (!isAlreadyBlocked) {
Expand All @@ -115,47 +109,4 @@ void checkIfThreadBlocked() {
isAlreadyBlocked = false;
}
}

private boolean isInForeground() {
ActivityManager.RunningAppProcessInfo info = getProcessInfo();

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

private ActivityManager.RunningAppProcessInfo getProcessInfo() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) {
ActivityManager.RunningAppProcessInfo info =
new ActivityManager.RunningAppProcessInfo();
ActivityManager.getMyMemoryState(info);
return info;
} else {
return getProcessInfoPreApi16();
}
}

@Nullable
private ActivityManager.RunningAppProcessInfo getProcessInfoPreApi16() {
List<ActivityManager.RunningAppProcessInfo> appProcesses;

try {
appProcesses = activityManager.getRunningAppProcesses();
} catch (SecurityException exc) {
return null;
}

if (appProcesses != null) {
int pid = Process.myPid();

for (ActivityManager.RunningAppProcessInfo appProcess : appProcesses) {
if (pid == appProcess.pid) {
return appProcess;
}
}
}
return null;
}
}
6 changes: 2 additions & 4 deletions sdk/src/main/java/com/bugsnag/android/Client.java
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,9 @@ public void onThreadBlocked(Thread thread) {
}
};

ActivityManager activityManager =
(ActivityManager) appContext.getSystemService(Context.ACTIVITY_SERVICE);

ForegroundDetector foregroundDetector = new ForegroundDetector(appContext);
BlockedThreadDetector detector =
new BlockedThreadDetector(thresholdMs, looper, activityManager, delegate);
new BlockedThreadDetector(thresholdMs, looper, foregroundDetector, delegate);
detector.start(); // begin monitoring for ANRs
}

Expand Down
74 changes: 74 additions & 0 deletions sdk/src/main/java/com/bugsnag/android/ForegroundDetector.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.bugsnag.android;

import android.app.ActivityManager;
import android.content.Context;
import android.os.Build;
import android.os.Process;
import android.support.annotation.Nullable;

import java.util.List;

class ForegroundDetector {

private final ActivityManager activityManager;

ForegroundDetector(Context context) {
this.activityManager =
(ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE);
}

/**
* Determines whether or not the application is in the foreground, by using the process'
* importance as a proxy.
* <p/>
* In the unlikely event that information about the process cannot be retrieved, this method
* will return true. This is deemed preferable as ANRs are only reported when the application
* is in the foreground, and we would rather deliver false-positives than miss true ANRs in
* this case. We also need to report 'inForeground' as a boolean value in API calls, and
* need to keep the definition of the value consistent throughout the application.
*
* @return whether the application is in the foreground or not
*/
boolean isInForeground() {
ActivityManager.RunningAppProcessInfo info = getProcessInfo();

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.

}
}

private ActivityManager.RunningAppProcessInfo getProcessInfo() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) {
ActivityManager.RunningAppProcessInfo info =
new ActivityManager.RunningAppProcessInfo();
ActivityManager.getMyMemoryState(info);
return info;
} else {
return getProcessInfoPreApi16();
}
}

@Nullable
private ActivityManager.RunningAppProcessInfo getProcessInfoPreApi16() {
List<ActivityManager.RunningAppProcessInfo> appProcesses;

try {
appProcesses = activityManager.getRunningAppProcesses();
} catch (SecurityException exc) {
return null;
}

if (appProcesses != null) {
int pid = Process.myPid();

for (ActivityManager.RunningAppProcessInfo appProcess : appProcesses) {
if (pid == appProcess.pid) {
return appProcess;
}
}
}
return null;
}
}
11 changes: 9 additions & 2 deletions sdk/src/main/java/com/bugsnag/android/SessionTracker.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class SessionTracker extends Observable implements Application.ActivityLifecycle
private AtomicLong lastEnteredForegroundMs = new AtomicLong(0);
private AtomicReference<Session> currentSession = new AtomicReference<>();
private Semaphore flushingRequest = new Semaphore(1);
private final ForegroundDetector foregroundDetector;

SessionTracker(Configuration configuration, Client client, SessionStore sessionStore) {
this(configuration, client, DEFAULT_TIMEOUT_MS, sessionStore);
Expand All @@ -55,6 +56,8 @@ class SessionTracker extends Observable implements Application.ActivityLifecycle
this.client = client;
this.timeoutMs = timeoutMs;
this.sessionStore = sessionStore;
this.foregroundDetector = new ForegroundDetector(client.appContext);
notifyNdkInForeground();
}

/**
Expand Down Expand Up @@ -390,13 +393,17 @@ void updateForegroundTracker(String activityName, boolean activityStarting, long
}
}
setChanged();
notifyNdkInForeground();
}

private void notifyNdkInForeground() {
notifyObservers(new NativeInterface.Message(
NativeInterface.MessageType.UPDATE_IN_FOREGROUND,
Arrays.asList(!foregroundActivities.isEmpty(), getContextActivity())));
Arrays.asList(isInForeground(), getContextActivity())));
}

boolean isInForeground() {
return !foregroundActivities.isEmpty();
return foregroundDetector.isInForeground();
}

//FUTURE:SM This shouldnt be here
Expand Down