Add timeouts to operations that could hang (#3553)

app/src/main/java/com/x8bit/bitwarden/data/autofill/provider/AutofillCipherProviderImpl.kt

@@ -6,11 +6,22 @@ import com.bitwarden.vault.CipherView
 import com.x8bit.bitwarden.data.auth.repository.AuthRepository
 import com.x8bit.bitwarden.data.autofill.model.AutofillCipher
 import com.x8bit.bitwarden.data.platform.manager.ciphermatching.CipherMatchingManager
+import com.x8bit.bitwarden.data.platform.util.firstWithTimeoutOrNull
 import com.x8bit.bitwarden.data.platform.util.subtitle
 import com.x8bit.bitwarden.data.vault.repository.VaultRepository
 import com.x8bit.bitwarden.data.vault.repository.model.VaultUnlockData
 import com.x8bit.bitwarden.data.vault.repository.util.statusFor
-import kotlinx.coroutines.flow.first
+
+/**
+ * The duration, in milliseconds, we should wait while waiting for the vault status to not be
+ * 'UNLOCKING' before proceeding.
+ */
+private const val VAULT_LOCKED_TIMEOUT_MS: Long = 500L
+
+/**
+ * The duration, in milliseconds, we should wait while retrieving ciphers before proceeding.
+ */
+private const val GET_CIPHERS_TIMEOUT_MS: Long = 5_000L
 
 /**
  * The default [AutofillCipherProvider] implementation. This service is used for getting current
@@ -28,9 +39,11 @@ class AutofillCipherProviderImpl(
 
         // Wait for any unlocking actions to finish. This can be relevant on startup for Never lock
         // accounts.
-        vaultRepository.vaultUnlockDataStateFlow.first {
+        vaultRepository
-            it.statusFor(userId) != VaultUnlockData.Status.UNLOCKING
+            .vaultUnlockDataStateFlow
-        }
+            .firstWithTimeoutOrNull(timeMillis = VAULT_LOCKED_TIMEOUT_MS) {
+                it.statusFor(userId = userId) != VaultUnlockData.Status.UNLOCKING
+            }
 
         return !vaultRepository.isVaultUnlocked(userId = userId)
     }
@@ -105,6 +118,6 @@ class AutofillCipherProviderImpl(
         vaultRepository
             .ciphersStateFlow
             .takeUnless { isVaultLocked() }
-            ?.first { it.data != null }
+            ?.firstWithTimeoutOrNull(timeMillis = GET_CIPHERS_TIMEOUT_MS) { it.data != null }
             ?.data
 }

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerImpl.kt

@@ -5,6 +5,7 @@ import com.bitwarden.vault.CipherView
 import com.bitwarden.vault.LoginUriView
 import com.bitwarden.vault.UriMatchType
 import com.x8bit.bitwarden.data.platform.repository.SettingsRepository
+import com.x8bit.bitwarden.data.platform.util.firstWithTimeoutOrNull
 import com.x8bit.bitwarden.data.platform.util.getDomainOrNull
 import com.x8bit.bitwarden.data.platform.util.getHostWithPortOrNull
 import com.x8bit.bitwarden.data.platform.util.getWebHostFromAndroidUriOrNull
@@ -13,7 +14,6 @@ import com.x8bit.bitwarden.data.platform.util.regexOrNull
 import com.x8bit.bitwarden.data.vault.repository.VaultRepository
 import com.x8bit.bitwarden.data.vault.repository.model.DomainsData
 import com.x8bit.bitwarden.ui.platform.feature.settings.autofill.util.toSdkUriMatchType
-import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.mapNotNull
 import kotlin.text.RegexOption
 import kotlin.text.isNullOrBlank
@@ -21,6 +21,11 @@ import kotlin.text.lowercase
 import kotlin.text.matches
 import kotlin.text.startsWith
 
+/**
+ * The duration, in milliseconds, we should wait while retrieving domain data before we proceed.
+ */
+private const val GET_DOMAINS_TIMEOUT_MS: Long = 1_000L
+
 /**
  * The default [CipherMatchingManager] implementation. This class is responsible for matching
  * ciphers based on special criteria.
@@ -37,7 +42,8 @@ class CipherMatchingManagerImpl(
         val equivalentDomainsData = vaultRepository
             .domainsStateFlow
             .mapNotNull { it.data }
-            .first()
+            .firstWithTimeoutOrNull(timeMillis = GET_DOMAINS_TIMEOUT_MS)
+            ?: return emptyList()
 
         val isAndroidApp = matchUri.isAndroidApp()
         val defaultUriMatchType = settingsRepository.defaultUriMatchType.toSdkUriMatchType()

app/src/main/java/com/x8bit/bitwarden/data/platform/util/FlowExtensions.kt

@@ -0,0 +1,22 @@
+package com.x8bit.bitwarden.data.platform.util
+
+import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.withTimeoutOrNull
+
+/**
+ * Returns the first element emitted by the [Flow] or `null` if the operation exceeds the given
+ * timeout of [timeMillis].
+ */
+suspend fun <T> Flow<T>.firstWithTimeoutOrNull(
+    timeMillis: Long,
+): T? = withTimeoutOrNull(timeMillis = timeMillis) { first() }
+
+/**
+ * Returns the first element emitted by the [Flow] matching the given [predicate] or `null` if the
+ * operation exceeds the given timeout of [timeMillis].
+ */
+suspend fun <T> Flow<T>.firstWithTimeoutOrNull(
+    timeMillis: Long,
+    predicate: suspend (T) -> Boolean,
+): T? = withTimeoutOrNull(timeMillis = timeMillis) { first(predicate) }

app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillCipherProviderTest.kt

@@ -21,6 +21,7 @@ import io.mockk.every
 import io.mockk.mockk
 import io.mockk.mockkStatic
 import io.mockk.unmockkStatic
+import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.async
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.test.runTest
@@ -31,6 +32,7 @@ import org.junit.jupiter.api.Assertions.assertTrue
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 
+@OptIn(ExperimentalCoroutinesApi::class)
 class AutofillCipherProviderTest {
     private val cardView: CardView = mockk {
         every { cardholderName } returns CARD_CARDHOLDER_NAME
@@ -139,7 +141,7 @@ class AutofillCipherProviderTest {
                 autofillCipherProvider.isVaultLocked()
             }
 
-            testScheduler.advanceUntilIdle()
+            testScheduler.runCurrent()
             assertFalse(result.isCompleted)
 
             mutableVaultStateFlow.value = listOf(
@@ -155,6 +157,53 @@ class AutofillCipherProviderTest {
             assertFalse(result.await())
         }
 
+    @Suppress("MaxLineLength")
+    @Test
+    fun `isVaultLocked when there is an active user should wait for pending unlocking to finish and return the locked state for that user when it times out`() =
+        runTest {
+            every { authRepository.activeUserId } returns ACTIVE_USER_ID
+            mutableVaultStateFlow.value = listOf(
+                VaultUnlockData(
+                    userId = ACTIVE_USER_ID,
+                    status = VaultUnlockData.Status.UNLOCKING,
+                ),
+            )
+
+            val result = async {
+                autofillCipherProvider.isVaultLocked()
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+
+            testScheduler.advanceTimeBy(delayTimeMillis = 1_000L)
+            testScheduler.runCurrent()
+            assertTrue(result.isCompleted)
+            assertTrue(result.await())
+        }
+
+    @Suppress("MaxLineLength")
+    @Test
+    fun `getCardAutofillCiphers when unlocked should return empty list when retrieving ciphers times out`() =
+        runTest {
+            coEvery { vaultRepository.isVaultUnlocked(ACTIVE_USER_ID) } returns true
+            mutableCiphersStateFlow.value = DataState.Loading
+
+            // Test
+            val actual = async {
+                autofillCipherProvider.getCardAutofillCiphers()
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(actual.isCompleted)
+            testScheduler.advanceTimeBy(delayTimeMillis = 5_000L)
+            testScheduler.runCurrent()
+
+            // Verify
+            assertTrue(actual.isCompleted)
+            assertEquals(emptyList<AutofillCipher.Card>(), actual.await())
+        }
+
     @Suppress("MaxLineLength")
     @Test
     fun `getCardAutofillCiphers when unlocked should return non-null, non-deleted, and non-reprompt card ciphers`() =
@@ -205,6 +254,28 @@ class AutofillCipherProviderTest {
         assertEquals(emptyList<AutofillCipher.Card>(), actual)
     }
 
+    @Suppress("MaxLineLength")
+    @Test
+    fun `getLoginAutofillCiphers when unlocked should return empty list when retrieving ciphers times out`() =
+        runTest {
+            coEvery { vaultRepository.isVaultUnlocked(ACTIVE_USER_ID) } returns true
+            mutableCiphersStateFlow.value = DataState.Loading
+
+            // Test
+            val actual = async {
+                autofillCipherProvider.getLoginAutofillCiphers(uri = URI)
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(actual.isCompleted)
+            testScheduler.advanceTimeBy(delayTimeMillis = 5_000L)
+            testScheduler.runCurrent()
+
+            // Verify
+            assertTrue(actual.isCompleted)
+            assertEquals(emptyList<AutofillCipher.Login>(), actual.await())
+        }
+
     @Suppress("MaxLineLength")
     @Test
     fun `getLoginAutofillCiphers when unlocked should return matched, non-deleted, non-reprompt, login ciphers`() =

app/src/test/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerTest.kt

@@ -17,13 +17,18 @@ import io.mockk.every
 import io.mockk.mockk
 import io.mockk.mockkStatic
 import io.mockk.unmockkStatic
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.async
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.test.runTest
 import org.junit.jupiter.api.AfterEach
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Assertions.assertFalse
+import org.junit.jupiter.api.Assertions.assertTrue
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 
+@OptIn(ExperimentalCoroutinesApi::class)
 class CipherMatchingManagerTest {
     private lateinit var cipherMatchingManager: CipherMatchingManager
 
@@ -32,8 +37,11 @@ class CipherMatchingManagerTest {
     private val settingsRepository: SettingsRepository = mockk {
         every { defaultUriMatchType } returns DEFAULT_URI_MATCH_TYPE
     }
+    private val mutableDomainsStateFlow = MutableStateFlow<DataState<DomainsData>>(
+        value = DataState.Loaded(DOMAINS_DATA),
+    )
     private val vaultRepository: VaultRepository = mockk {
-        every { domainsStateFlow } returns MutableStateFlow(DataState.Loaded(DOMAINS_DATA))
+        every { domainsStateFlow } returns mutableDomainsStateFlow
     }
 
     // Setup test ciphers
@@ -179,6 +187,31 @@ class CipherMatchingManagerTest {
         )
     }
 
+    @Test
+    fun `filterCiphersForMatches should return an empty list when retrieving domains times out`() =
+        runTest {
+            // Setup
+            val uri = "google.com"
+            mutableDomainsStateFlow.value = DataState.Loading
+
+            // Test
+            val actual = async {
+                cipherMatchingManager.filterCiphersForMatches(
+                    ciphers = ciphers,
+                    matchUri = uri,
+                )
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(actual.isCompleted)
+            testScheduler.advanceTimeBy(delayTimeMillis = 1_000L)
+            testScheduler.runCurrent()
+
+            // Verify
+            assertTrue(actual.isCompleted)
+            assertEquals(emptyList<CipherView>(), actual.await())
+        }
+
     @Suppress("MaxLineLength")
     @Test
     fun `filterCiphersForMatches should perform cipher matching when is android app and matching URI`() =

app/src/test/java/com/x8bit/bitwarden/data/platform/util/FlowExtensionsTest.kt

@@ -0,0 +1,112 @@
+package com.x8bit.bitwarden.data.platform.util
+
+import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.async
+import kotlinx.coroutines.test.runTest
+import org.junit.jupiter.api.Assertions.assertFalse
+import org.junit.jupiter.api.Assertions.assertNotNull
+import org.junit.jupiter.api.Assertions.assertNull
+import org.junit.jupiter.api.Assertions.assertTrue
+import org.junit.jupiter.api.Test
+
+@OptIn(ExperimentalCoroutinesApi::class)
+class FlowExtensionsTest {
+
+    @Test
+    fun `firstWithTimeoutOrNull should return null when the defined timeout has be reached`() =
+        runTest {
+            val timeout = 1_000L
+            val mutableSharedFlow = bufferedMutableSharedFlow<Unit>()
+
+            val result = async {
+                mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout)
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+            testScheduler.advanceTimeBy(delayTimeMillis = timeout)
+            testScheduler.runCurrent()
+
+            assertTrue(result.isCompleted)
+            assertNull(result.await())
+        }
+
+    @Suppress("MaxLineLength")
+    @Test
+    fun `firstWithTimeoutOrNull should return a value when the value is emitted before the defined timeout has be reached`() =
+        runTest {
+            val timeout = 1_000L
+            val mutableSharedFlow = bufferedMutableSharedFlow<Unit>()
+
+            val result = async {
+                mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout)
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+            testScheduler.advanceTimeBy(delayTimeMillis = timeout / 2)
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+
+            mutableSharedFlow.tryEmit(Unit)
+            testScheduler.runCurrent()
+
+            assertTrue(result.isCompleted)
+            assertNotNull(result.await())
+        }
+
+    @Suppress("MaxLineLength")
+    @Test
+    fun `firstWithTimeoutOrNull with predicate should return null when the defined timeout has be reached`() =
+        runTest {
+            val timeout = 1_000L
+            val mutableSharedFlow = bufferedMutableSharedFlow<Boolean>()
+
+            val result = async {
+                mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout) { it }
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+            testScheduler.advanceTimeBy(delayTimeMillis = timeout / 2)
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+
+            mutableSharedFlow.tryEmit(false)
+            testScheduler.advanceTimeBy(delayTimeMillis = timeout)
+            testScheduler.runCurrent()
+
+            assertTrue(result.isCompleted)
+            assertNull(result.await())
+        }
+
+    @Suppress("MaxLineLength")
+    @Test
+    fun `firstWithTimeoutOrNull with predicate should return a value when the value is emitted before the defined timeout has be reached`() =
+        runTest {
+            val timeout = 1_000L
+            val mutableSharedFlow = bufferedMutableSharedFlow<Boolean>()
+
+            val result = async {
+                mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout) { it }
+            }
+
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+            testScheduler.advanceTimeBy(delayTimeMillis = timeout / 2)
+            testScheduler.runCurrent()
+            assertFalse(result.isCompleted)
+
+            mutableSharedFlow.tryEmit(false)
+            testScheduler.runCurrent()
+
+            assertFalse(result.isCompleted)
+
+            mutableSharedFlow.tryEmit(true)
+            testScheduler.runCurrent()
+
+            assertTrue(result.isCompleted)
+            assertNotNull(result.await())
+        }
+}