PM-9081: Should cancel the job not the scope when managing autofill requests (#3389)

Author: david-livefront

app/src/main/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorImpl.kt

@@ -18,7 +18,7 @@ import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager
 import com.x8bit.bitwarden.data.platform.repository.SettingsRepository
 import com.x8bit.bitwarden.data.vault.datasource.network.model.PolicyTypeJson
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.cancel
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.launch
 
 /**
@@ -41,14 +41,19 @@ class AutofillProcessorImpl(
      */
     private val scope: CoroutineScope = CoroutineScope(dispatcherManager.unconfined)
 
+    /**
+     * The job being used to process the fill request.
+     */
+    private var job: Job = Job().apply { complete() }
+
     override fun processFillRequest(
         autofillAppInfo: AutofillAppInfo,
         cancellationSignal: CancellationSignal,
         fillCallback: FillCallback,
         request: FillRequest,
     ) {
         // Set the listener so that any long running work is cancelled when it is no longer needed.
-        cancellationSignal.setOnCancelListener { scope.cancel() }
+        cancellationSignal.setOnCancelListener { job.cancel() }
         // Process the OS data and handle invoking the callback with the result.
         process(
             autofillAppInfo = autofillAppInfo,
@@ -113,7 +118,8 @@ class AutofillProcessorImpl(
         )
         when (autofillRequest) {
             is AutofillRequest.Fillable -> {
-                scope.launch {
+                job.cancel()
+                job = scope.launch {
                     // Fulfill the [autofillRequest].
                     val filledData = filledDataBuilder.build(
                         autofillRequest = autofillRequest,

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

@@ -21,40 +21,41 @@ import com.x8bit.bitwarden.data.autofill.model.FilledData
 import com.x8bit.bitwarden.data.autofill.parser.AutofillParser
 import com.x8bit.bitwarden.data.autofill.util.createAutofillSavedItemIntentSender
 import com.x8bit.bitwarden.data.autofill.util.toAutofillSaveItem
+import com.x8bit.bitwarden.data.platform.base.FakeDispatcherManager
 import com.x8bit.bitwarden.data.platform.manager.PolicyManager
-import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager
 import com.x8bit.bitwarden.data.platform.repository.SettingsRepository
 import com.x8bit.bitwarden.data.vault.datasource.network.model.PolicyTypeJson
 import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson
+import io.mockk.clearMocks
 import io.mockk.coEvery
 import io.mockk.coVerify
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
 import io.mockk.mockkStatic
 import io.mockk.runs
+import io.mockk.slot
 import io.mockk.unmockkStatic
 import io.mockk.verify
-import kotlinx.coroutines.ExperimentalCoroutinesApi
-import kotlinx.coroutines.test.UnconfinedTestDispatcher
+import kotlinx.coroutines.test.StandardTestDispatcher
 import kotlinx.coroutines.test.runTest
 import org.junit.jupiter.api.AfterEach
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 
-@OptIn(ExperimentalCoroutinesApi::class)
 class AutofillProcessorTest {
     private lateinit var autofillProcessor: AutofillProcessor
 
-    private val dispatcherManager: DispatcherManager = mockk()
+    private val testDispatcher = StandardTestDispatcher()
+    private val dispatcherManager: FakeDispatcherManager =
+        FakeDispatcherManager(unconfined = testDispatcher)
     private val cancellationSignal: CancellationSignal = mockk()
     private val filledDataBuilder: FilledDataBuilder = mockk()
     private val fillResponseBuilder: FillResponseBuilder = mockk()
     private val parser: AutofillParser = mockk()
     private val policyManager: PolicyManager = mockk()
     private val saveInfoBuilder: SaveInfoBuilder = mockk()
     private val settingsRepository: SettingsRepository = mockk()
-    private val testDispatcher = UnconfinedTestDispatcher()
 
     private val appInfo: AutofillAppInfo = AutofillAppInfo(
         context = mockk(),
@@ -67,7 +68,6 @@ class AutofillProcessorTest {
     fun setup() {
         mockkStatic(::createAutofillSavedItemIntentSender)
         mockkStatic(AutofillRequest.Fillable::toAutofillSaveItem)
-        every { dispatcherManager.unconfined } returns testDispatcher
 
         autofillProcessor = AutofillProcessorImpl(
             dispatcherManager = dispatcherManager,
@@ -82,9 +82,6 @@ class AutofillProcessorTest {
 
     @AfterEach
     fun teardown() {
-        verify(exactly = 1) {
-            dispatcherManager.unconfined
-        }
         unmockkStatic(::createAutofillSavedItemIntentSender)
         unmockkStatic(AutofillRequest.Fillable::toAutofillSaveItem)
     }
@@ -179,6 +176,8 @@ class AutofillProcessorTest {
                 request = fillRequest,
             )
 
+            testDispatcher.scheduler.runCurrent()
+
             // Verify
             coVerify(exactly = 1) {
                 filledDataBuilder.build(
@@ -386,6 +385,106 @@ class AutofillProcessorTest {
             saveCallback.onSuccess()
         }
     }
+
+    @Suppress("MaxLineLength")
+    @Test
+    fun `processFillRequest should allow additional requests to be invoked after cancellation signal is triggered`() {
+        // Setup
+        val autofillPartition: AutofillPartition = mockk()
+        val autofillRequest: AutofillRequest.Fillable = mockk {
+            every { packageName } returns PACKAGE_NAME
+            every { partition } returns autofillPartition
+        }
+        val fillRequest: FillRequest = mockk()
+        val saveInfo: SaveInfo = mockk()
+        val filledData = FilledData(
+            filledPartitions = listOf(mockk()),
+            ignoreAutofillIds = emptyList(),
+            originalPartition = mockk(),
+            uri = null,
+            vaultItemInlinePresentationSpec = null,
+            isVaultLocked = false,
+        )
+        val fillResponse: FillResponse = mockk()
+        val cancellationSignalListener = slot<CancellationSignal.OnCancelListener>()
+        every {
+            cancellationSignal.setOnCancelListener(capture(cancellationSignalListener))
+        } just runs
+        every {
+            parser.parse(autofillAppInfo = appInfo, fillRequest = fillRequest)
+        } returns autofillRequest
+        coEvery { filledDataBuilder.build(autofillRequest = autofillRequest) } returns filledData
+        every {
+            saveInfoBuilder.build(
+                autofillAppInfo = appInfo,
+                autofillPartition = autofillPartition,
+                fillRequest = fillRequest,
+                packageName = PACKAGE_NAME,
+            )
+        } returns saveInfo
+        every {
+            fillResponseBuilder.build(
+                autofillAppInfo = appInfo,
+                filledData = filledData,
+                saveInfo = saveInfo,
+            )
+        } returns fillResponse
+        every { fillCallback.onSuccess(fillResponse) } just runs
+
+        // Test
+        autofillProcessor.processFillRequest(
+            autofillAppInfo = appInfo,
+            cancellationSignal = cancellationSignal,
+            fillCallback = fillCallback,
+            request = fillRequest,
+        )
+
+        // Cancel the job and validate that nothing runs
+        cancellationSignalListener.captured.onCancel()
+        testDispatcher.scheduler.runCurrent()
+        verify(exactly = 1) {
+            // These run as they are not part of the coroutine
+            cancellationSignal.setOnCancelListener(any())
+            parser.parse(autofillAppInfo = appInfo, fillRequest = fillRequest)
+        }
+        coVerify(exactly = 0) {
+            filledDataBuilder.build(autofillRequest = autofillRequest)
+        }
+        verify(exactly = 0) {
+            fillResponseBuilder.build(
+                autofillAppInfo = appInfo,
+                filledData = filledData,
+                saveInfo = saveInfo,
+            )
+            fillCallback.onSuccess(fillResponse)
+        }
+        clearMocks(cancellationSignal, parser, answers = false)
+
+        // Test again after cancelling
+        autofillProcessor.processFillRequest(
+            autofillAppInfo = appInfo,
+            cancellationSignal = cancellationSignal,
+            fillCallback = fillCallback,
+            request = fillRequest,
+        )
+
+        testDispatcher.scheduler.runCurrent()
+
+        // Verify
+        verify(exactly = 1) {
+            cancellationSignal.setOnCancelListener(any())
+            parser.parse(autofillAppInfo = appInfo, fillRequest = fillRequest)
+            fillResponseBuilder.build(
+                autofillAppInfo = appInfo,
+                filledData = filledData,
+                saveInfo = saveInfo,
+            )
+            fillCallback.onSuccess(fillResponse)
+        }
+        coVerify(exactly = 1) {
+            filledDataBuilder.build(autofillRequest = autofillRequest)
+        }
+    }
 }
 
 private const val PACKAGE_NAME: String = "com.google"