Ensure FragmentNavigator does not destroy entries early

There is a case where if you navigate to a destination and the fragment
that is assocated with the destination does a navigate with popUpTo in
its onResume() lifecycle callback, the FragmentNavigator incorrectly
destroys the entry before the associated fragment is destroyed. The
reason for this is the timing between fragment lifecycle callbacks and
Lifecycle. The onResume() lifecycle callback happens immediately before
the `ON_RESUME` Lifecycle event and once the lifecycle listeners in
FragmentNavigator get the `ON_RESUME` event, the state of the entry has
changed and gets destroyed.

Instead of always marking entries as complete when we get the end
Lifecycle events (ON_RESUME and ON_DESTROY) from the fragment, we should
check whether the back stack contains the entry or not.

RelNote: "Fixed an issue in Navigation with fragments where removing a
fragment via navigate with popUpTo in its `onResume()` lifecycle
callback would cause an `IllegalStateException`."
Test: added test
Bug: 279644470

Change-Id: I21884c268cf213caacabe19db38fd49a1ac829fa

Author: Jeremy Woods

navigation/navigation-fragment/src/androidTest/java/androidx/navigation/fragment/NavControllerWithFragmentTest.kt

@@ -24,6 +24,8 @@ import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.ViewModelProvider
 import androidx.navigation.NavOptions
+import androidx.navigation.createGraph
+import androidx.navigation.fragment.test.EmptyFragment
 import androidx.navigation.fragment.test.NavigationActivity
 import androidx.navigation.fragment.test.R
 import androidx.navigation.navOptions
@@ -284,6 +286,20 @@ class NavControllerWithFragmentTest {
         assertThat(fm.fragments.size).isEqualTo(2) // start + last dialog fragment
     }
 
+    @Test
+    fun testPopEntryInFragmentResumed() = withNavigationActivity {
+        navController.graph = navController.createGraph("first") {
+            fragment<EmptyFragment>("first")
+            fragment<PopInOnResumeFragment>("second")
+        }
+        navController.navigate("second")
+
+        val fm = supportFragmentManager.findFragmentById(R.id.nav_host)?.childFragmentManager
+        fm?.executePendingTransactions()
+
+        assertThat(navController.currentBackStackEntry?.destination?.route).isEqualTo("first")
+    }
+
     private fun withNavigationActivity(
         block: NavigationActivity.() -> Unit
     ) {
@@ -295,6 +311,15 @@ class NavControllerWithFragmentTest {
     }
 }
 
+class PopInOnResumeFragment : Fragment(R.layout.strict_view_fragment) {
+    override fun onResume() {
+        super.onResume()
+        findNavController().navigate("first") {
+            popUpTo("first")
+        }
+    }
+}
+
 class TestDialogFragment : DialogFragment() {
     val dialogs = mutableListOf<Dialog>()
 

navigation/navigation-fragment/src/main/java/androidx/navigation/fragment/FragmentNavigator.kt

@@ -29,7 +29,6 @@ import androidx.fragment.app.FragmentManager.OnBackStackChangedListener
 import androidx.fragment.app.FragmentTransaction
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.LifecycleEventObserver
-import androidx.lifecycle.LifecycleOwner
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.ViewModelProvider
 import androidx.lifecycle.viewmodel.CreationExtras
@@ -75,38 +74,34 @@ public open class FragmentNavigator(
      */
     internal val backStack get() = state.backStack
 
-    private val fragmentObserver = object : LifecycleEventObserver {
-        override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) {
-            if (event == Lifecycle.Event.ON_DESTROY) {
-                val fragment = source as Fragment
-                val entry = state.transitionsInProgress.value.lastOrNull { entry ->
-                    entry.id == fragment.tag
-                }
-                entry?.let {
-                    entriesToPop.remove(entry.id)
-                    state.markTransitionComplete(it)
+    private val fragmentObserver = LifecycleEventObserver { source, event ->
+        if (event == Lifecycle.Event.ON_DESTROY) {
+            val fragment = source as Fragment
+            val entry = state.transitionsInProgress.value.lastOrNull { entry ->
+                entry.id == fragment.tag
+            }
+            if (entry != null) {
+                entriesToPop.remove(entry.id)
+                if (!state.backStack.value.contains(entry)) {
+                    state.markTransitionComplete(entry)
                 }
-                fragment.lifecycle.removeObserver(this)
             }
         }
     }
 
     private val fragmentViewObserver = { entry: NavBackStackEntry ->
-        object : LifecycleEventObserver {
-            override fun onStateChanged(
-                source: LifecycleOwner,
-                event: Lifecycle.Event
-            ) {
-                // Once the lifecycle reaches RESUMED, we can mark the transition complete
-                if (event == Lifecycle.Event.ON_RESUME) {
-                    state.markTransitionComplete(entry)
-                }
-                // Once the lifecycle reaches DESTROYED, we can mark the transition complete and
-                // remove the observer.
-                if (event == Lifecycle.Event.ON_DESTROY) {
-                    entriesToPop.remove(entry.id)
+        LifecycleEventObserver { _, event ->
+            // Once the lifecycle reaches RESUMED, if the entry is in the back stack we can mark
+            // the transition complete
+            if (event == Lifecycle.Event.ON_RESUME && state.backStack.value.contains(entry)) {
+                state.markTransitionComplete(entry)
+            }
+            // Once the lifecycle reaches DESTROYED, if the entry is not in the back stack, we can
+            // mark the transition complete
+            if (event == Lifecycle.Event.ON_DESTROY) {
+                entriesToPop.remove(entry.id)
+                if (!state.backStack.value.contains(entry)) {
                     state.markTransitionComplete(entry)
-                    source.lifecycle.removeObserver(this)
                 }
             }
         }