Improve callback handling and test visibility (#4593)

Author: markushi

sentry-android-core/src/main/java/io/sentry/android/core/AppState.java

@@ -2,6 +2,7 @@
 
 import androidx.annotation.NonNull;
 import androidx.lifecycle.DefaultLifecycleObserver;
+import androidx.lifecycle.LifecycleObserver;
 import androidx.lifecycle.LifecycleOwner;
 import androidx.lifecycle.ProcessLifecycleOwner;
 import io.sentry.ILogger;
@@ -24,8 +25,8 @@
 public final class AppState implements Closeable {
   private static @NotNull AppState instance = new AppState();
   private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();
-  volatile LifecycleObserver lifecycleObserver;
-  MainLooperHandler handler = new MainLooperHandler();
+  private volatile LifecycleObserver lifecycleObserver;
+  private MainLooperHandler handler = new MainLooperHandler();
 
   private AppState() {}
 
@@ -35,6 +36,16 @@ private AppState() {}
 
   private volatile @Nullable Boolean inBackground = null;
 
+  @TestOnly
+  LifecycleObserver getLifecycleObserver() {
+    return lifecycleObserver;
+  }
+
+  @TestOnly
+  void setHandler(final @NotNull MainLooperHandler handler) {
+    this.handler = handler;
+  }
+
   @TestOnly
   void resetInstance() {
     instance = new AppState();
@@ -159,31 +170,32 @@ final class LifecycleObserver implements DefaultLifecycleObserver {
         new CopyOnWriteArrayList<AppStateListener>() {
           @Override
           public boolean add(AppStateListener appStateListener) {
+            final boolean addResult = super.add(appStateListener);
             // notify the listeners immediately to let them "catch up" with the current state
             // (mimics the behavior of androidx.lifecycle)
             if (Boolean.FALSE.equals(inBackground)) {
               appStateListener.onForeground();
             } else if (Boolean.TRUE.equals(inBackground)) {
               appStateListener.onBackground();
             }
-            return super.add(appStateListener);
+            return addResult;
           }
         };
 
     @Override
     public void onStart(@NonNull LifecycleOwner owner) {
+      setInBackground(false);
       for (AppStateListener listener : listeners) {
         listener.onForeground();
       }
-      setInBackground(false);
     }
 
     @Override
     public void onStop(@NonNull LifecycleOwner owner) {
+      setInBackground(true);
       for (AppStateListener listener : listeners) {
         listener.onBackground();
       }
-      setInBackground(true);
     }
   }
 

sentry-android-core/src/test/java/io/sentry/android/core/AppStateTest.kt

@@ -6,6 +6,7 @@ import io.sentry.android.core.internal.util.AndroidThreadChecker
 import java.util.concurrent.CountDownLatch
 import kotlin.test.AfterTest
 import kotlin.test.BeforeTest
+import kotlin.test.assertEquals
 import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
@@ -35,7 +36,7 @@ class AppStateTest {
     fun getSut(isMainThread: Boolean = true): AppState {
       val appState = AppState.getInstance()
       whenever(mockThreadChecker.isMainThread).thenReturn(isMainThread)
-      appState.handler = mockHandler
+      appState.setHandler(mockHandler)
       return appState
     }
 
@@ -261,6 +262,66 @@ class AppStateTest {
     assertTrue(sut.isInBackground()!!)
   }
 
+  @Test
+  fun `a listener can be unregistered within a callback`() {
+    val sut = fixture.getSut()
+
+    var onForegroundCalled = false
+    val listener =
+      object : AppStateListener {
+        override fun onForeground() {
+          sut.removeAppStateListener(this)
+          onForegroundCalled = true
+        }
+
+        override fun onBackground() {
+          // ignored
+        }
+      }
+
+    sut.registerLifecycleObserver(fixture.options)
+    val observer = sut.lifecycleObserver!!
+    observer.onStart(mock())
+
+    // if an observer is added
+    sut.addAppStateListener(listener)
+
+    // it should be notified
+    assertTrue(onForegroundCalled)
+
+    // and removed from the list of listeners if it unregisters itself within the callback
+    assertEquals(sut.lifecycleObserver?.listeners?.size, 0)
+  }
+
+  @Test
+  fun `state is correct within onStart and onStop callbacks`() {
+    val sut = fixture.getSut()
+
+    var onForegroundCalled = false
+    var onBackgroundCalled = false
+    val listener =
+      object : AppStateListener {
+        override fun onForeground() {
+          assertFalse(sut.isInBackground!!)
+          onForegroundCalled = true
+        }
+
+        override fun onBackground() {
+          assertTrue(sut.isInBackground!!)
+          onBackgroundCalled = true
+        }
+      }
+
+    sut.addAppStateListener(listener)
+
+    val observer = sut.lifecycleObserver!!
+    observer.onStart(mock())
+    observer.onStop(mock())
+
+    assertTrue(onForegroundCalled)
+    assertTrue(onBackgroundCalled)
+  }
+
   @Test
   fun `thread safety - concurrent access is handled`() {
     val listeners = (1..5).map { fixture.createListener() }