commit 4c489df8910ed5392ddd38eb39618419af4b6815
parent c47db865e2e826e5bbc7e46d1452c691910831d2
Author: mike a. <mavduevskiy@mozilla.com>
Date: Mon, 17 Nov 2025 23:23:05 +0000
Bug 1999853 - Prevent NimbusObserver from leaking the HomeActivity r=android-reviewers,twhite,sfamisa
This leak is something simple yet overlooked during the feature development. The fetching observer would only unregister from nimbus when the fetch was completed. But that is an if not a when.
On top of that, once the manager hits a timeout on the operation, we are no longer interested in the result of a SplashScreenOperation; its result only valid if reached under the timeout limit. So a better logic here is to cancel any observers once we have a result from an operation or a timer.
The leak was caused by the Nimbus object itself. We subscribed to it ih the manager, so it can't get destroyed. I believe having the lambda in the SplashScreenManager is fine because nothing points to the splashscreen, and its using lifecycle scope. If it doesn't subsribe to Nimbus, it should get cleared from the memory with the activity as well.
Differential Revision: https://phabricator.services.mozilla.com/D272545
Diffstat:
4 files changed, 86 insertions(+), 45 deletions(-)
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/splashscreen/SplashScreenManager.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/splashscreen/SplashScreenManager.kt
@@ -138,6 +138,8 @@ class SplashScreenManager(
}.onAwait { it }
}
+ splashScreenOperation.dispose()
+
withContext(this@SplashScreenManager.coroutineContext) {
isSplashScreenFinished = true
onSplashScreenFinished(result)
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/splashscreen/SplashScreenOperation.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/splashscreen/SplashScreenOperation.kt
@@ -31,6 +31,11 @@ interface SplashScreenOperation {
* Executes the splash screen operation.
*/
suspend fun run()
+
+ /**
+ * Releases any observers associated with the operation.
+ */
+ fun dispose()
}
/**
@@ -85,6 +90,9 @@ class FetchExperimentsOperation(
}
}
}
+ }
+
+ override fun dispose() {
fetchNimbusObserver?.let { nimbus.unregister(it) }
}
}
@@ -133,7 +141,9 @@ class ApplyExperimentsOperation(
nimbus.register(this)
}
}
+ }
+ override fun dispose() {
fetchNimbusObserver?.let { nimbus.unregister(it) }
applyNimbusObserver?.let { nimbus.unregister(it) }
}
diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/splashscreen/SplashScreenManagerTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/splashscreen/SplashScreenManagerTest.kt
@@ -77,6 +77,8 @@ class SplashScreenManagerTest {
override suspend fun run() {
delay(operationTime)
}
+
+ override fun dispose() {}
}
var result: SplashScreenManagerResult? = null
val splashScreenManager = buildSplashScreen(
@@ -105,6 +107,41 @@ class SplashScreenManagerTest {
override suspend fun run() {
delay(operationTime)
}
+
+ override fun dispose() {}
+ }
+ var result: SplashScreenManagerResult? = null
+ val splashScreenManager = buildSplashScreen(
+ splashScreenOperation = slowOperation,
+ splashScreenTimeout = splashScreenTimeout,
+ onSplashScreenFinished = { result = it },
+ )
+
+ splashScreenManager.showSplashScreen()
+
+ Assert.assertNull(result)
+ this.testScheduler.advanceUntilIdle()
+ Assert.assertTrue(result is SplashScreenManagerResult.TimeoutExceeded)
+ }
+
+ @Test
+ fun `WHEN splash screen times out THEN the operation is disposed`() = runTest {
+ val operationTime = 11_000L
+ val splashScreenTimeout = 2_500L
+ val slowOperation = object : SplashScreenOperation {
+ var disposed: Boolean = false
+ override val type: String
+ get() = "so slow much trouble"
+ override val dataFetched: Boolean
+ get() = false
+
+ override suspend fun run() {
+ delay(operationTime)
+ }
+
+ override fun dispose() {
+ disposed = true
+ }
}
var result: SplashScreenManagerResult? = null
val splashScreenManager = buildSplashScreen(
@@ -118,6 +155,41 @@ class SplashScreenManagerTest {
Assert.assertNull(result)
this.testScheduler.advanceUntilIdle()
Assert.assertTrue(result is SplashScreenManagerResult.TimeoutExceeded)
+ Assert.assertTrue(slowOperation.disposed)
+ }
+
+ @Test
+ fun `WHEN an operation finishes THEN the operation is disposed`() = runTest {
+ val operationTime = 400L
+ val splashScreenTimeout = 2_500L
+ val slowOperation = object : SplashScreenOperation {
+ var disposed: Boolean = false
+ override val type: String
+ get() = "so slow much trouble"
+ override val dataFetched: Boolean
+ get() = false
+
+ override suspend fun run() {
+ delay(operationTime)
+ }
+
+ override fun dispose() {
+ disposed = true
+ }
+ }
+ var result: SplashScreenManagerResult? = null
+ val splashScreenManager = buildSplashScreen(
+ splashScreenOperation = slowOperation,
+ splashScreenTimeout = splashScreenTimeout,
+ onSplashScreenFinished = { result = it },
+ )
+
+ splashScreenManager.showSplashScreen()
+
+ Assert.assertNull(result)
+ this.testScheduler.advanceUntilIdle()
+ Assert.assertTrue(result is SplashScreenManagerResult.OperationFinished)
+ Assert.assertTrue(slowOperation.disposed)
}
private fun TestScope.buildSplashScreen(
@@ -130,6 +202,8 @@ class SplashScreenManagerTest {
override suspend fun run() {
delay(2_400)
}
+
+ override fun dispose() {}
},
splashScreenTimeout: Long = 2_500,
showSplashScreen: (SplashScreen.KeepOnScreenCondition) -> Unit = { _ -> },
diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/splashscreen/SplashScreenOperationTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/splashscreen/SplashScreenOperationTest.kt
@@ -66,26 +66,6 @@ class SplashScreenOperationTest {
@OptIn(ExperimentalCoroutinesApi::class) // advanceUntilIdle
@Test
- fun `WHEN fetch operation is finished THEN nimbus callback is unregistered`() = runTest {
- val testNimbus = TestNimbusApi(this)
- val operation = FetchExperimentsOperation(
- buildStorage(),
- testNimbus,
- )
-
- launch { operation.run() }
- delay(100)
-
- assertTrue(testNimbus.observers.contains(operation.fetchNimbusObserver))
-
- launch { testNimbus.fakeExperimentsFetch(100) }
- advanceUntilIdle()
-
- assertFalse(testNimbus.observers.contains(operation.fetchNimbusObserver))
- }
-
- @OptIn(ExperimentalCoroutinesApi::class) // advanceUntilIdle
- @Test
fun `GIVEN nimbus data not fetched WHEN apply operation is called THEN we observe and record nimbus fetching the data and nimbus applying the data`() = runTest {
val testNimbus = TestNimbusApi(scope = this, applyDelay = 1000L)
val operation = ApplyExperimentsOperation(
@@ -125,31 +105,6 @@ class SplashScreenOperationTest {
assertTrue(operation.dataFetched)
}
- @OptIn(ExperimentalCoroutinesApi::class) // advanceUntilIdle
- @Test
- fun `WHEN apply operation is finished THEN nimbus callback is unregistered`() = runTest {
- val testNimbus = TestNimbusApi(this)
- val operation = ApplyExperimentsOperation(
- buildStorage(),
- testNimbus,
- )
-
- launch { operation.run() }
- delay(100)
-
- assertTrue(testNimbus.observers.contains(operation.fetchNimbusObserver))
-
- launch { testNimbus.fakeExperimentsFetch(100) }
- delay(200)
-
- assertTrue(testNimbus.observers.contains(operation.applyNimbusObserver))
-
- advanceUntilIdle()
-
- assertFalse(testNimbus.observers.contains(operation.fetchNimbusObserver))
- assertFalse(testNimbus.observers.contains(operation.applyNimbusObserver))
- }
-
class TestNimbusApi(
private val scope: CoroutineScope,
private val applyDelay: Long = 500L,
