Exception when using nested navigation graphs [369616172]

Fixed

Bug

Status Update

No update yet.

Description

ko...@jetbrains.com

created issue #1

Sep 26, 2024 09:06AM

Component used: Navigation
Version used: 2.8.1

@Composable
internal fun App() = AppTheme {
    Column(
        modifier = Modifier.fillMaxSize(),
        verticalArrangement = Arrangement.Center
    ) {
        val navController = rememberNavController()
        val innerNavController = rememberNavController()
        NavHost(navController, startDestination = "first") {
            composable("first") {
                //val innerNavController = rememberNavController()
                NavHost(innerNavController, "nested1") {
                    composable("nested1") { Text("nested1") }
                    composable("nested2") { Text("nested2") }
                }
            }
            composable("second") { Text("second") }
        }
        Button(onClick = {
            navController.navigate("second") {
                popUpTo("first") {
                    inclusive = true
                    saveState = true
                }
            }
        }) {
            Text("second")
        }
        Button(onClick = {
            navController.navigate("first") {
                restoreState = true
                popUpTo("second") { inclusive = true }
            }
        }) {
            Text("first")
        }
    }
}

Steps:

navigate to Second
navigate to First there will be an exception:

You cannot access the NavBackStackEntry's ViewModels after the NavBackStackEntry is destroyed.

I move the innerNavController inside composable("first") it fixes the problem.

The problem easy can be reproduced in androidx-main navigation tests:

open androidx.navigation.compose.NavHostTest#nestedNavHostRestore
add composeTestRule.waitForIdle() to the end of the test
run it on a device

Comments

il...@google.com <il...@google.com> Sep 26, 2024 10:44PM

Assigned to jb...@google.com.

ap...@google.com <ap...@google.com> #2Oct 1, 2024 05:56AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

jb...@google.com <jb...@google.com> #3Oct 1, 2024 08:22AM

After some investigation here, we decided that it was best to improve the error message around this situation instead of supporting this particular use case.

While the goal of moving the innerNavController away from the inner NavHost is to hoist the state, the Lifecycle of that innerNavController is still set to the LocalLifecycleOwner, which in this case is the first NavBackStackEntry. When you navigate and pop first off of the back stack, you move the state of the innerNavController down to DESTROYED which is a "terminal" state and there is no good way for us to recover from that.

The state has all been DESTROYED so we can't reuse any of the same entries that were previously on the back stack of the innerNavController. Maybe we could make new instances of all of the DESTROYED entries and put them back on the back stack, but then you didn't gain anything from the hoisting cause we would still lose all of it. We could forget about the state, clear the back stack and start over from scratch, but is that would users would expect when they return back to the nested Nav if that were deep in the stack?

For these reasons, we decided to go with the more defined behavior of throwing an error and just letting users know that their NavController is out living the NavHost.

If developers are okay with the last option of clearing the entire stack if the controller is destroyed, doing that manually is still supported and will not throw the new error.

jb...@google.com <jb...@google.com> #4Oct 1, 2024 08:22AM

Marked as fixed.

This has been fixed internally and will be available in the Navigation 2.8.3 release.

na...@google.com <na...@google.com> #5Oct 16, 2024 05:48PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.navigation:navigation-compose:2.9.0-alpha01

jb...@google.com <jb...@google.com> #6Oct 22, 2024 01:17PM

This missed the 2.8.3 release and this will actually be part of navigation 2.8.4.

na...@google.com <na...@google.com> #7Nov 13, 2024 06:19PM