From d463184dbe5ba3ce8eb27bd6cdbe6efedb0d85d9 Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Wed, 26 Aug 2020 12:01:52 -0400 Subject: [PATCH] Closes #8240: Only dismiss prompts that are not already dismissed --- .../gecko/prompt/GeckoPromptDelegate.kt | 28 ++++++++---- .../gecko/prompt/GeckoPromptDelegateTest.kt | 43 +++++++++++++++---- .../gecko/prompt/GeckoPromptDelegate.kt | 29 +++++++++---- .../gecko/prompt/GeckoPromptDelegateTest.kt | 27 ++++++++++++ .../gecko/prompt/GeckoPromptDelegate.kt | 28 ++++++++---- .../gecko/prompt/GeckoPromptDelegateTest.kt | 27 ++++++++++++ docs/changelog.md | 3 ++ 7 files changed, 149 insertions(+), 36 deletions(-) diff --git a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt b/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt index 38c295359db..7fb3db14009 100644 --- a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt +++ b/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/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt b/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt index 794fac807f9..7154d84b5d4 100644 --- a/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt +++ b/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 @@ -145,11 +147,11 @@ class GeckoPromptDelegateTest { ) mockSession.register( - object : EngineSession.Observer { - override fun onPromptRequest(promptRequest: PromptRequest) { - promptRequestSingleChoice = promptRequest - } - }) + object : EngineSession.Observer { + override fun onPromptRequest(promptRequest: PromptRequest) { + promptRequestSingleChoice = promptRequest + } + }) val geckoResult = gecko.onChoicePrompt(mock(), geckoPrompt) geckoResult!!.accept { @@ -478,8 +480,7 @@ class GeckoPromptDelegateTest { dateRequest = promptRequest } }) - val geckoResult = - promptDelegate.onDateTimePrompt(mock(), GeckoDateTimePrompt(type = DATETIME_LOCAL)) + val geckoResult = promptDelegate.onDateTimePrompt(mock(), GeckoDateTimePrompt(type = DATETIME_LOCAL)) geckoResult!!.accept { confirmCalled = true } @@ -1080,6 +1081,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, @@ -1102,7 +1127,7 @@ class GeckoPromptDelegateTest { title: String = "title", type: Int, capture: Int = 0, - mimeTypes: Array = emptyArray() + mimeTypes: Array = emptyArray() ) : GeckoSession.PromptDelegate.FilePrompt(title, type, capture, mimeTypes) class GeckoAuthPrompt( diff --git a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt index 38c295359db..e694034e817 100644 --- a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt +++ b/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/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt b/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt index 794fac807f9..dd64484ec2e 100644 --- a/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt +++ b/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/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt b/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt index 38c295359db..7fb3db14009 100644 --- a/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt +++ b/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/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt index 62c5ba3aeb0..4ae8e0f48d1 100644 --- a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegateTest.kt +++ b/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/docs/changelog.md b/docs/changelog.md index 00cab67a4e4..37be4539956 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -27,6 +27,9 @@ permalink: /changelog/ * **lib-push-firebase** * Removed non-essential dependency on `com.google.firebase:firebase-core`. +* **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. + # 56.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v55.0.0...v56.0.0)