Merge mozilla-mobile#4143

4143: CCloses mozilla-mobile#4125: Fragment.consumeFrom(): Temporarily check whether fragment is added. r=Amejia481 a=pocmo



Co-authored-by: Sebastian Kaspari <[email protected]>

Author: MickeyMoz

components/lib/state/src/main/java/mozilla/components/lib/state/ext/Fragment.kt

@@ -28,12 +28,33 @@ import mozilla.components.support.ktx.android.view.toScope
 @ExperimentalCoroutinesApi // Channel
 @ObsoleteCoroutinesApi // consumeEach
 fun <S : State, A : Action> Fragment.consumeFrom(store: Store<S, A>, block: (S) -> Unit) {
+    val fragment = this
     val view = checkNotNull(view) { "Fragment has no view yet. Call from onViewCreated()." }
 
     val scope = view.toScope()
     val channel = store.channel(owner = this)
 
     scope.launch {
-        channel.consumeEach { state -> block(state) }
+        channel.consumeEach { state ->
+            // We are using a scope that is bound to the view being attached here. It can happen
+            // that the "view detached" callback gets executed *after* the fragment was detached. If
+            // a `consumeFrom` runs in exactly this moment then we run inside a detached fragment
+            // without a `Context` and this can cause a variety of issues/crashes.
+            // See: https://github.com/mozilla-mobile/android-components/issues/4125
+            //
+            // To avoid this, we check whether the fragment is added. If it's not added then we run
+            // in exactly that moment between fragment detach and view detach.
+            // It would be better if we could use `viewLifecycleOwner` which is bound to
+            // onCreateView() and onDestroyView() of the fragment. But:
+            // - `viewLifecycleOwner` is only available in alpha versions of AndroidX currently.
+            // - We found a bug where `viewLifecycleOwner.lifecycleScope` is not getting cancelled
+            //   causing this coroutine to run forever.
+            //   See: https://github.com/mozilla-mobile/android-components/issues/3828
+            // Once those two issues get resolved we can remove the `isAdded` check and use
+            // `viewLifecycleOwner.lifecycleScope` instead of the view scope.
+            if (fragment.isAdded) {
+                block(state)
+            }
+        }
     }
 }

components/lib/state/src/test/java/mozilla/components/lib/state/ext/FragmentKtTest.kt

@@ -65,6 +65,7 @@ class FragmentKtTest {
         )
 
         val fragment: Fragment = mock()
+        doReturn(true).`when`(fragment).isAdded
 
         val view = View(testContext)
         val activity = Robolectric.buildActivity(Activity::class.java).create().get()
@@ -114,4 +115,68 @@ class FragmentKtTest {
         assertFalse(latch.await(1, TimeUnit.SECONDS))
         assertEquals(26, receivedValue)
     }
+
+    @Test
+    @Synchronized
+    @ExperimentalCoroutinesApi // consumeFrom
+    @ObsoleteCoroutinesApi // consumeFrom
+    fun `consumeFrom does not run when fragment is not added`() {
+        val owner = MockedLifecycleOwner(Lifecycle.State.STARTED)
+
+        val store = Store(
+                TestState(counter = 23),
+                ::reducer
+        )
+
+        val fragment: Fragment = mock()
+
+        val view = View(testContext)
+        val activity = Robolectric.buildActivity(Activity::class.java).create().get()
+        activity.windowManager.addView(view, WindowManager.LayoutParams(100, 100))
+        assertTrue(view.isAttachedToWindow)
+        doReturn(view).`when`(fragment).view
+        doReturn(owner.lifecycle).`when`(fragment).lifecycle
+
+        doReturn(true).`when`(fragment).isAdded
+
+        var receivedValue = 0
+        var latch = CountDownLatch(1)
+
+        fragment.consumeFrom(store) { state ->
+            receivedValue = state.counter
+            latch.countDown()
+        }
+
+        assertTrue(latch.await(1, TimeUnit.SECONDS))
+        assertEquals(23, receivedValue)
+
+        latch = CountDownLatch(1)
+        store.dispatch(TestAction.IncrementAction).joinBlocking()
+        assertTrue(latch.await(1, TimeUnit.SECONDS))
+        assertEquals(24, receivedValue)
+
+        latch = CountDownLatch(1)
+        store.dispatch(TestAction.IncrementAction).joinBlocking()
+        assertTrue(latch.await(1, TimeUnit.SECONDS))
+        assertEquals(25, receivedValue)
+
+        doReturn(false).`when`(fragment).isAdded
+
+        latch = CountDownLatch(1)
+        store.dispatch(TestAction.IncrementAction).joinBlocking()
+        assertFalse(latch.await(1, TimeUnit.SECONDS))
+        assertEquals(25, receivedValue)
+
+        latch = CountDownLatch(1)
+        store.dispatch(TestAction.IncrementAction).joinBlocking()
+        assertFalse(latch.await(1, TimeUnit.SECONDS))
+        assertEquals(25, receivedValue)
+
+        doReturn(true).`when`(fragment).isAdded
+
+        latch = CountDownLatch(1)
+        store.dispatch(TestAction.IncrementAction).joinBlocking()
+        assertTrue(latch.await(1, TimeUnit.SECONDS))
+        assertEquals(28, receivedValue)
+    }
 }