Closes mozilla-mobile#7782: Fix crashes in addon installation dialog

csadilek · csadilek · commit 1d858c44ee8c · 2020-07-21T12:00:43.000-04:00
diff --git a/components/feature/addons/src/main/java/mozilla/components/feature/addons/ui/AddonInstallationDialogFragment.kt b/components/feature/addons/src/main/java/mozilla/components/feature/addons/ui/AddonInstallationDialogFragment.kt
@@ -6,6 +6,7 @@ package mozilla.components.feature.addons.ui
 
 import android.annotation.SuppressLint
 import android.app.Dialog
+import android.graphics.Bitmap
 import android.graphics.Color
 import android.graphics.drawable.BitmapDrawable
 import android.graphics.drawable.ColorDrawable
@@ -25,6 +26,7 @@ import androidx.annotation.VisibleForTesting
 import androidx.appcompat.app.AppCompatDialogFragment
 import androidx.appcompat.widget.AppCompatCheckBox
 import androidx.core.content.ContextCompat
+import androidx.fragment.app.FragmentManager
 import kotlinx.android.synthetic.main.mozac_feature_addons_fragment_dialog_addon_installed.view.*
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
@@ -38,26 +40,33 @@ import mozilla.components.support.ktx.android.content.appName
 import mozilla.components.support.ktx.android.content.res.resolveAttribute
 import java.io.IOException
 
+@VisibleForTesting internal const val KEY_INSTALLED_ADDON = "KEY_ADDON"
 private const val KEY_DIALOG_GRAVITY = "KEY_DIALOG_GRAVITY"
 private const val KEY_DIALOG_WIDTH_MATCH_PARENT = "KEY_DIALOG_WIDTH_MATCH_PARENT"
 private const val KEY_CONFIRM_BUTTON_BACKGROUND_COLOR = "KEY_CONFIRM_BUTTON_BACKGROUND_COLOR"
 private const val KEY_CONFIRM_BUTTON_TEXT_COLOR = "KEY_CONFIRM_BUTTON_TEXT_COLOR"
 private const val KEY_CONFIRM_BUTTON_RADIUS = "KEY_CONFIRM_BUTTON_RADIUS"
+@VisibleForTesting internal const val KEY_ICON = "KEY_ICON"
+
 private const val DEFAULT_VALUE = Int.MAX_VALUE
 
 /**
  * A dialog that shows [Addon] installation confirmation.
  */
-class AddonInstallationDialogFragment(
-    private val addonCollectionProvider: AddonCollectionProvider
-) : AppCompatDialogFragment() {
+class AddonInstallationDialogFragment : AppCompatDialogFragment() {
     private val scope = CoroutineScope(Dispatchers.IO)
+    @VisibleForTesting internal var iconJob: Job? = null
     private val logger = Logger("AddonInstallationDialogFragment")
     /**
      * A lambda called when the confirm button is clicked.
      */
     var onConfirmButtonClicked: ((Addon, Boolean) -> Unit)? = null
 
+    /**
+     * Reference to the application's [AddonCollectionProvider] to fetch add-on icons.
+     */
+    var addonCollectionProvider: AddonCollectionProvider? = null
+
     private val safeArguments get() = requireNotNull(arguments)
 
     internal val addon get() = requireNotNull(safeArguments.getParcelable<Addon>(KEY_ADDON))
@@ -91,6 +100,11 @@ class AddonInstallationDialogFragment(
                 DEFAULT_VALUE
             )
 
+    override fun onStop() {
+        super.onStop()
+        iconJob?.cancel()
+    }
+
     override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {
         val sheetDialog = Dialog(requireContext())
         sheetDialog.requestWindowFeature(Window.FEATURE_NO_TITLE)
@@ -144,7 +158,12 @@ class AddonInstallationDialogFragment(
                 requireContext().appName
             )
 
-        fetchIcon(addon, rootView.icon)
+        val icon = safeArguments.getParcelable<Bitmap>(KEY_ICON)
+        if (icon != null) {
+            rootView.icon.setImageDrawable(BitmapDrawable(resources, icon))
+        } else {
+            iconJob = fetchIcon(addon, rootView.icon)
+        }
 
         val allowedInPrivateBrowsing = rootView.findViewById<AppCompatCheckBox>(R.id.allow_in_private_browsing)
         allowedInPrivateBrowsing.setOnCheckedChangeListener { _, isChecked ->
@@ -188,9 +207,10 @@ class AddonInstallationDialogFragment(
     internal fun fetchIcon(addon: Addon, iconView: ImageView, scope: CoroutineScope = this.scope): Job {
         return scope.launch {
             try {
-                val iconBitmap = addonCollectionProvider.getAddonIconBitmap(addon)
+                val iconBitmap = addonCollectionProvider?.getAddonIconBitmap(addon)
                 iconBitmap?.let {
                     scope.launch(Dispatchers.Main) {
+                        safeArguments.putParcelable(KEY_ICON, it)
                         iconView.setImageDrawable(BitmapDrawable(iconView.resources, it))
                     }
                 }
@@ -206,6 +226,19 @@ class AddonInstallationDialogFragment(
         }
     }
 
+    override fun show(manager: FragmentManager, tag: String?) {
+        // This dialog is shown as a result of an async operation (installing
+        // an add-on). Once installation succeeds, the activity may already be
+        // in the process of being destroyed. Since the dialog doesn't have any
+        // state we need to keep, and since it's also fine to not display the
+        // dialog at all in case the user navigates away, we can simply use
+        // commitAllowingStateLoss here to prevent crashing on commit:
+        // https://github.com/mozilla-mobile/android-components/issues/7782
+        val ft = manager.beginTransaction()
+        ft.add(this, tag)
+        ft.commitAllowingStateLoss()
+    }
+
     @Suppress("LongParameterList")
     companion object {
         /**
@@ -224,11 +257,11 @@ class AddonInstallationDialogFragment(
             onConfirmButtonClicked: ((Addon, Boolean) -> Unit)? = null
         ): AddonInstallationDialogFragment {
 
-            val fragment = AddonInstallationDialogFragment(addonCollectionProvider)
+            val fragment = AddonInstallationDialogFragment()
             val arguments = fragment.arguments ?: Bundle()
 
             arguments.apply {
-                putParcelable(KEY_ADDON, addon)
+                putParcelable(KEY_INSTALLED_ADDON, addon)
 
                 promptsStyling?.gravity?.apply {
                     putInt(KEY_DIALOG_GRAVITY, this)
@@ -246,6 +279,7 @@ class AddonInstallationDialogFragment(
             }
             fragment.onConfirmButtonClicked = onConfirmButtonClicked
             fragment.arguments = arguments
+            fragment.addonCollectionProvider = addonCollectionProvider
             return fragment
         }
     }
diff --git a/components/feature/addons/src/test/java/mozilla/components/feature/addons/ui/AddonInstallationDialogFragmentTest.kt b/components/feature/addons/src/test/java/mozilla/components/feature/addons/ui/AddonInstallationDialogFragmentTest.kt
@@ -14,25 +14,32 @@ import androidx.appcompat.widget.AppCompatCheckBox
 import androidx.fragment.app.FragmentManager
 import androidx.fragment.app.FragmentTransaction
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.test.TestCoroutineDispatcher
 import kotlinx.coroutines.test.TestCoroutineScope
 import mozilla.components.feature.addons.Addon
 import mozilla.components.feature.addons.R
 import mozilla.components.feature.addons.amo.AddonCollectionProvider
 import mozilla.components.feature.addons.ui.AddonInstallationDialogFragment
+import mozilla.components.feature.addons.ui.KEY_ICON
+import mozilla.components.feature.addons.ui.KEY_INSTALLED_ADDON
 import mozilla.components.feature.addons.ui.translatedName
 import mozilla.components.support.test.mock
 import mozilla.components.support.test.robolectric.testContext
 import mozilla.components.support.test.rule.MainCoroutineRule
 import mozilla.components.support.test.whenever
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
+import org.junit.Assert.assertSame
 import org.junit.Assert.assertTrue
 import org.junit.Assert.fail
 import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.Mockito
+import org.mockito.Mockito.`when`
 import org.mockito.Mockito.doReturn
 import org.mockito.Mockito.spy
 import org.mockito.Mockito.verify
@@ -51,7 +58,10 @@ class AddonInstallationDialogFragmentTest {
             "id", translatableName = mapOf(Addon.DEFAULT_LOCALE to "my_addon"),
             permissions = listOf("privacy", "<all_urls>", "tabs")
         )
-        val fragment = createAddonInstallationDialogFragment(addon, mock())
+        val mockedCollectionProvider = mock<AddonCollectionProvider>()
+        val fragment = createAddonInstallationDialogFragment(addon, mockedCollectionProvider)
+        assertSame(mockedCollectionProvider, fragment.addonCollectionProvider)
+        assertSame(addon, fragment.arguments?.getParcelable(KEY_INSTALLED_ADDON))
 
         doReturn(testContext).`when`(fragment).requireContext()
         val dialog = fragment.onCreateDialog(null)
@@ -131,7 +141,7 @@ class AddonInstallationDialogFragmentTest {
     }
 
     @Test
-    fun `fetching the add-on icon() successfully `() {
+    fun `fetching the add-on icon successfully`() {
         val addon = mock<Addon>()
         val bitmap = mock<Bitmap>()
         val mockedImageView = spy(ImageView(testContext))
@@ -140,13 +150,15 @@ class AddonInstallationDialogFragmentTest {
 
         runBlocking {
             whenever(mockedCollectionProvider.getAddonIconBitmap(addon)).thenReturn(bitmap)
+            assertNull(fragment.arguments?.getParcelable<Bitmap>(KEY_ICON))
             fragment.fetchIcon(addon, mockedImageView, scope).join()
+            assertNotNull(fragment.arguments?.getParcelable<Bitmap>(KEY_ICON))
             verify(mockedImageView).setImageDrawable(Mockito.any())
         }
     }
 
     @Test
-    fun `handle errors while fetching the add-on icon() `() {
+    fun `handle errors while fetching the add-on icon`() {
         val addon = mock<Addon>()
         val mockedImageView = spy(ImageView(testContext))
         val mockedCollectionProvider = mock<AddonCollectionProvider>()
@@ -165,6 +177,32 @@ class AddonInstallationDialogFragmentTest {
         }
     }
 
+    @Test
+    fun `allows state loss when committing`() {
+        val addon = mock<Addon>()
+        val mockedCollectionProvider = mock<AddonCollectionProvider>()
+        val fragment = createAddonInstallationDialogFragment(addon, mockedCollectionProvider)
+
+        val fragmentManager = mock<FragmentManager>()
+        val fragmentTransaction = mock<FragmentTransaction>()
+        `when`(fragmentManager.beginTransaction()).thenReturn(fragmentTransaction)
+
+        fragment.show(fragmentManager, "test")
+        verify(fragmentTransaction).commitAllowingStateLoss()
+    }
+
+    @Test
+    fun `cancels icon job on stop`() {
+        val addon = mock<Addon>()
+        val mockedCollectionProvider = mock<AddonCollectionProvider>()
+        val fragment = createAddonInstallationDialogFragment(addon, mockedCollectionProvider)
+
+        val job = mock<Job>()
+        fragment.iconJob = job
+        fragment.onStop()
+        verify(job).cancel()
+    }
+
     private fun createAddonInstallationDialogFragment(
         addon: Addon,
         addonCollectionProvider: AddonCollectionProvider,
diff --git a/samples/browser/src/main/java/org/mozilla/samples/browser/addons/AddonsFragment.kt b/samples/browser/src/main/java/org/mozilla/samples/browser/addons/AddonsFragment.kt
@@ -58,8 +58,13 @@ class AddonsFragment : Fragment(), AddonsManagerAdapterDelegate {
             bindRecyclerView(view)
         }
 
-        findPreviousDialogFragment()?.let { dialog ->
-            dialog.onPositiveButtonClicked = onPositiveButtonClicked
+        findPreviousPermissionDialogFragment()?.let { dialog ->
+            dialog.onPositiveButtonClicked = onConfirmPermissionButtonClicked
+        }
+
+        findPreviousInstallationDialogFragment()?.let { dialog ->
+            dialog.onConfirmButtonClicked = onConfirmInstallationButtonClicked
+            dialog.addonCollectionProvider = requireContext().components.addonCollectionProvider
         }
     }
 
@@ -121,21 +126,25 @@ class AddonsFragment : Fragment(), AddonsManagerAdapterDelegate {
     }
 
     private fun isAlreadyADialogCreated(): Boolean {
-        return findPreviousDialogFragment() != null
+        return findPreviousPermissionDialogFragment() != null && findPreviousInstallationDialogFragment() != null
     }
 
-    private fun findPreviousDialogFragment(): PermissionsDialogFragment? {
+    private fun findPreviousPermissionDialogFragment(): PermissionsDialogFragment? {
         return fragmentManager?.findFragmentByTag(PERMISSIONS_DIALOG_FRAGMENT_TAG) as? PermissionsDialogFragment
     }
 
+    private fun findPreviousInstallationDialogFragment(): AddonInstallationDialogFragment? {
+        return fragmentManager?.findFragmentByTag(INSTALLATION_DIALOG_FRAGMENT_TAG) as? AddonInstallationDialogFragment
+    }
+
     private fun showPermissionDialog(addon: Addon) {
         if (isInstallationInProgress) {
             return
         }
 
         val dialog = PermissionsDialogFragment.newInstance(
             addon = addon,
-            onPositiveButtonClicked = onPositiveButtonClicked
+            onPositiveButtonClicked = onConfirmPermissionButtonClicked
         )
 
         if (!isAlreadyADialogCreated() && fragmentManager != null) {
@@ -151,32 +160,36 @@ class AddonsFragment : Fragment(), AddonsManagerAdapterDelegate {
         val dialog = AddonInstallationDialogFragment.newInstance(
             addon = addon,
             addonCollectionProvider = addonCollectionProvider,
-            onConfirmButtonClicked = { _, allowInPrivateBrowsing ->
-                if (allowInPrivateBrowsing) {
-                    requireContext().components.addonManager.setAddonAllowedInPrivateBrowsing(
-                        addon,
-                        allowInPrivateBrowsing
-                    )
-                }
-            }
+            onConfirmButtonClicked = onConfirmInstallationButtonClicked
         )
 
         if (!isAlreadyADialogCreated() && fragmentManager != null) {
             dialog.show(requireFragmentManager(), INSTALLATION_DIALOG_FRAGMENT_TAG)
         }
     }
 
-    private val onPositiveButtonClicked: ((Addon) -> Unit) = { addon ->
+    private val onConfirmInstallationButtonClicked: ((Addon, Boolean) -> Unit) = { addon, allowInPrivateBrowsing ->
+        if (allowInPrivateBrowsing) {
+            requireContext().components.addonManager.setAddonAllowedInPrivateBrowsing(
+                addon,
+                allowInPrivateBrowsing
+            )
+        }
+    }
+
+    private val onConfirmPermissionButtonClicked: ((Addon) -> Unit) = { addon ->
         addonProgressOverlay.visibility = View.VISIBLE
         isInstallationInProgress = true
 
         val installOperation = requireContext().components.addonManager.installAddon(
             addon,
-            onSuccess = {
-                adapter?.updateAddon(it)
-                addonProgressOverlay.visibility = View.GONE
-                isInstallationInProgress = false
-                showInstallationDialog(it)
+            onSuccess = { installedAddon ->
+                context?.let {
+                    adapter?.updateAddon(installedAddon)
+                    addonProgressOverlay.visibility = View.GONE
+                    isInstallationInProgress = false
+                    showInstallationDialog(installedAddon)
+                }
             },
             onError = { _, e ->
                 // No need to display an error message if installation was cancelled by the user.
