[port] Fix memory leak due to skipped internal animators not unregistered (#2431) (#2444)

Author: jush

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 Mapbox welcomes participation and contributions from everyone.
 
+# 10.17.1
+## Bug fixes 🐞
+* Fix memory leak when camera animations are skipped.
+
 # 10.17.0 April 02, 2024
 ## Bug fixes 🐞
 * Fix map being pixelated on some devices when `ContextMode.SHARED` is used (e.g. in AndroidAuto extension).

plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImpl.kt

@@ -413,23 +413,24 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin, MapCameraPlugin {
 
         private fun finishAnimation(animation: Animator, finishStatus: AnimationFinishStatus) {
           (animation as? CameraAnimator<*>)?.apply {
-            if (skipped) {
-              return
-            }
-            runningAnimatorsQueue.remove(animation)
             if (debugMode) {
               val logText = when (finishStatus) {
                 AnimationFinishStatus.CANCELED -> "was canceled."
                 AnimationFinishStatus.ENDED -> "ended."
               }
-              logI(TAG, "Animation ${type.name}(${hashCode()}) $logText")
+              logI(TAG, "Animation ${type.name}(${hashCode()})${if (skipped) " skipped" else ""} $logText")
             }
+            // Even if skipped we need to unregister it
             if (isInternal) {
+              unregisterAnimators(this, cancelAnimators = false)
               if (debugMode) {
-                logI(TAG, "Internal Animator ${type.name}(${hashCode()}) was unregistered")
+                logI(TAG, "Internal Animator ${type.name}(${hashCode()}) was unregistered (${animators.size})")
               }
-              unregisterAnimators(this, cancelAnimators = false)
             }
+            if (skipped) {
+              return
+            }
+            runningAnimatorsQueue.remove(animation)
             if (runningAnimatorsQueue.isEmpty()) {
               mapTransformDelegate.setUserAnimationInProgress(false)
             }
@@ -443,7 +444,7 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin, MapCameraPlugin {
               commitChanges()
             }
           } ?: throw MapboxCameraAnimationException(
-            "Could not start animation as it must be an instance of CameraAnimator and not null!"
+            "Could not finish animation as it must be an instance of CameraAnimator and not null!"
           )
         }
       })
@@ -507,6 +508,9 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin, MapCameraPlugin {
         }
       }
       animators.addAll(cameraAnimators.map { it as CameraAnimator<*> })
+      if (debugMode) {
+        logI(TAG, "Registered ${cameraAnimators.size} animators. Currently, ${animators.size} animators registered.")
+      }
     }
   }
 

plugin-animation/src/test/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImplTest.kt

@@ -102,6 +102,9 @@ class CameraAnimationsPluginImplTest {
     animators.forEach {
       verify { it.addInternalListener(any()) }
     }
+    animators.forEach {
+      assertTrue(cameraAnimationsPluginImpl.animators.contains(it))
+    }
   }
 
   @Test
@@ -117,6 +120,7 @@ class CameraAnimationsPluginImplTest {
         it.removeInternalListener()
       }
     }
+    assertTrue("Animators is not empty", cameraAnimationsPluginImpl.animators.isEmpty())
   }
 
   @Test
@@ -316,10 +320,14 @@ class CameraAnimationsPluginImplTest {
     every {
       mapCameraManagerDelegate.cameraState
     } answers { cameraPosition.toCameraState() }
-    every { mapCameraManagerDelegate.setCamera(any<CameraOptions>()) } answers {
+
+    every {
+      mapCameraManagerDelegate.setCamera(any<CameraOptions>())
+    } answers {
       cameraPosition = firstArg<CameraOptions>()
       cameraAnimationsPluginImpl.onCameraMove(cameraPosition.toCameraState())
     }
+
     val targetPitch = 5.0
     val cameraOptions = CameraOptions.Builder().pitch(targetPitch).build()
     val expectedValues = mutableSetOf(targetPitch)
@@ -372,6 +380,72 @@ class CameraAnimationsPluginImplTest {
     assertArrayEquals(expectedValues.toDoubleArray(), updatedValues.toDoubleArray(), EPS)
   }
 
+  @Test
+  fun `two same immediate animations, second is skipped`() {
+    var cameraPosition = CameraOptions.Builder().build()
+    every {
+      mapCameraManagerDelegate.cameraState
+    } answers {
+      cameraPosition.toCameraState()
+    }
+    every {
+      mapCameraManagerDelegate.setCamera(any<CameraOptions>())
+    } answers {
+      cameraPosition = firstArg<CameraOptions>()
+      cameraAnimationsPluginImpl.onCameraMove(cameraPosition.toCameraState())
+    }
+
+    val cameraAnimatorOptions = cameraAnimatorOptions(10.0) {
+      startValue(10.0)
+    }
+    val bearingFirst = CameraBearingAnimator(cameraAnimatorOptions, true) {
+      duration = 0
+    }
+    bearingFirst.addListener(
+      onStart = {
+        assertEquals(2, cameraAnimationsPluginImpl.animators.size)
+        assertEquals(false, (it as CameraAnimator<*>).skipped)
+      }
+    )
+
+    val bearingSecond = CameraBearingAnimator(cameraAnimatorOptions, true) {
+      duration = 0
+    }
+    bearingSecond.addListener(
+      onStart = {
+        assertEquals(1, cameraAnimationsPluginImpl.animators.size)
+        assertEquals(true, (it as CameraAnimator<*>).skipped)
+      }
+    )
+
+    cameraAnimationsPluginImpl.playAnimatorsTogether(bearingFirst, bearingSecond)
+    assertTrue("Animators is not empty", cameraAnimationsPluginImpl.animators.isEmpty())
+  }
+
+  @Test
+  fun `animation skipped if camera already has target value`() {
+    val cameraPosition = CameraOptions.Builder().bearing(10.0).build().toCameraState()
+    // Make sure current camera animations plugin has the right initial value
+    cameraAnimationsPluginImpl.onCameraMove(cameraPosition)
+
+    val cameraAnimatorOptions = cameraAnimatorOptions(10.0) {
+      startValue(10.0)
+    }
+
+    val bearingAnimator = CameraBearingAnimator(cameraAnimatorOptions, true) {
+      duration = 0
+    }
+    bearingAnimator.addListener(
+      onStart = {
+        assertEquals(1, cameraAnimationsPluginImpl.animators.size)
+        assertEquals(true, (it as CameraAnimator<*>).skipped)
+      }
+    )
+
+    cameraAnimationsPluginImpl.playAnimatorsTogether(bearingAnimator)
+    assertTrue("Animators is not empty", cameraAnimationsPluginImpl.animators.isEmpty())
+  }
+
   @Test
   fun testEaseToSequenceDurationZero() {
     var cameraPosition = CameraState(
@@ -405,9 +479,9 @@ class CameraAnimationsPluginImplTest {
 
     shadowOf(getMainLooper()).pause()
 
-    val handler = Handler(getMainLooper())
     cameraAnimationsPluginImpl.easeTo(cameraOptions1, mapAnimationOptions { duration(0) })
 
+    val handler = Handler(getMainLooper())
     handler.postDelayed(
       {
         cameraAnimationsPluginImpl.easeTo(