From 32e3f1e9badfe897ccd5470b7d788168403d4b77 Mon Sep 17 00:00:00 2001 From: David Perez Date: Mon, 1 Jul 2024 13:09:50 -0500 Subject: [PATCH] PM-9081: Should cancel the job not the scope when managing autofill requests (#3389) --- .../processor/AutofillProcessorImpl.kt | 12 +- .../processor/AutofillProcessorTest.kt | 119 ++++++++++++++++-- 2 files changed, 118 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorImpl.kt index d8d49810bd..a6d17403f7 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorImpl.kt +++ b/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,6 +41,11 @@ 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, @@ -48,7 +53,7 @@ class AutofillProcessorImpl( 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, diff --git a/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorTest.kt index 274e0c4517..31abbcd295 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillProcessorTest.kt @@ -21,11 +21,12 @@ 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 @@ -33,20 +34,21 @@ 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() @@ -54,7 +56,6 @@ class AutofillProcessorTest { 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() + 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"