For mozilla-mobile#12930 - Unregister observers to prevent memory leaks

Mugurell · Oct 12, 2022 · 0914c90 · 0914c90
1 parent 39b45b3
commit 0914c90
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 2 deletions.
diff --git a/...cedtabs/src/main/java/mozilla/components/feature/syncedtabs/presenter/DefaultPresenter.kt b/...cedtabs/src/main/java/mozilla/components/feature/syncedtabs/presenter/DefaultPresenter.kt
@@ -81,7 +81,8 @@ internal class DefaultPresenter(
     }
 
     override fun stop() {
-        // no-op
+        accountManager.unregisterForSyncEvents(eventObserver)
+        accountManager.unregister(accountObserver)
     }
 
     companion object {

diff --git a/...abs/src/test/java/mozilla/components/feature/syncedtabs/presenter/DefaultPresenterTest.kt b/...abs/src/test/java/mozilla/components/feature/syncedtabs/presenter/DefaultPresenterTest.kt
@@ -236,4 +236,20 @@ class DefaultPresenterTest {
 
         verify(view).onError(ErrorType.SYNC_ENGINE_UNAVAILABLE)
     }
+
+    @Test
+    fun `GIVEN the presenter is started WHEN it is stopped THEN unregister the account and sync events observers`() {
+        val presenter = DefaultPresenter(
+            context,
+            controller,
+            accountManager,
+            view,
+            lifecycleOwner,
+        )
+
+        presenter.stop()
+
+        verify(accountManager).unregisterForSyncEvents(presenter.eventObserver)
+        verify(accountManager).unregister(presenter.accountObserver)
+    }
 }
diff --git a/...irefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt b/...irefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt
@@ -178,7 +178,9 @@ open class FxaAccountManager(
 
     @VisibleForTesting
     val accountEventObserverRegistry = ObserverRegistry<AccountEventsObserver>()
-    private val syncStatusObserverRegistry = ObserverRegistry<SyncStatusObserver>()
+
+    @VisibleForTesting
+    open val syncStatusObserverRegistry = ObserverRegistry<SyncStatusObserver>()
 
     // We always obtain a "profile" scope, as that's assumed to be needed for any application integration.
     // We obtain a sync scope only if this was requested by the application via SyncConfig.
@@ -487,6 +489,14 @@ open class FxaAccountManager(
         syncStatusObserverRegistry.register(observer, owner, autoPause)
     }
 
+    /**
+     * Unregister a [SyncStatusObserver] from being informed about "sync lifecycle" events.
+     * The method is safe to call even if the provided observer was not registered before.
+     */
+    fun unregisterForSyncEvents(observer: SyncStatusObserver) {
+        syncStatusObserverRegistry.unregister(observer)
+    }
+
     override fun close() {
         GlobalAccountManager.close()
         coroutineContext.cancel()

diff --git a/...ce/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt b/...ce/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt
@@ -5,6 +5,7 @@
 package mozilla.components.service.fxa
 
 import android.content.Context
+import androidx.lifecycle.LifecycleOwner
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -99,6 +100,9 @@ internal open class TestableFxaAccountManager(
     }
 ) : FxaAccountManager(context, config, DeviceConfig("test", DeviceType.UNKNOWN, capabilities), syncConfig, emptySet(), crashReporter, coroutineContext) {
     private val testableStorageWrapper = TestableStorageWrapper(this, accountEventObserverRegistry, serverConfig, block)
+
+    override var syncStatusObserverRegistry = ObserverRegistry<SyncStatusObserver>()
+
     override fun getStorageWrapper(): StorageWrapper {
         return testableStorageWrapper
     }
@@ -1734,6 +1738,45 @@ class FxaAccountManagerTest {
         verifyNoMoreInteractions(syncManager)
     }
 
+    @Test
+    fun `GIVEN a sync observer WHEN registering it THEN add it to the sync observer registry`() {
+        val fxaManager = TestableFxaAccountManager(
+            context = testContext,
+            config = mock(),
+            storage = mock(),
+            capabilities = setOf(DeviceCapability.SEND_TAB),
+            syncConfig = null,
+            coroutineContext = mock(),
+        )
+        fxaManager.syncStatusObserverRegistry = mock()
+        val observer: SyncStatusObserver = mock()
+        val lifecycleOwner: LifecycleOwner = mock()
+
+        fxaManager.registerForSyncEvents(observer, lifecycleOwner, false)
+
+        verify(fxaManager.syncStatusObserverRegistry).register(observer, lifecycleOwner, false)
+        verifyNoMoreInteractions(fxaManager.syncStatusObserverRegistry)
+    }
+
+    @Test
+    fun `GIVEN a sync observer WHEN unregistering it THEN remove it from the sync observer registry`() {
+        val fxaManager = TestableFxaAccountManager(
+            context = testContext,
+            config = mock(),
+            storage = mock(),
+            capabilities = setOf(DeviceCapability.SEND_TAB),
+            syncConfig = null,
+            coroutineContext = mock(),
+        )
+        fxaManager.syncStatusObserverRegistry = mock()
+        val observer: SyncStatusObserver = mock()
+
+        fxaManager.unregisterForSyncEvents(observer)
+
+        verify(fxaManager.syncStatusObserverRegistry).unregister(observer)
+        verifyNoMoreInteractions(fxaManager.syncStatusObserverRegistry)
+    }
+
     private suspend fun prepareHappyAuthenticationFlow(
         mockAccount: OAuthAccount,
         profile: Profile,

diff --git a/docs/changelog.md b/docs/changelog.md
@@ -11,6 +11,9 @@ permalink: /changelog/
 * [Gecko](https://github.com/mozilla-mobile/android-components/blob/main/buildSrc/src/main/java/Gecko.kt)
 * [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml)
 
+* **feature-syncedtabs**
+  * 🚒 Bug fixed [issue #12930](https://github.com/mozilla-mobile/android-components/issues/12930) Ensure `DefaultPresenter` will unregister it's `FxaAccountManager` observers when it's `lifecycleOwner` is stopped to prevent memory leaks. 
+
 * **feature-app-links**
   * 🚒 Bug fixed [issue #12804](https://github.com/mozilla-mobile/android-components/issues/12804) Speculative fix for a TransactionTooLargeException or RuntimeException when querying activities.