From b3c8e1f3eea1812036da05beb13f2037d6b13109 Mon Sep 17 00:00:00 2001
From: William Durand <will+git@drnd.me>
Date: Mon, 18 Sep 2023 11:54:17 +0200
Subject: [PATCH] Bug 1853177 - Avoid multiple crash dialogs to be created and
 shown

---
 .../ExtensionProcessDisabledController.kt     | 113 ++++++++++--------
 .../ExtensionProcessDisabledControllerTest.kt |  39 +++++-
 2 files changed, 101 insertions(+), 51 deletions(-)

diff --git a/app/src/main/java/org/mozilla/fenix/addons/ExtensionProcessDisabledController.kt b/app/src/main/java/org/mozilla/fenix/addons/ExtensionProcessDisabledController.kt
index 129f859581..4d85768c22 100644
--- a/app/src/main/java/org/mozilla/fenix/addons/ExtensionProcessDisabledController.kt
+++ b/app/src/main/java/org/mozilla/fenix/addons/ExtensionProcessDisabledController.kt
@@ -10,6 +10,7 @@ import android.widget.Button
 import android.widget.TextView
 import androidx.annotation.UiContext
 import androidx.appcompat.app.AlertDialog
+import androidx.lifecycle.LifecycleOwner
 import mozilla.components.browser.state.action.ExtensionProcessDisabledPopupAction
 import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.concept.engine.Engine
@@ -20,52 +21,6 @@ import org.mozilla.fenix.R
 import org.mozilla.fenix.ext.components
 import org.mozilla.geckoview.WebExtensionController
 
-/**
- * Present a dialog to the user notifying of extension process spawning disabled and also asking
- * whether they would like to continue trying or disable extensions. If the user chooses to retry,
- * enable the extension process spawning with [WebExtensionController.enableExtensionProcessSpawning].
- *
- * @param context to show the AlertDialog
- * @param store The [BrowserStore] which holds the state for showing the dialog
- * @param webExtensionController to call when a user enables the process spawning
- * @param builder to use for creating the dialog which can be styled as needed
- * @param appName to be added to the message. Necessary to be added as a param for testing
- */
-private fun presentDialog(
-    @UiContext context: Context,
-    store: BrowserStore,
-    engine: Engine,
-    builder: AlertDialog.Builder,
-    appName: String,
-) {
-    val message = context.getString(R.string.addon_process_crash_dialog_message, appName)
-    var onDismissDialog: (() -> Unit)? = null
-    val layout = LayoutInflater.from(context)
-        .inflate(R.layout.crash_extension_dialog, null, false)
-    layout?.apply {
-        findViewById<TextView>(R.id.message)?.text = message
-        findViewById<Button>(R.id.positive)?.setOnClickListener {
-            engine.enableExtensionProcessSpawning()
-            Addons.extensionsProcessUiRetry.add()
-            store.dispatch(ExtensionProcessDisabledPopupAction(false))
-            onDismissDialog?.invoke()
-        }
-        findViewById<Button>(R.id.negative)?.setOnClickListener {
-            Addons.extensionsProcessUiDisable.add()
-            store.dispatch(ExtensionProcessDisabledPopupAction(false))
-            onDismissDialog?.invoke()
-        }
-    }
-    builder.apply {
-        setCancelable(false)
-        setView(layout)
-        setTitle(R.string.addon_process_crash_dialog_title)
-    }
-
-    val dialog = builder.show()
-    onDismissDialog = { dialog?.dismiss() }
-}
-
 /**
  * Controller for showing the user a dialog when the the extension process spawning has been disabled.
  *
@@ -84,4 +39,68 @@ class ExtensionProcessDisabledController(
 ) : ExtensionProcessDisabledPopupObserver(
     store,
     { presentDialog(context, store, engine, builder, appName) },
-)
+) {
+    override fun onDestroy(owner: LifecycleOwner) {
+        super.onDestroy(owner)
+        // In case the activity gets destroyed, we want to re-create the dialog.
+        shouldCreateDialog = true
+    }
+
+    companion object {
+        private var shouldCreateDialog: Boolean = true
+
+        /**
+         * Present a dialog to the user notifying of extension process spawning disabled and also asking
+         * whether they would like to continue trying or disable extensions. If the user chooses to retry,
+         * enable the extension process spawning with [WebExtensionController.enableExtensionProcessSpawning].
+         *
+         * @param context to show the AlertDialog
+         * @param store The [BrowserStore] which holds the state for showing the dialog
+         * @param webExtensionController to call when a user enables the process spawning
+         * @param builder to use for creating the dialog which can be styled as needed
+         * @param appName to be added to the message. Necessary to be added as a param for testing
+         */
+        private fun presentDialog(
+            @UiContext context: Context,
+            store: BrowserStore,
+            engine: Engine,
+            builder: AlertDialog.Builder,
+            appName: String,
+        ) {
+            if (!shouldCreateDialog) {
+                return
+            }
+
+            val message = context.getString(R.string.addon_process_crash_dialog_message, appName)
+            var onDismissDialog: (() -> Unit)? = null
+            val layout = LayoutInflater.from(context)
+                .inflate(R.layout.crash_extension_dialog, null, false)
+            layout?.apply {
+                findViewById<TextView>(R.id.message)?.text = message
+                findViewById<Button>(R.id.positive)?.setOnClickListener {
+                    engine.enableExtensionProcessSpawning()
+                    Addons.extensionsProcessUiRetry.add()
+                    store.dispatch(ExtensionProcessDisabledPopupAction(false))
+                    onDismissDialog?.invoke()
+                }
+                findViewById<Button>(R.id.negative)?.setOnClickListener {
+                    Addons.extensionsProcessUiDisable.add()
+                    store.dispatch(ExtensionProcessDisabledPopupAction(false))
+                    onDismissDialog?.invoke()
+                }
+            }
+            builder.apply {
+                setCancelable(false)
+                setView(layout)
+                setTitle(R.string.addon_process_crash_dialog_title)
+            }
+
+            val dialog = builder.show()
+            shouldCreateDialog = false
+            onDismissDialog = {
+                dialog?.dismiss()
+                shouldCreateDialog = true
+            }
+        }
+    }
+}
diff --git a/app/src/test/java/org/mozilla/fenix/addons/ExtensionProcessDisabledControllerTest.kt b/app/src/test/java/org/mozilla/fenix/addons/ExtensionProcessDisabledControllerTest.kt
index 4b48e12aeb..315978e13c 100644
--- a/app/src/test/java/org/mozilla/fenix/addons/ExtensionProcessDisabledControllerTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/addons/ExtensionProcessDisabledControllerTest.kt
@@ -22,6 +22,7 @@ import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.Mockito.mock
 import org.mockito.Mockito.never
+import org.mockito.Mockito.times
 import org.mockito.Mockito.verify
 import org.mozilla.fenix.R
 import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
@@ -40,8 +41,7 @@ class ExtensionProcessDisabledControllerTest {
         val dialog: AlertDialog = mock()
         val appName = "TestApp"
         val builder: AlertDialog.Builder = mock()
-        val controller =
-            ExtensionProcessDisabledController(testContext, store, engine, builder, appName)
+        val controller = ExtensionProcessDisabledController(testContext, store, engine, builder, appName)
         val buttonsContainerCaptor = argumentCaptor<View>()
 
         controller.start()
@@ -74,8 +74,7 @@ class ExtensionProcessDisabledControllerTest {
         val appName = "TestApp"
         val dialog: AlertDialog = mock()
         val builder: AlertDialog.Builder = mock()
-        val controller =
-            ExtensionProcessDisabledController(testContext, store, engine, builder, appName)
+        val controller = ExtensionProcessDisabledController(testContext, store, engine, builder, appName)
         val buttonsContainerCaptor = argumentCaptor<View>()
 
         controller.start()
@@ -100,4 +99,36 @@ class ExtensionProcessDisabledControllerTest {
         verify(engine, never()).enableExtensionProcessSpawning()
         verify(dialog).dismiss()
     }
+
+    @Test
+    fun `WHEN dispatching the same event twice THEN the dialog should only be created once`() {
+        val store = BrowserStore()
+        val engine: Engine = mock()
+        val appName = "TestApp"
+        val dialog: AlertDialog = mock()
+        val builder: AlertDialog.Builder = mock()
+        val controller = ExtensionProcessDisabledController(testContext, store, engine, builder, appName)
+        val buttonsContainerCaptor = argumentCaptor<View>()
+
+        controller.start()
+
+        whenever(builder.show()).thenReturn(dialog)
+
+        // First dispatch...
+        store.dispatch(ExtensionProcessDisabledPopupAction(showPopup = true))
+        dispatcher.scheduler.advanceUntilIdle()
+        store.waitUntilIdle()
+
+        // Second dispatch... without having dismissed the dialog before!
+        store.dispatch(ExtensionProcessDisabledPopupAction(showPopup = true))
+        dispatcher.scheduler.advanceUntilIdle()
+        store.waitUntilIdle()
+
+        verify(builder).setView(buttonsContainerCaptor.capture())
+        verify(builder, times(1)).show()
+
+        // Click a button to dismiss the dialog.
+        buttonsContainerCaptor.value.findViewById<Button>(R.id.negative).performClick()
+        store.waitUntilIdle()
+    }
 }