Fix FragmentManager and pendingOps race
FragmentNavigator adds pendingOps after triggering FragmentManager pop. This leads to a race where sometimes pendingOps are added after onBackStackChangeStarted callback, when ops should be added prior this callback. Now we add pendingOps before triggering FragmentManager pop.
Test: ./gradlew navigator:navigator-fragment:cC
Fixes: 326239315
Change-Id: I0259067c4ffed25c886d652c890b9ff7aeb0c47c
diff --git a/navigation/navigation-fragment/src/androidTest/java/androidx/navigation/fragment/FragmentNavigatorTest.kt b/navigation/navigation-fragment/src/androidTest/java/androidx/navigation/fragment/FragmentNavigatorTest.kt
index 4b9ad03..13c20d9 100644
--- a/navigation/navigation-fragment/src/androidTest/java/androidx/navigation/fragment/FragmentNavigatorTest.kt
+++ b/navigation/navigation-fragment/src/androidTest/java/androidx/navigation/fragment/FragmentNavigatorTest.kt
@@ -490,23 +490,27 @@
.isEqualTo(2)
assertThat(navigatorState.transitionsInProgress.value).isEmpty()
assertThat(fragmentNavigator.pendingOps.entries()).isEmpty()
-
+ // obs for upcoming pop
+ var pendingOps1: List<String> = emptyList()
fragmentNavigator.pendingOps.onBackStackChangedStarted {
- assertThat(it.entries()).containsExactly(entry2.id, entry3.id)
+ pendingOps1 = it.entries().toList()
}
// first pop entry3
fragmentNavigator.popBackStack(entry3, false)
+ assertThat(pendingOps1).containsExactly(entry2.id, entry3.id)
activityRule.runOnUiThread {
fragmentManager.executePendingTransactions()
assertThat(fragmentNavigator.pendingOps.entries()).isEmpty()
}
-
+ // obs for upcoming pop
+ var pendingOps2: List<String> = emptyList()
fragmentNavigator.pendingOps.onBackStackChangedStarted {
- assertThat(it.entries()).containsExactly(entry1.id, entry2.id)
+ pendingOps2 = it.entries().toList()
}
// And then pop entry2
fragmentNavigator.popBackStack(entry2, false)
+ assertThat(pendingOps2).containsExactly(entry1.id, entry2.id)
activityRule.runOnUiThread {
fragmentManager.executePendingTransactions()
assertThat(fragmentNavigator.pendingOps.entries()).isEmpty()
@@ -1729,24 +1733,26 @@
}
})
}
-
+ // obs for upcoming pop
+ var pendingOps1: List<String> = emptyList()
fragmentNavigator.pendingOps.onBackStackChangedStarted {
- assertThat(it.entries()).containsExactly(entry2.id, entry3.id)
+ pendingOps1 = it.entries().toList()
}
fragmentNavigator.popBackStack(entry3, false)
+ assertThat(pendingOps1).containsExactly(entry2.id, entry3.id)
assertThat(fragmentNavigator.pendingOps.entries()).isEmpty()
-
+ // obs for upcoming pop
+ var pendingOps2: List<String> = emptyList()
fragmentNavigator.pendingOps.onBackStackChangedStarted {
- assertThat(it.entries()).containsExactly(entry1.id, entry2.id)
+ pendingOps2 = it.entries().toList()
}
fragmentNavigator.popBackStack(entry2, false)
-
assertThat(navigatorState.backStack.value).containsExactly(entry1)
activityRule.runOnUiThread {
fragmentManager.executePendingTransactions()
assertThat(fragmentNavigator.pendingOps.entries()).isEmpty()
}
-
+ assertThat(pendingOps2).containsExactly(entry1.id, entry2.id)
val fragment1 = fragmentManager.findFragmentById(R.id.container) as AnimatorFragment
assertWithMessage("Fragment should be added")
.that(fragment1)
diff --git a/navigation/navigation-fragment/src/main/java/androidx/navigation/fragment/FragmentNavigator.kt b/navigation/navigation-fragment/src/main/java/androidx/navigation/fragment/FragmentNavigator.kt
index f58d911..80a18ce 100644
--- a/navigation/navigation-fragment/src/main/java/androidx/navigation/fragment/FragmentNavigator.kt
+++ b/navigation/navigation-fragment/src/main/java/androidx/navigation/fragment/FragmentNavigator.kt
@@ -305,6 +305,22 @@
beforePopList.size
)
val initialEntry = beforePopList.first()
+
+ // add pending ops here before any animation (if present) or FragmentManager work starts
+ val incomingEntry = beforePopList.elementAtOrNull(popUpToIndex - 1)
+ if (incomingEntry != null) {
+ addPendingOps(incomingEntry.id)
+ }
+ poppedList.filter { entry ->
+ // normally we don't add initialEntry to pending ops because the adding/popping
+ // of an isolated fragment does not trigger onBackStackCommitted. But if initial
+ // entry was already added to pendingOps, it was likely an incomingEntry that now
+ // needs to be popped, so we need to overwrite isPop to true here.
+ pendingOps.asSequence().map { it.first }.contains(entry.id) ||
+ entry.id != initialEntry.id
+ }.forEach { entry ->
+ addPendingOps(entry.id, isPop = true)
+ }
if (savedState) {
// Now go through the list in reversed order (i.e., started from the most added)
// and save the back stack state of each.
@@ -333,22 +349,6 @@
)
}
- val incomingEntry = beforePopList.elementAtOrNull(popUpToIndex - 1)
- if (incomingEntry != null) {
- addPendingOps(incomingEntry.id)
- }
- // add pending ops here before any animation (if present) starts
- poppedList.filter { entry ->
- // normally we don't add initialEntry to pending ops because the adding/popping
- // of an isolated fragment does not trigger onBackStackCommitted. But if initial
- // entry was already added to pendingOps, it was likely an incomingEntry that now
- // needs to be popped, so we need to overwrite isPop to true here.
- pendingOps.asSequence().map { it.first }.contains(entry.id) ||
- entry.id != initialEntry.id
- }.forEach { entry ->
- addPendingOps(entry.id, isPop = true)
- }
-
state.popWithTransition(popUpTo, savedState)
}
