In DefaultFragmentExecutor.open, fromContext childFragmentManager can be null

[mgod]

I'm seeing some issues around this line in the library:

Enro/enro-core/src/main/java/dev/enro/core/fragment/DefaultFragmentExecutor.kt

Line 40 in 5464e1f

fromContext.childFragmentManager,

I haven't been able to find reproduction steps yet, but I'm wondering if it's related to switching this from commitNow to commit:

Enro/enro-core/src/main/java/dev/enro/core/fragment/DefaultFragmentExecutor.kt

Line 75 in 5464e1f

host.fragmentManager.commit {

It seems like I run into this in cases where I have several navigation instructions happening in a row, so it seems like maybe child keys are starting to execute before the parent key transaction has finished. I'm inclined to do some experimenting with swapping this back to commitNow and see if that stops this crash in production, but I can't figure out why this changed. Was there a bug when using commitNow or is it just avoiding using the slower call?

[isaac-udy]

Hi, apologies for taking so long to respond to this issue. I believe I changed from commitNow to commit due to some bugs with commitNow. I can't remember exactly what the issue was (and now I'm kicking myself for not writing better commit messages!).

Changing back to commitNow might solve your issues. It may also be worthwhile looking into tryExecutePendingTransitions and perhaps changing that so that it also does the re-post of the instruction in cases where childFragmentManager is null.

Unfortunately, I'm busy for the next few days and won't get a chance to dig deeper into the issue until the weekend. Over the weekend, I'm intended on refreshing myself on why I changed to commit as opposed to commitNow. I'll also see if I can reproduce a case where childFragmentManager is null - if you have any luck with reproduction steps I'd be very interested in those. Any code that you can share would also be useful.

[isaac-udy]

@mgod do you have any stack traces you'd be able to share?

[mgod]

Fatal Exception: java.lang.IllegalStateException: Fragment [REDACTED]{ef615f2} (706e9004-00ad-418d-8d45-d0c2a3029f45) has not been attached yet.
       at androidx.fragment.app.Fragment.getChildFragmentManager(Fragment.java:1075)
       at dev.enro.core.FragmentContext.getChildFragmentManager(FragmentContext.java:46)
       at dev.enro.core.fragment.DefaultFragmentExecutor.open(DefaultFragmentExecutor.java:40)
       at dev.enro.core.controller.NavigationController.open$enro_core_release(NavigationController.java:51)
       at dev.enro.core.internal.handle.NavigationHandleViewModel.executePendingInstruction(NavigationHandleViewModel.java:90)
       at dev.enro.core.internal.handle.NavigationHandleViewModel.executeInstruction(NavigationHandleViewModel.java:75)
       at dev.enro.core.fragment.DefaultFragmentExecutor$tryExecutePendingTransitions$1.run(DefaultFragmentExecutor.java:162)
       at android.os.Handler.handleCallback(Handler.java:883)
       at android.os.Handler.dispatchMessage(Handler.java:100)
       at android.os.Looper.loop(Looper.java:237)
       at android.app.ActivityThread.main(ActivityThread.java:8167)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:496)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1100)

I'm pretty sure these are cases where I'm using child keys in the navigation key, but I haven't been able to reproduce this yet.

I'm wondering if tryExecutePendingTransitions just needs to have a check that fromContext.fragmentHostFor(instruction.navigationKey)?.fragmentManager is not null and if it is, run the code for the IllegalStateException for that as well. It seems like we depend on it being not null in the open code, but I'm not sure I'm reading the different paths to get a fragment manager correctly.

[isaac-udy]

I have semi-good news! I have a test that can reproduce this stack trace. I don't have a solution yet, but I'm exploring options.

If you're interested, here's the test I am using to work on these changes (this is added to "ActivityToFragmentTests.kt"):

    @Test
    fun givenActivityWithChildFragment_whenMultipleChildrenAreOpenedOnActivity_andStaleChildFragmentHandleIsUsedToOpenAnotherChild_thenStaleNavigationActionIsIgnored() {
        val scenario = ActivityScenario.launch(ActivityWithFragments::class.java)
        expectActivity<ActivityWithFragments>()
            .getNavigationHandle()
            .forward(ActivityChildFragmentKey(UUID.randomUUID().toString()))

        val activityHandle = expectActivity<ActivityWithFragments>().getNavigationHandle()
        val fragmentHandle = expectFragment<ActivityChildFragment>().getNavigationHandle()

        scenario.onActivity {
            activityHandle
                .forward(ActivityChildFragmentKey(UUID.randomUUID().toString()))

            activityHandle
                .forward(ActivityChildFragmentKey(UUID.randomUUID().toString()))

            fragmentHandle
                .forward(ActivityChildFragmentKey(UUID.randomUUID().toString()))

            activityHandle
                .forward(ActivityChildFragmentKey(UUID.randomUUID().toString()))
        }
    }

[isaac-udy]

It looks like changing back to commitNow does fix the issue. There are some alternative solutions that might also work, but I feel changing back to commitNow gives more predictable outcomes. Thanks for bearing with me, even though your original suggestion would have fixed the problem. I found a couple of other bugs along the way (which have now also been fixed), and we now have a test that will let us know if this issue regresses (which I think is an awesome win!).

I will make this change and release a new version tomorrow.

[mgod]

Thank you! I think I've also been seeing the navigation handle bug as well, but have been working around it.

[isaac-udy]

I have released Enro 1.3.7, which reverts back to commitNow for fragment transactions and should resolve this issue.

[mgod]

1.3.7 resolved all the issues I was seeing. Thanks!