From 24c42c956a0648710c7573074288d494528f1feb Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Wed, 26 Aug 2020 12:01:52 -0400 Subject: [PATCH] [components] Closes https://github.com/mozilla-mobile/android-components/issues/8240: Only dismiss prompts that are not already dismissed --- .../gecko/prompt/GeckoPromptDelegate.kt | 28 ++++++++++++------ .../gecko/prompt/GeckoPromptDelegateTest.kt | 28 +++++++++++++++++- .../gecko/prompt/GeckoPromptDelegate.kt | 29 +++++++++++++------ .../gecko/prompt/GeckoPromptDelegateTest.kt | 27 +++++++++++++++++ .../gecko/prompt/GeckoPromptDelegate.kt | 28 ++++++++++++------ .../gecko/prompt/GeckoPromptDelegateTest.kt | 27 +++++++++++++++++ .../android-components/docs/changelog.md | 3 ++ 7 files changed, 142 insertions(+), 28 deletions(-) diff --git a/mobile/android/android-components/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt b/mobile/android/android-components/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt index 38c295359dbad..7fb3db140095f 100644 --- a/mobile/android/android-components/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt +++ b/mobile/android/android-components/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt @@ -85,7 +85,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(Autocomplete.LoginSelectOption(login.toLoginEntry()))) } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { @@ -110,7 +110,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(Autocomplete.LoginSelectOption(login.toLoginEntry()))) } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } // Exactly one of `httpRealm` and `formSubmitURL` must be present to be a valid login entry. @@ -174,7 +174,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe prompt: PromptDelegate.AlertPrompt ): GeckoResult { val geckoResult = GeckoResult() - val onConfirm: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onConfirm: () -> Unit = { prompt.dismissSafely(geckoResult) } val title = prompt.title ?: "" val message = prompt.message ?: "" @@ -219,7 +219,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { @@ -298,7 +298,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe } } - val onDismiss: () -> Unit = { geckoResult.complete(geckoPrompt.dismiss()) } + val onDismiss: () -> Unit = { geckoPrompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -328,7 +328,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe val title = prompt.title ?: "" val inputLabel = prompt.message ?: "" val inputValue = prompt.defaultValue ?: "" - val onDismiss: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: () -> Unit = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -352,7 +352,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe ): GeckoResult? { val geckoResult = GeckoResult() val onConfirm: (String) -> Unit = { geckoResult.complete(prompt.confirm(it)) } - val onDismiss: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: () -> Unit = { prompt.dismissSafely(geckoResult) } val defaultColor = prompt.defaultValue ?: "" @@ -403,7 +403,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe val geckoResult = GeckoResult() val onSuccess = { geckoResult.complete(prompt.confirm(GECKO_PROMPT_SHARE_RESULT.SUCCESS)) } val onFailure = { geckoResult.complete(prompt.confirm(GECKO_PROMPT_SHARE_RESULT.FAILURE)) } - val onDismiss = { geckoResult.complete(prompt.dismiss()) } + val onDismiss = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -437,7 +437,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(PromptDelegate.ButtonPrompt.Type.NEGATIVE)) } - val onDismiss: (Boolean) -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: (Boolean) -> Unit = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -573,3 +573,13 @@ internal fun Date.toString(format: String): String { val formatter = SimpleDateFormat(format, Locale.ROOT) return formatter.format(this) ?: "" } + +/** + * Only dismiss if the prompt is not already dismissed. + */ +@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) +internal fun PromptDelegate.BasePrompt.dismissSafely(geckoResult: GeckoResult) { + if (!this.isComplete) { + geckoResult.complete(dismiss()) + } +} diff --git a/mobile/android/android-components/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt b/mobile/android/android-components/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt index 794fac807f9c1..cdb0ad3ee038a 100644 --- a/mobile/android/android-components/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt +++ b/mobile/android/android-components/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt @@ -26,10 +26,12 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.mockito.Mockito.doReturn import org.mockito.Mockito.spy +import org.mockito.Mockito +import org.mockito.Mockito.doReturn import org.mozilla.gecko.util.GeckoBundle import org.mozilla.geckoview.Autocomplete +import org.mozilla.geckoview.GeckoResult import org.mozilla.geckoview.GeckoRuntime import org.mozilla.geckoview.GeckoSession import org.mozilla.geckoview.GeckoSession.PromptDelegate.DateTimePrompt.Type.DATE @@ -1080,6 +1082,30 @@ class GeckoPromptDelegateTest { assertTrue(dismissWasCalled) } + @Test + fun `dismissSafely only dismiss if the prompt is NOT already dismissed`() { + val prompt = spy(GeckoAlertPrompt()) + val geckoResult = mock>() + + doReturn(false).`when`(prompt).isComplete + + prompt.dismissSafely(geckoResult) + + Mockito.verify(geckoResult).complete(any()) + } + + @Test + fun `dismissSafely do nothing if the prompt is already dismissed`() { + val prompt = spy(GeckoAlertPrompt()) + val geckoResult = mock>() + + doReturn(true).`when`(prompt).isComplete + + prompt.dismissSafely(geckoResult) + + Mockito.verify(geckoResult, Mockito.never()).complete(any()) + } + class GeckoChoicePrompt( title: String, message: String, diff --git a/mobile/android/android-components/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt b/mobile/android/android-components/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt index 38c295359dbad..e694034e81735 100644 --- a/mobile/android/android-components/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt +++ b/mobile/android/android-components/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt @@ -85,7 +85,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(Autocomplete.LoginSelectOption(login.toLoginEntry()))) } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { @@ -110,7 +110,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(Autocomplete.LoginSelectOption(login.toLoginEntry()))) } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } // Exactly one of `httpRealm` and `formSubmitURL` must be present to be a valid login entry. @@ -174,7 +174,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe prompt: PromptDelegate.AlertPrompt ): GeckoResult { val geckoResult = GeckoResult() - val onConfirm: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onConfirm: () -> Unit = { prompt.dismissSafely(geckoResult) } val title = prompt.title ?: "" val message = prompt.message ?: "" @@ -219,7 +219,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { @@ -298,7 +298,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe } } - val onDismiss: () -> Unit = { geckoResult.complete(geckoPrompt.dismiss()) } + val onDismiss: () -> Unit = { geckoPrompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -328,7 +328,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe val title = prompt.title ?: "" val inputLabel = prompt.message ?: "" val inputValue = prompt.defaultValue ?: "" - val onDismiss: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: () -> Unit = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -352,7 +352,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe ): GeckoResult? { val geckoResult = GeckoResult() val onConfirm: (String) -> Unit = { geckoResult.complete(prompt.confirm(it)) } - val onDismiss: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: () -> Unit = { prompt.dismissSafely(geckoResult) } val defaultColor = prompt.defaultValue ?: "" @@ -403,7 +403,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe val geckoResult = GeckoResult() val onSuccess = { geckoResult.complete(prompt.confirm(GECKO_PROMPT_SHARE_RESULT.SUCCESS)) } val onFailure = { geckoResult.complete(prompt.confirm(GECKO_PROMPT_SHARE_RESULT.FAILURE)) } - val onDismiss = { geckoResult.complete(prompt.dismiss()) } + val onDismiss = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -437,7 +437,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(PromptDelegate.ButtonPrompt.Type.NEGATIVE)) } - val onDismiss: (Boolean) -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: (Boolean) -> Unit = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -573,3 +573,14 @@ internal fun Date.toString(format: String): String { val formatter = SimpleDateFormat(format, Locale.ROOT) return formatter.format(this) ?: "" } + +/** + * Only dismiss if the prompt is not already dismissed. + */ +@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + +internal fun PromptDelegate.BasePrompt.dismissSafely(geckoResult: GeckoResult) { + if (!this.isComplete) { + geckoResult.complete(dismiss()) + } +} diff --git a/mobile/android/android-components/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt b/mobile/android/android-components/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt index 794fac807f9c1..dd64484ec2e67 100644 --- a/mobile/android/android-components/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt +++ b/mobile/android/android-components/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt @@ -28,8 +28,11 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.doReturn import org.mockito.Mockito.spy +import org.mockito.Mockito.verify +import org.mockito.Mockito.never import org.mozilla.gecko.util.GeckoBundle import org.mozilla.geckoview.Autocomplete +import org.mozilla.geckoview.GeckoResult import org.mozilla.geckoview.GeckoRuntime import org.mozilla.geckoview.GeckoSession import org.mozilla.geckoview.GeckoSession.PromptDelegate.DateTimePrompt.Type.DATE @@ -1080,6 +1083,30 @@ class GeckoPromptDelegateTest { assertTrue(dismissWasCalled) } + @Test + fun `dismissSafely only dismiss if the prompt is NOT already dismissed`() { + val prompt = spy(GeckoAlertPrompt()) + val geckoResult = mock>() + + doReturn(false).`when`(prompt).isComplete + + prompt.dismissSafely(geckoResult) + + verify(geckoResult).complete(any()) + } + + @Test + fun `dismissSafely do nothing if the prompt is already dismissed`() { + val prompt = spy(GeckoAlertPrompt()) + val geckoResult = mock>() + + doReturn(true).`when`(prompt).isComplete + + prompt.dismissSafely(geckoResult) + + verify(geckoResult, never()).complete(any()) + } + class GeckoChoicePrompt( title: String, message: String, diff --git a/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt b/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt index 38c295359dbad..7fb3db140095f 100644 --- a/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt +++ b/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt @@ -85,7 +85,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(Autocomplete.LoginSelectOption(login.toLoginEntry()))) } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { @@ -110,7 +110,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(Autocomplete.LoginSelectOption(login.toLoginEntry()))) } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } // Exactly one of `httpRealm` and `formSubmitURL` must be present to be a valid login entry. @@ -174,7 +174,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe prompt: PromptDelegate.AlertPrompt ): GeckoResult { val geckoResult = GeckoResult() - val onConfirm: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onConfirm: () -> Unit = { prompt.dismissSafely(geckoResult) } val title = prompt.title ?: "" val message = prompt.message ?: "" @@ -219,7 +219,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe } val onDismiss: () -> Unit = { - geckoResult.complete(prompt.dismiss()) + prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { @@ -298,7 +298,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe } } - val onDismiss: () -> Unit = { geckoResult.complete(geckoPrompt.dismiss()) } + val onDismiss: () -> Unit = { geckoPrompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -328,7 +328,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe val title = prompt.title ?: "" val inputLabel = prompt.message ?: "" val inputValue = prompt.defaultValue ?: "" - val onDismiss: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: () -> Unit = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -352,7 +352,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe ): GeckoResult? { val geckoResult = GeckoResult() val onConfirm: (String) -> Unit = { geckoResult.complete(prompt.confirm(it)) } - val onDismiss: () -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: () -> Unit = { prompt.dismissSafely(geckoResult) } val defaultColor = prompt.defaultValue ?: "" @@ -403,7 +403,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe val geckoResult = GeckoResult() val onSuccess = { geckoResult.complete(prompt.confirm(GECKO_PROMPT_SHARE_RESULT.SUCCESS)) } val onFailure = { geckoResult.complete(prompt.confirm(GECKO_PROMPT_SHARE_RESULT.FAILURE)) } - val onDismiss = { geckoResult.complete(prompt.dismiss()) } + val onDismiss = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -437,7 +437,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe geckoResult.complete(prompt.confirm(PromptDelegate.ButtonPrompt.Type.NEGATIVE)) } - val onDismiss: (Boolean) -> Unit = { geckoResult.complete(prompt.dismiss()) } + val onDismiss: (Boolean) -> Unit = { prompt.dismissSafely(geckoResult) } geckoEngineSession.notifyObservers { onPromptRequest( @@ -573,3 +573,13 @@ internal fun Date.toString(format: String): String { val formatter = SimpleDateFormat(format, Locale.ROOT) return formatter.format(this) ?: "" } + +/** + * Only dismiss if the prompt is not already dismissed. + */ +@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) +internal fun PromptDelegate.BasePrompt.dismissSafely(geckoResult: GeckoResult) { + if (!this.isComplete) { + geckoResult.complete(dismiss()) + } +} diff --git a/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt b/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt index 62c5ba3aeb03c..4ae8e0f48d130 100644 --- a/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt +++ b/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt @@ -27,7 +27,10 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.spy import org.mockito.Mockito.doReturn +import org.mockito.Mockito.verify +import org.mockito.Mockito.never import org.mozilla.gecko.util.GeckoBundle +import org.mozilla.geckoview.GeckoResult import org.mozilla.geckoview.GeckoRuntime import org.mozilla.geckoview.GeckoSession import org.mozilla.geckoview.GeckoSession.PromptDelegate.DateTimePrompt.Type.DATE @@ -957,6 +960,30 @@ class GeckoPromptDelegateTest { assertTrue(dismissWasCalled) } + @Test + fun `dismissSafely only dismiss if the prompt is NOT already dismissed`() { + val prompt = spy(GeckoAlertPrompt()) + val geckoResult = mock>() + + doReturn(false).`when`(prompt).isComplete + + prompt.dismissSafely(geckoResult) + + verify(geckoResult).complete(any()) + } + + @Test + fun `dismissSafely do nothing if the prompt is already dismissed`() { + val prompt = spy(GeckoAlertPrompt()) + val geckoResult = mock>() + + doReturn(true).`when`(prompt).isComplete + + prompt.dismissSafely(geckoResult) + + verify(geckoResult, never()).complete(any()) + } + class GeckoChoicePrompt( title: String, message: String, diff --git a/mobile/android/android-components/docs/changelog.md b/mobile/android/android-components/docs/changelog.md index e5360e7211fe4..96a5e85eb76f1 100644 --- a/mobile/android/android-components/docs/changelog.md +++ b/mobile/android/android-components/docs/changelog.md @@ -34,6 +34,9 @@ permalink: /changelog/ * **feature-prompts** * Replaced generic icon in `LoginDialogFragment` with site icon (keep the generic one as fallback) +* **browser-engine-gecko**, **browser-engine-gecko-beta**, **browser-engine-gecko-nightly** + * 🚒 Bug fixed [issue #8240](https://github.com/mozilla-mobile/android-components/issues/8240) Crash when dismissing Share dialog. + * **feature-downloads** * ⚠️ **This is a breaking change**: `AndroidDownloadManager.download` returns a `Strings`, `AndroidDownloadManager.tryAgain` requires a `Strings` `id` parameter. * ⚠️ **This is a breaking change**: `ConsumeDownloadAction` requires a `Strings` `id` parameter.