PopUpTo to parent NavGraph crashes when child nav-graph has non-nullable arguments [249988437]

Fixed

Bug

Status Update

No update yet.

Description

da...@sucharda.cz

created issue #1

Sep 30, 2022 02:32PM

Component used: Navigation
Version used: 2.5.1 and 2.5.2
Last known working version: 2.3.5
Devices/Android versions reproduced on: Huawei P20 Light / AN 9 (EMUI 9.1.0)

So our navigation is kinda complex so I will try to simplify the configuration. If there is a nested graph with arguments which cannot be null (and no default value is supplied) and popUpTo is used to remove nested graph during destination (nested) navigation it tries to recreate nested graph as a parent for the destination being navigated to.

<navigation xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    android:id="@+id/parentNavGraph"
    app:startDestination="@id/parentFragment">

    <fragment
        android:id="@+id/parentFragment"
        android:label="parentFragment"
        tools:layout="@layout/fragment_parent">

        <action
            android:id="@+id/actionToChildGraph"
            app:destination="@id/childNavGraph"/>

    </fragment>

    <navigation
        android:id="@+id/childNavGraph"
        app:startDestination="@id/childFragment">

        // Note: this is a duplicate due to NavArgs since it will not see "childFragment" args
        <argument
            android:name="argumentLong"
            app:argType="long"
            app:nullable="false"/>

        <fragment
            android:id="@+id/childFragment"
            android:label="childFragment"
            tools:layout="@layout/fragment_child">

            <argument
                android:name="argumentLong"
                app:argType="long"
                app:nullable="false"/>

            <action
                android:id="@+id/actionToChildFragment2"
                app:destination="@id/childFragment2"
                app:popUpTo="@id/childNavgraph"
                app:popUpToInclusive="true"/>

        </fragment>

        <fragment
            android:id="@+id/childFragment2"
            android:label="childFragment2"
            tools:layout="@layout/fragment_child_2"/>

</navigation>

So now if the navigation is triggered like this:

findNavController().navigate(
  ChildFragmentDirections.actionToChildFragment2()
)

It will pop fragments from back stack, but it will not change parent for ChildFragment2 which will be set to childNavGraph and since it is not in the back stack anymore it will try to recreate it with default values. But argumentLong is not nullable and has no default value so it will crash:

// NOTE: this crash is from Navigation version 2.5.1
java.lang.NullPointerException: null cannot be cast to non-null type kotlin.Long
  at androidx.navigation.NavType$Companion$LongType$1.get(NavType.kt:352)
  at androidx.navigation.NavType$Companion$LongType$1.get(NavType.kt:342)
  at androidx.navigation.NavArgument.verify(NavArgument.kt:76)
  at androidx.navigation.NavDestination.addInDefaultArgs(NavDestination.kt:502)
  at androidx.navigation.NavController.addEntryToBackStack(NavController.kt:1865)
  at androidx.navigation.NavController.addEntryToBackStack$default(NavController.kt:1813)
  at androidx.navigation.NavController$navigate$4.invoke(NavController.kt:1721)
  at androidx.navigation.NavController$navigate$4.invoke(NavController.kt:1719)
  at androidx.navigation.NavController$NavControllerNavigatorState.push(NavController.kt:287)
  at androidx.navigation.fragment.FragmentNavigator.navigate(FragmentNavigator.kt:246)
  at androidx.navigation.fragment.FragmentNavigator.navigate(FragmentNavigator.kt:162)
  at androidx.navigation.NavController.navigateInternal(NavController.kt:260)
  at androidx.navigation.NavController.navigate(NavController.kt:1719)
  at androidx.navigation.NavController.navigate(NavController.kt:1545)
  at androidx.navigation.NavController.navigate(NavController.kt:1472)
  at androidx.navigation.NavController.navigate(NavController.kt:1930)

Note: Interesting thing is that it does not crash with non-nullable String argument for some reason (ah I see, it is because String NavType is nullable by default: StringType: NavType<String?> vs LongType: NavType<Long>).

Possible fixes for us:

Do not use popUpTo to pop parent (navGraph) of a Fragment (node) from XML but use popBackStack from code instead.
Add default value to argumentLong.

Basically I am not sure if this an intended behavior or a bug. Gonna mark it as a bug because it worked in 2.3.5. I understand that Destination should have a parent but what is the reason to create a parent that does not exist and will never be displayed in this case, since it was already poped and thus it should not be displayed to the user.

At the moment two solutions come to mind:

Change new Fragment (node) parent to last destination in back stack after poping. Here I am not really sure if parent is designed this way and if it can be done without breaking something else.
Make LongType and other types nullable same as StringType. Again not sure if this can be done and it probably is not a proper solution (change).

There might of course be other solutions that I cannot see.

Thank you for your time and help.

Comments

da...@sucharda.cz <da...@sucharda.cz> #2Sep 30, 2022 02:38PM

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> #3Sep 30, 2022 04:37PM

Please provide a minimal sample project along with the minimal steps to recreate the issue in the project.

da...@sucharda.cz <da...@sucharda.cz> #4Oct 20, 2022 04:26PM

Sorry for the delay. I got a working example here: https://github.com/Del-S/issues. The project is set up to crash when navigating to ThirdFragment (nested nav graph) with poping the graph.

I poked into it a little bit and it seems to be connected to SafeArgs gradle plugin. When safe args is used to navigate to ThirdFragment (with nested graph poped) it will try to recreate te parent but there were no arguments passed so it crashes.

Steps to reproduce:

Navigate from Second Fragment to Child nav graph (with non-nullable parameters).
Navigate to Third fragment using SafeArgs and the app crashes.

Crash log:

Process: cz.dels.issues, PID: 1743
java.lang.NullPointerException: null cannot be cast to non-null type kotlin.Long
at androidx.navigation.NavType$Companion$LongType$1.get(NavType.kt:352)
at androidx.navigation.NavType$Companion$LongType$1.get(NavType.kt:342)
at androidx.navigation.NavArgument.verify(NavArgument.kt:76)
at androidx.navigation.NavDestination.addInDefaultArgs(NavDestination.kt:502)
at androidx.navigation.NavController.addEntryToBackStack(NavController.kt:1865)
at androidx.navigation.NavController.addEntryToBackStack$default(NavController.kt:1813)
at androidx.navigation.NavController$navigate$4.invoke(NavController.kt:1721)
at androidx.navigation.NavController$navigate$4.invoke(NavController.kt:1719)
at androidx.navigation.NavController$NavControllerNavigatorState.push(NavController.kt:287)
at androidx.navigation.fragment.FragmentNavigator.navigate(FragmentNavigator.kt:246)
at androidx.navigation.fragment.FragmentNavigator.navigate(FragmentNavigator.kt:162)
at androidx.navigation.NavController.navigateInternal(NavController.kt:260)
at androidx.navigation.NavController.navigate(NavController.kt:1719)
at androidx.navigation.NavController.navigate(NavController.kt:1545)
at androidx.navigation.NavController.navigate(NavController.kt:1472)
at androidx.navigation.NavController.navigate(NavController.kt:1930)
at cz.dels.issues.SecondFragment.onViewCreated$lambda-0(SecondFragment.kt:38)
at cz.dels.issues.SecondFragment.$r8$lambda$XDYnOS_cYrafiNQ5rcCu1WCn0IE(Unknown Source:0)
at cz.dels.issues.SecondFragment$$ExternalSyntheticLambda0.onClick(Unknown Source:2)

Note: If in step 2 SaveArgs is not used then navigation works correctly. More information is here: https://github.com/Del-S/issues/blob/master/app/src/main/java/cz/dels/issues/SecondFragment.kt#L37.

da...@sucharda.cz <da...@sucharda.cz> #5Oct 20, 2022 04:30PM

Ups a typo: Navigate from ~~Second~~ First Fragment to Child nav graph (with non-nullable parameters).

Note: sorry for the spam but I am not able to edit my own comment.

jb...@google.com <jb...@google.com> Nov 8, 2022 05:32PM

Assigned to cl...@google.com.

cl...@google.com <cl...@google.com> #6Mar 15, 2023 08:32PM

Marked as fixed.

This has been fixed and will be available in navigation 2.6.0-alpha08

ap...@google.com <ap...@google.com> #7Mar 15, 2023 08:32PM

Project: platform/frameworks/support
Branch: androidx-main

commit 6b358154b794a0456b089ac8e548bfb830dd6c22
Author: Clara Fok <clarafok@google.com>
Date: Tue Mar 14 17:56:12 2023

Fix missing non-nullable arg when rebuilding hierarchy

When navigating with NavDirections, args is populated with an empty bundle. This causes issue when we rebuild parent hierarchy while adding a new entry to NavBackStack. If the Entry being rebuilt contains a non-nullalbe arg, i.e. Long, this empty bundle will cause an exception.

Test: ./gradlew navigation:navigation-runtime:cC
Bug: 249988437
Change-Id: I5c8ce739ad9a3428c8a8de13eae391bfff0db5df

M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerTest.kt
M navigation/navigation-runtime/src/main/java/androidx/navigation/NavController.kt

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

pr...@google.com <pr...@google.com> #8Mar 22, 2023 08:23PM

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

androidx.navigation:navigation-runtime:2.6.0-alpha08