getMvpDelegate().onDestroy() not called in MvpAppCompatFragment in some cases

[september669]

For reproducing
State 1. Activity -> FullScreenFragment1
Then run

        fragmentTransaction
            .replace(fragmentContainerId, FullScreenFragment2, screen.screenKey)
            .addToBackStack(screen.screenKey)
            .commit()

State 2. Activity -> FullScreenFragment1 -> FullScreenFragment2
Collapse the app and restore it.
Then remove all fragments and add another.

Log for FullScreenFragment1

        Start FullScreenFragment1

FullScreenFragment1.onAttach()
FullScreenFragment1.providePresenter()
FullScreenFragment1.onCreateView()
FullScreenFragment1.onViewCreated()
FullScreenFragment1.onStart()
FullScreenFragment1.onResume()

        Start FullScreenFragment2

FullScreenFragment1.onPause()
FullScreenFragment1.onStop()
FullScreenFragment1.onDestroyView()

        Hide

FullScreenFragment1.onSaveInstanceState() isStateSaved = true

        Restore

nothing called for  FullScreenFragment1

        Go away

FullScreenFragment1.onDestroy() isStateSaved == true
FullScreenFragment1.onDetach()

As a result getMvpDelegate().onDestroy() will not be called for FullScreenFragment1
This is because onStart() not called for fragments in backstack and variable isStateSaved will be true.

Possible solution is the using activity.isChangingConfigurations() instead of isStateSaved

[bejibx]

Can you please specify reproduction steps a bit more clearly? I can't quite understand how exactly should I "remove all fragments and add another" to reproduce this issue.

[bomiyr]

Seems I have similar issue, but with different flow. You just need to open activity for result and change fragments in onActivityResult when it's closed.

Simple project to reproduce the issue: https://drive.google.com/file/d/1FXbj1F0norFwjzyMM-z8MaSkmkg20ZAU/view?usp=sharing

[september669]

Can you please specify reproduction steps a bit more clearly? I can't quite understand how exactly should I "remove all fragments and add another" to reproduce this issue.

For reproduce you need clear backstack in fragment manager, e.g.

fragmentManager.popBackStack(null, FragmentManager.POP_BACK_STACK_INCLUSIVE);

Now i use this implementation (mixed with Mosby's solution) :

open class MvpAppCompatFragment : Fragment, MvpDelegateHolder {
...

    override fun onDestroy() {
        super.onDestroy()

        //We leave the screen and respectively all fragments will be destroyed
        if (requireActivity().isFinishing) {
            getMvpDelegate().onDestroy()
            return
        }

        // When we rotate device isRemoving() return true for fragment placed in backstack
        // http://stackoverflow.com/questions/34649126/fragment-back-stack-and-isremoving
        if (activity?.isChangingConfigurations == true) {
            return
        }

        var anyParentIsRemoving = false
        var parent = parentFragment
        while (!anyParentIsRemoving && parent != null) {
            anyParentIsRemoving = parent.isRemoving
            parent = parent.parentFragment
        }

        if (!isInBackStackAndroidX()) {
            getMvpDelegate().onDestroy()
        }
    }

    override fun getMvpDelegate(): MvpDelegate<*> {
        if (mvpDelegate == null) {
            mvpDelegate = MvpDelegate(this)
        }

        return mvpDelegate as MvpDelegate<out MvpAppCompatFragment>
    }
}

fun Fragment.isInBackStackAndroidX(): Boolean {
/*
    // see https://github.com/sockeqwe/mosby/blob/6edf7f3674013b57bde3fb11ae139a30680286e3/utils-fragment/src/main/java/android/support/v4/app/BackstackAccessor.java
    // and https://github.com/sockeqwe/mosby/blob/master/mvi/src/main/java/com/hannesdorfmann/mosby3/FragmentMviDelegateImpl.java
    // and this https://github.com/grandcentrix/ThirtyInch/issues/206
    val writer = StringWriter()
    dump("", null, PrintWriter(writer), null)
    val dump: String = writer.toString()
    return !dump.contains("mBackStackNesting=0")
*/

    //  see https://github.com/grandcentrix/ThirtyInch/pull/205/commits/aaea0a2994dda83876d360fef593a8c8a4df0639
    return try {
        val backStackNestingField: Field = Fragment::class.java.getDeclaredField("mBackStackNesting")
        backStackNestingField.isAccessible = true
        val backStackNesting: Int = backStackNestingField.getInt(this)
        backStackNesting > 0
    } catch (e: NoSuchFieldException) {
        throw RuntimeException(e)
    } catch (e: IllegalAccessException) {
        throw RuntimeException(e)
    }
}

[bejibx]

Thank you for clarification! I'll look at this issue soon.

[bejibx]

So yeah, it is definitely a problem. I kinda want to stop using those hacky solutions but it seems there is no official way to check if fragment is removing finally or temporary. I have another idea though. What if for every MvpFragment we add some retained child fragment so we could use its onDestroy lifecycle hook to know if our parent fragment is removing completely? I checked this approach in your scenario and looks like it's working fine. I should check it in another cases.

@xanderblinov @alaershov @aasitnikov @senneco what do you think about this approach?

[alaershov]

There kinda is a way, take a look at how the viewModelStore is implemented in JetPack ViewModel. There is a check, something like activity.isFinishing && !fragment.isChangingConfigurations.
The only problem with this check is that it does not handle "Don't keep activities" flag. Like you have an Activity, go forward, go back, and ViewModel is recreated.
This is not so bad, because it's a developer flag, and official doc states that this behavior never actually happens in production. But some hacky vendors enable this flag by default, and break apps.
So we have 2 options I'm aware of: ignore the problem with "Don't keep activities" and behave like ViewModels, or keep using our hacky solution.

[bejibx]

So it seems for me we have 3 options to fix this:

1. Reset isStateSaved flag for fragments in backstack. It seems like when you hide and show application no callbacks are called so we have to reset this flag from outside component. It seems like Activity#onStart() might be a good place for this. There is another problem though - getting fragments from FragmentManager backstack is not an easy task. Only solution that I found require reflection:

override fun onStart() {
    super.onStart()
    try {
        val activeFragmentMethod = FragmentManager::class.java.getDeclaredMethod("getActiveFragments")
        activeFragmentMethod.isAccessible = true
        @Suppress("UNCHECKED_CAST")
        val activeFragments = activeFragmentMethod.invoke(supportFragmentManager) as List<Fragment>
        activeFragments.forEach {
            (it as? MvpFragment)?.resetSavedStateFlag()
        }
    } catch (t: Throwable) {
        Log.w(javaClass.simpleName, t)
    }
}

Obvious drawbacks are - already hacky solution became even more hacky, reflection usage (which is slow on Android), usage of private api which may change unpredictably and also increased difficulty to properly setup library for users when they are not using our base containers.

2. Try to solve initial problem (incorrect isRemoving flag) with another approach. Looks like instead of using isStateSaved flag we can use following check:

override fun onDestroy() {
    super.onDestroy()

    if (activity?.isFinishing == true) {
        mvpDelegate.onDestroy()
        return
    }

    if (activity?.isChangingConfigurations == true && isInBackStack) {
        return
    }

    if (isRemoving) {
        mvpDelegate.onDestroy()
        return
    }
    
    val isAnyParentRemoving = generateSequence(parentFragment) { parentFragment }
            .any { it.isRemoving }
    if (isAnyParentRemoving) {
        mvpDelegate.onDestroy()
    }
}

private val isInBackStack: Boolean
    get() {
        return try {
            val backStackNestingField = Fragment::class.java.getDeclaredField("mBackStackNesting")
            backStackNestingField.isAccessible = true
            val backStackNesting: Int = backStackNestingField.getInt(this)
            backStackNesting > 0
        } catch (t: Throwable) {
            false
        }
    }

Again, very hacky solution, usage of private api, reflection, but not that different from current one in terms of setup difficulty. Actually in this case we can get rid of isStateSaved flag and move onDestroy logic to some utility class which will simplify library setup for users (no need to copy-paste this logic from MvpFragment).

3. Stop Don't keep Activities support. I mean, if even android architecture components don't support this, why should we keep all this hacky stuff in our library? Yeah, I understand we cannot just change this behavior because many library users may rely on it, but maybe we can add an option for users to decide which one to use?

[bejibx]

So we had some internal discussion about this issue and also I've done some research which led to creating new issue in androidx.fragment library. Basically android fragments using mBackStackNesting field to identify if fragment is finishing or not. Here is relevant line where you can see this. This approach tho' have one issue - mBackStackNesting is > 0 even if we add our fragment after state save which leads to some unpleasant memory leaks.

It seems to me that checking if fragment saves its state before onDestroy is the only way to be sure if fragment is finishing or not, so I'll stick with option 1 for now. I also have some ideas how to make MvpDelegate handling easier, maybe even without need to extend MvpFragment's.

[AlexeyKorshun]

Hi, I faced with similar issue when I use ViewPager2 with FragmentStateAdapter. When we leave screen with ViewPager inner presenters for fragments(pages) are not destroying. FragmentStateAdapter use removeFragment in this method mFragmentManager.saveFragmentInstanceState is called, and after it mFragmentManager.beginTransaction().remove(fragment).commitNow();
For this case moxy doesn't call this.getMvpDelegate().onDestroy(); because isStateSaved is true. But I think it isn't a moxy problem, it is for your information.