commit 33a83be7c3287322604749086e0e79d7ab0bf74f
parent 389d9db63cc81f88f3e391d928754ce437342c97
Author: Titouan Thibaud <tthibaud@mozilla.com>
Date: Wed, 19 Nov 2025 09:06:48 +0000
Bug 1989604 - Fix sync button loading forever on Tabs Tray r=android-reviewers,pollymce
WorkManagerSyncManager's workersLiveData might skip the RUNNING emission if WorkManagerSyncWorker.doWork's execution is really quick (in the case where the last sync was within the past 5 seconds and the sync operation is skipped). The previous code was relying on RUNNING to be received to start set isSyncActive to true, and would only dispatch onIdle() when receiving a non-RUNNING state if isSyncActive was already true.
The new implementation should be more reliable, as it will trigger onIdle when receiving a "finished" state, even if the running state wasn't previously received. With that, we make sure we correctly stop the loading state on screen.
Differential Revision: https://phabricator.services.mozilla.com/D272098
Diffstat:
4 files changed, 189 insertions(+), 15 deletions(-)
diff --git a/mobile/android/android-components/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/SyncManager.kt b/mobile/android/android-components/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/SyncManager.kt
@@ -4,6 +4,8 @@
package mozilla.components.service.fxa.sync
+import androidx.annotation.VisibleForTesting
+import androidx.work.WorkInfo
import mozilla.components.concept.storage.KeyProvider
import mozilla.components.concept.sync.SyncableStore
import mozilla.components.service.fxa.SyncConfig
@@ -117,7 +119,7 @@ internal interface SyncDispatcher : Closeable, Observable<SyncStatusObserver> {
)
fun startPeriodicSync(unit: TimeUnit, period: Long, initialDelay: Long)
fun stopPeriodicSync()
- fun workersStateChanged(isRunning: Boolean)
+ fun workersStateChanged(currentWorkStates: List<WorkInfo.State>?)
}
/**
@@ -130,7 +132,8 @@ internal abstract class SyncManager(
open val logger = Logger("SyncManager")
// A SyncDispatcher instance bound to an account and a set of syncable stores.
- private var syncDispatcher: SyncDispatcher? = null
+ @VisibleForTesting
+ internal var syncDispatcher: SyncDispatcher? = null
private val syncStatusObserverRegistry = ObserverRegistry<SyncStatusObserver>()
diff --git a/mobile/android/android-components/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/WorkManagerSyncManager.kt b/mobile/android/android-components/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/WorkManagerSyncManager.kt
@@ -124,14 +124,8 @@ internal object WorkersLiveDataObserver {
if (workersLiveData.hasObservers()) return
// This must be called on the UI thread.
- workersLiveData.observeForever {
- val isRunning = when (it?.any { worker -> worker.state == WorkInfo.State.RUNNING }) {
- null -> false
- false -> false
- true -> true
- }
-
- dispatcher?.workersStateChanged(isRunning)
+ workersLiveData.observeForever { workers ->
+ dispatcher?.workersStateChanged(workers?.map { it.state })
// TODO process errors coming out of worker.outputData
}
@@ -157,13 +151,13 @@ internal class WorkManagerSyncDispatcher(
stopPeriodicSync()
}
- override fun workersStateChanged(isRunning: Boolean) {
- if (isSyncActive && !isRunning) {
- notifyObservers { onIdle() }
- isSyncActive = false
- } else if (!isSyncActive && isRunning) {
+ override fun workersStateChanged(currentWorkStates: List<WorkInfo.State>?) {
+ if (currentWorkStates?.any { it == WorkInfo.State.RUNNING } == true) {
notifyObservers { onStarted() }
isSyncActive = true
+ } else if (currentWorkStates?.any { it.isFinished } == true) {
+ notifyObservers { onIdle() }
+ isSyncActive = false
}
}
diff --git a/mobile/android/android-components/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/sync/FakeSyncStatusObserver.kt b/mobile/android/android-components/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/sync/FakeSyncStatusObserver.kt
@@ -0,0 +1,29 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.service.fxa.sync
+
+import java.lang.Exception
+
+class FakeSyncStatusObserver : SyncStatusObserver {
+ sealed class Event {
+ object OnStarted : Event()
+ object OnIdle : Event()
+ data class OnError(val error: Exception?) : Event()
+ }
+
+ val events = mutableListOf<Event>()
+
+ override fun onStarted() {
+ events.add(Event.OnStarted)
+ }
+
+ override fun onIdle() {
+ events.add(Event.OnIdle)
+ }
+
+ override fun onError(error: Exception?) {
+ events.add(Event.OnError(error))
+ }
+}
diff --git a/mobile/android/android-components/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/sync/WorkManagerSyncManagerTest.kt b/mobile/android/android-components/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/sync/WorkManagerSyncManagerTest.kt
@@ -5,8 +5,15 @@
package mozilla.components.service.fxa.sync
import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.work.Configuration
+import androidx.work.WorkInfo
import androidx.work.WorkerParameters
import androidx.work.impl.utils.taskexecutor.TaskExecutor
+import androidx.work.testing.WorkManagerTestInitHelper
+import mozilla.components.service.fxa.SyncConfig
+import mozilla.components.service.fxa.SyncEngine
+import mozilla.components.service.fxa.sync.FakeSyncStatusObserver.Event.OnIdle
+import mozilla.components.service.fxa.sync.FakeSyncStatusObserver.Event.OnStarted
import mozilla.components.service.fxa.sync.WorkManagerSyncWorker.Companion.SYNC_STAGGER_BUFFER_MS
import mozilla.components.service.fxa.sync.WorkManagerSyncWorker.Companion.engineSyncTimestamp
import mozilla.components.support.test.mock
@@ -14,6 +21,7 @@ import mozilla.components.support.test.robolectric.testContext
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@@ -33,6 +41,11 @@ class WorkManagerSyncManagerTest {
`when`(mockParam.taskExecutor).thenReturn(mockTaskExecutor)
`when`(mockTaskExecutor.serialTaskExecutor).thenReturn(mock())
`when`(mockParam.tags).thenReturn(mockTags)
+
+ WorkManagerTestInitHelper.initializeTestWorkManager(
+ testContext,
+ Configuration.Builder().build(),
+ )
}
@Test
@@ -94,4 +107,139 @@ class WorkManagerSyncManagerTest {
assert(workerManagerSyncWorker.isDebounced())
assertFalse(workerManagerSyncWorker.lastSyncedWithinStaggerBuffer("test"))
}
+
+ @Test
+ fun `WHEN workerStateChanged receives RUNNING state THEN onStarted is notified`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.RUNNING))
+
+ assertEquals(listOf(OnStarted), observer.events)
+ assertTrue(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged receives SUCCEEDED state THEN onIdle is notified`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.ENQUEUED))
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.SUCCEEDED))
+
+ assertEquals(listOf(OnIdle), observer.events)
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged receives FAILED state THEN onIdle is notified`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.ENQUEUED))
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.FAILED))
+
+ assertEquals(listOf(OnIdle), observer.events)
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged receives CANCELLED state THEN onIdle is notified`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.CANCELLED))
+
+ assertEquals(listOf(OnIdle), observer.events)
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged receives null state THEN nothing happens`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(null)
+
+ assertTrue(observer.events.isEmpty())
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged receives empty list THEN nothing happens`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(emptyList())
+
+ assertTrue(observer.events.isEmpty())
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged receives ENQUEUED state THEN nothing happens`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.ENQUEUED))
+
+ assertTrue(observer.events.isEmpty())
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged receives RUNNING then SUCCEEDED THEN both onStarted and onIdle are notified in right order`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.ENQUEUED))
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.RUNNING))
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.SUCCEEDED))
+
+ assertEquals(listOf(OnStarted, OnIdle), observer.events)
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged is called with multiple workers in different states including one RUNNING THEN onStarted is notified`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.ENQUEUED, WorkInfo.State.RUNNING))
+
+ // Verify onStarted was called (because at least one is RUNNING)
+ assertEquals(listOf(OnStarted), observer.events)
+ assertTrue(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged is called with multiple workers in different states including one SUCCEEDED THEN onIdle is notified`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.SUCCEEDED, WorkInfo.State.FAILED))
+
+ assertEquals(listOf(OnIdle), observer.events)
+ assertFalse(syncManager.isSyncActive())
+ }
+
+ @Test
+ fun `WHEN workerStateChanged is called with multiple workers in different states including one SUCCEEDED and one RUNNING THEN onStarted is notified`() {
+ val observer = FakeSyncStatusObserver()
+ val syncManager = createSyncManager(observer)
+
+ syncManager.syncDispatcher?.workersStateChanged(listOf(WorkInfo.State.SUCCEEDED, WorkInfo.State.RUNNING))
+
+ assertEquals(listOf(OnStarted), observer.events)
+ assertTrue(syncManager.isSyncActive())
+ }
+
+ private fun createSyncManager(observer: FakeSyncStatusObserver): WorkManagerSyncManager =
+ WorkManagerSyncManager(
+ testContext,
+ SyncConfig(supportedEngines = setOf(SyncEngine.Tabs), periodicSyncConfig = null),
+ ).apply {
+ registerSyncStatusObserver(observer)
+ start()
+ }
}
