[JankStatMonitor] Add early flagging and log internal errors instead of handled errors (#905)

Author: FranAguilera

CHANGELOG.md

@@ -29,6 +29,7 @@
 
 **Fixed**
 
+- Fix memory leak in JankStatsMonitor when activity is destroyed without `onPause` being called
 - Fix inaccurate calculation at `Battery Level Change Per Min`
 
 ### iOS

platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/events/performance/JankStatsMonitor.kt

@@ -62,6 +62,7 @@ internal class JankStatsMonitor(
     JankStats.OnFrameListener {
     @VisibleForTesting
     internal var jankStats: JankStats? = null
+    private var currentWindow: Window? = null
     private var performanceMetricsStateHolder: PerformanceMetricsState.Holder? = null
 
     override fun start() {
@@ -101,7 +102,7 @@ internal class JankStatsMonitor(
             val errorMessage =
                 "Unexpected frame duration. durationInNano: $durationNano." +
                     " durationMillis: $durationMillis"
-            logger.handleInternalError(errorMessage)
+            logInternalError(errorMessage)
             return
         }
 
@@ -121,7 +122,7 @@ internal class JankStatsMonitor(
     }
 
     /**
-     * [LifecycleEventObserver] callback to determine when Application is first created
+     * [LifecycleEventObserver] callback to determine when the Application process is first resumed
      */
     override fun onStateChanged(
         source: LifecycleOwner,
@@ -130,11 +131,11 @@ internal class JankStatsMonitor(
         if (!runtime.isEnabled(RuntimeFeature.DROPPED_EVENTS_MONITORING)) {
             return
         }
-        if (event == Lifecycle.Event.ON_CREATE) {
+        if (event == Lifecycle.Event.ON_RESUME) {
             windowManager.findFirstValidActivity()?.let {
                 setJankStatsForCurrentWindow(it.window)
             }
-            // We are done detecting initial Application ON_CREATE, we don't need to listen anymore
+            // We are done detecting initial Application ON_RESUME, we don't need to listen anymore
             processLifecycleOwner.lifecycle.removeObserver(this)
         }
     }
@@ -147,7 +148,7 @@ internal class JankStatsMonitor(
     }
 
     override fun onActivityPaused(activity: Activity) {
-        stopCollection()
+        stopCollectionIfNeeded(activity)
     }
 
     override fun onActivityCreated(
@@ -173,7 +174,7 @@ internal class JankStatsMonitor(
     }
 
     override fun onActivityDestroyed(activity: Activity) {
-        // no-op
+        stopCollectionIfNeeded(activity)
     }
 
     fun trackScreenNameChanged(screenName: String) {
@@ -190,20 +191,25 @@ internal class JankStatsMonitor(
                 return
             }
             jankStats = JankStats.createAndTrack(window, this)
+            currentWindow = window
             performanceMetricsStateHolder = PerformanceMetricsState.getHolderForHierarchy(window.decorView.rootView)
             jankStats?.jankHeuristicMultiplier = runtime.getConfigValue(RuntimeConfig.JANK_FRAME_HEURISTICS_MULTIPLIER).toFloat()
         } catch (illegalStateException: IllegalStateException) {
-            logger.handleInternalError(
-                "Couldn't create JankStats instance",
-                illegalStateException,
-            )
+            logInternalError(errorMessage = "Couldn't create JankStats instance", throwable = illegalStateException)
+        }
+    }
+
+    private fun stopCollectionIfNeeded(activity: Activity) {
+        if (activity.window == currentWindow) {
+            stopCollection()
         }
     }
 
     private fun stopCollection() {
         jankStats?.isTrackingEnabled = false
         performanceMetricsStateHolder?.state?.removeState(SCREEN_NAME_KEY)
         jankStats = null
+        currentWindow = null
         performanceMetricsStateHolder = null
     }
 
@@ -256,6 +262,20 @@ internal class JankStatsMonitor(
             }
         }
 
+    private fun logInternalError(
+        errorMessage: String,
+        throwable: Throwable? = null,
+    ) {
+        logger.logInternal(
+            type = LogType.INTERNALSDK,
+            level = LogLevel.ERROR,
+            arrayFields = ArrayFields.EMPTY,
+            throwable = throwable,
+        ) {
+            errorMessage
+        }
+    }
+
     /**
      * The different type of Janks according to Play Vitals
      */

platform/jvm/capture/src/test/kotlin/io/bitdrift/capture/events/performance/JankStatsMonitorTest.kt

@@ -23,6 +23,7 @@ import com.nhaarman.mockitokotlin2.mock
 import com.nhaarman.mockitokotlin2.verify
 import com.nhaarman.mockitokotlin2.whenever
 import io.bitdrift.capture.IInternalLogger
+import io.bitdrift.capture.LogLevel
 import io.bitdrift.capture.LogType
 import io.bitdrift.capture.Mocks
 import io.bitdrift.capture.common.IWindowManager
@@ -52,8 +53,8 @@ class JankStatsMonitorTest {
     private val windowManager: IWindowManager = mock()
     private val mainThreadHandler = Mocks.sameThreadHandler
 
-    private val errorMessageCaptor = argumentCaptor<String>()
     private val illegalStateExceptionCaptor = argumentCaptor<IllegalStateException>()
+    private val logMessageCaptor = argumentCaptor<() -> String>()
 
     private lateinit var application: Application
     private lateinit var activity: Activity
@@ -267,11 +268,15 @@ class JankStatsMonitorTest {
 
         jankStatsMonitor.onActivityResumed(mockedActivity)
 
-        verify(logger).handleInternalError(
-            errorMessageCaptor.capture(),
-            illegalStateExceptionCaptor.capture(),
+        verify(logger).logInternal(
+            type = eq(LogType.INTERNALSDK),
+            level = eq(LogLevel.ERROR),
+            arrayFields = eq(ArrayFields.EMPTY),
+            throwable = illegalStateExceptionCaptor.capture(),
+            blocking = eq(false),
+            message = logMessageCaptor.capture(),
         )
-        assertThat(errorMessageCaptor.lastValue).isEqualTo("Couldn't create JankStats instance")
+        assertThat(logMessageCaptor.firstValue()).isEqualTo("Couldn't create JankStats instance")
         assertThat(illegalStateExceptionCaptor.lastValue).isInstanceOf(IllegalStateException::class.java)
         assertThat(
             illegalStateExceptionCaptor.lastValue.message,
@@ -315,6 +320,27 @@ class JankStatsMonitorTest {
         )
     }
 
+    @Test
+    fun onActivityDestroyed_afterOnStateChangedAttachesJankStats_shouldClearJankStatsWithoutLeak() {
+        // 1. SDK starts, onStateChanged ON_CREATE and ON_RESUME triggers findFirstValidActivity
+        //    which returns our activity and attaches JankStats to its window
+        jankStatsMonitor.onStateChanged(processLifecycleOwner, Lifecycle.Event.ON_CREATE)
+        jankStatsMonitor.onStateChanged(processLifecycleOwner, Lifecycle.Event.ON_RESUME)
+
+        assertThat(jankStatsMonitor.jankStats).isNotNull()
+
+        // 2. Activity is destroyed WITHOUT going through onActivityPaused
+        //    (simulates finish() from onCreate, or activity was already paused when SDK started)
+        jankStatsMonitor.onActivityDestroyed(activity)
+
+        // 3. Prior Leak: jankStats wasn't null before
+        assertThat(jankStatsMonitor.jankStats).isNull()
+
+        jankStatsMonitor.onActivityResumed(activity)
+
+        assertThat(jankStatsMonitor.jankStats).isNotNull
+    }
+
     private fun triggerOnFrame(
         isJankyFrame: Boolean,
         durationInMilli: Long,
@@ -349,19 +375,14 @@ class JankStatsMonitorTest {
     }
 
     private fun assertWrongDuration(expectedMessage: String) {
-        verify(logger, never()).logInternal(
-            any(),
-            any(),
-            any(),
-            any(),
-            anyOrNull(),
-            any(),
-            any(),
-        )
-        verify(logger).handleInternalError(
-            errorMessageCaptor.capture(),
-            eq(null),
+        verify(logger).logInternal(
+            type = eq(LogType.INTERNALSDK),
+            level = eq(LogLevel.ERROR),
+            arrayFields = eq(ArrayFields.EMPTY),
+            throwable = anyOrNull(),
+            blocking = eq(false),
+            message = logMessageCaptor.capture(),
         )
-        assertThat(errorMessageCaptor.lastValue).isEqualTo(expectedMessage)
+        assertThat(logMessageCaptor.firstValue()).isEqualTo(expectedMessage)
     }
 }