Could ViewComponentManager$FragmentContextWrapper expose the Fragment's lifecycle

[eboudrant]

Hello, as you may know, Hilt is causing a memory leak in Epoxy. Details are here airbnb/epoxy#1025

I believe it would be fixed in the ViewComponentManager$FragmentContextWrapper was implementing LifecycleOwner and would return the fragment lifecycle. Ex:

    public static final class FragmentContextWrapper extends ContextWrapper implements LifecycleOwner {
        ...
        @NonNull
        @Override
        public Lifecycle getLifecycle() {
            return fragment.getLifecycle();
        }
        ...
    }

For details about why it would fix the memory leak see ActivityRecyclerPool.kt#L69. Epoxy would then use a view pool tied to the fragment lifecycle, not the activity. As FragmentContextWrapper contains a reference to a fragment, if the view pool is tied to the activity it will leak the fragment.

However I am not sure about all the implication this change would have on Hilt but it would make sense to be able to access the fragment lifecycle from the view's context?

What do you think ?

We use Hilt 2.29.1-alpha.

Thanks

[bcorso]

Hi @eboudrant,

I think there will still be a leak, at least for original issue posted in #2070. IIUC, the underlying issue is that a view created with the FragmentContextWrapper will hold a reference to the fragment, so a leak occurs when the view's lifetime exceeds the fragment's lifetime. So I think we'll still need to do the fix suggested in #2070 (comment).

However, we can also look into the change that you've suggested once we've fixed #2070, if you think there is still benefits for your use case.

[eboudrant]

I think if, in the case of Epoxy, (view.context as? LifecycleOwner).lifecyle was the fragment's lifecycle then the view pool (cause of the leak) would not exceeds the fragment's lifetime. Let me quickly test it...

[eboudrant]

@bcorso I tested and yes, this change, in Hilt only, won't be enough. We also require a change in Epoxy, see airbnb/epoxy#1099.

So at this point I am not sure, the changes I am proposing does not seems crazy but I don't know all the impacts. Like is it acceptable to make the FragmentContextWrapper a LifecycleOwner and delegate to the fragment?

It will also depends do you plan to address #2070. If you plan to clear the fragment's reference, once destroyed, in the wrapper then this change is not good (as lifecycle should not be optional).

[bcorso]

It will also depends do you plan to address #2070. If you plan to clear the fragment's reference, once destroyed, in the wrapper then this change is not good (as lifecycle should not be optional).

It sounds like there are cases where a view inflated by the fragment needs to outlive the fragment, so I think we'll definitely want to clear the reference.

IIUC, I think epoxy would no longer have a leak if we cleared the fragment's reference as suggested in #2070, right?

[eboudrant]

Yes, Epoxy won't leak and this change won't be needed if the fragment reference get cleared.

However, I never used the view scoped component but won't it be a problem here? Where a view initialized for a fragment outlive it, how do you plan to handle it? I am thinking of ViewWithFragmentComponent.

[bcorso]

Sorry for the delayed response.

For views using ViewWithFragmentComponent they will still leak because the view itself contains fragment state. However, I think that's okay. The main case we're trying to solve is for views that would normally not leak state without Hilt but are only leaking state due to the unconventional storing the Fragment within the contextwrapper.

As for making FragmentContextWrapper a LifecycleOwner, I don't think we want to do this. Given that a context normally is not a LifecycleOwner we don't want to introduce that specifically for Hilt context.

I'll close this issue out, and you can follow #2070 for the leak fix.