Leak while integrating with navigation component #2070
Comments
|
I'd like to at least understand which component is the faulty one. I've looked into sources of navigation library and it would seem that they're holdin I'm purely guessing here, but it also sounds like it could be related to this: I didn't understand what exactly in the generated Dagger.Hilt could cause that behaviour however. |
|
The issue is likely that views inflated by a Hilt fragment wrap the context in The reason there is no leak when inflating from a non-Hilt fragment is because normally the inflator just uses the activity's context, so there is no reference to the fragment. Possible solutions:
|
|
Is there a workaround possible to mitigate the memory leak until this bug is fixed? The only workaround I see is creating my own ToolbarOnDestinationChangedListener, passing in the activity context. But then I need to duplicate NavigationUI too. |
|
@wbervoets I´ve created this extension function for now: It's far from ideal and I hope a fix is released soon, but for now, works for my (rather simple) use-cases. |
|
@mpierucci seems like I have a problem tinting the drawer arrow drawable with a color (when the toolbar has a style applied which sets a android:theme, which sets the colorControlNormal color); it just doesn't work when one passes requiresContext() instead of toolbar.context I hope the dagger team fixes this bug soon, is their any ETA? |
|
The fix for this should be submitted shortly, but unfortunately the fix won't make the cut for the next release happening this week. We'll have another release within the next 2 weeks, so the fix will be in that release. |
|
Am I right to assume this fix is inside 2.31.1 but not anymore in 2.31.2 ? |
|
I don't believe it's in either, or at least that was the plan. IIRC, it was committed after 2.31 was released and we reverted it before 2.31.1 was released. The reason we reverted it was because we didn't want to introduce this potentially breaking feature in a patch release (2.31.1 / 2.31.2). Instead, we'll aim to get it back in for the 2.32 release. |
|
Is the fix for this in 2.32? I can't find an issue link in https://github.com/google/dagger/releases/tag/dagger-2.32 |
|
I don't see changes related to this issue in |
|
Maybe they accidently forgot because this issue is already closed or are waiting for Androidx Fragment 1.3 final release? |
|
Any updates on this bug? 2.33 doesn't appear to contain a fix. |
…Destroy() This CL uses the fragment Lifecycle to clear the fragment instance from the FragmentContextWrapper after onDestroy. Fixes: #2070 See #2070 RELNOTES=Fixes #2070: Clears the Fragment reference in FragmentContextWrapper after Fragment#onDestroy() to prevent leaks if a non-Hilt View outlives the Hilt Fragment that created it. PiperOrigin-RevId: 358388338
…Destroy() This CL uses the fragment Lifecycle to clear the fragment instance from the FragmentContextWrapper after onDestroy. Fixes: #2070 See #2070 RELNOTES=Fixes #2070: Clears the Fragment reference in FragmentContextWrapper after Fragment#onDestroy() to prevent leaks if a non-Hilt View outlives the Hilt Fragment that created it. PiperOrigin-RevId: 359775904
|
@marcoRS sorry, we ran into some issues rolling this fix forward internally. It's in now, but unfortunately it didn't make it into the 2.33 release by a few days so it's only in the HEAD snapshots. |
|
any update on the eta for the 2.34 release? thanks |
|
This issue is blocking our new release, any workaround here? thanks |
|
Looks like this issue resolved in 2.34 🎉 |
Just tried |
|
@mpierucci, feel free to leave more details about your leak, and we can take a look. Note that the particular fix (tested in FragmentContextWrapperLeakTest.java) only removes the Fragment reference we store in the context, but it doesn't prevent your view from leaking the fragment in other ways. For example, if the view in question is a Hilt view with fragment bindings (i.e., it's annotated with |
|
@mpierucci thanks! Looks like we'll need to release the inflator reference too. I'll add a fix for that. |
|
@bcorso excellent thanks! Shall we re-open this issue then until the fix is released? |
|
Sounds good. Reopening until we fix the inflator leak. |
…t exists. Fixes #2070 RELNOTES=Fix leak in FragmentContextWrapper by releasing the baseInflator if it exists. PiperOrigin-RevId: 367480374
|
@mpierucci this should be fixed in Dagger 2.34.1 if you want to verify. I ran through your instructions on the sample app a few times with the fix and didn't get any notification from LeakCanary, so hopefully this is fixed for you now. |
|
@bcorso Can´t reproduce with |
|
@bcorso It seems this issue is still unresolved. I´m still seeing the leak on Hilt version 2.36 |
|
@ducnv3012 I see that the Can you confirm this is a leak due to Hilt (e.g. by swapping the Hilt view with a normal view and verifying there is no leak). |
|
@bcorso I swapped the Hilt view with a normal view and verifying there is no leak. But i found the cause because i was using Paging3 in my fragment. Thanks for your support |
|
Is this finally fixed in the latest version? There is some instructions to enable flag |
|
@marcoRS this has been fixed since Dagger 2.34.1 (#2070 (comment)). |
|
Could it be that this issue is re-occuring? I'm seeing the following in my LeakCanary logs (running Hilt 2.40.5): |
|
@salberin I don't think that's an issue with Hilt. In this case, it looks like the To clarify, the original problem was that a Hilt view was keeping an instance of the parent fragment. This meant that if the Hilt view outlived the fragment it would leak the fragment. That problem is now solved. |
|
@bcorso looks like the same issue still appears (latest version of Dagger/Hilt 2.41) when in the fragment's xml the android:theme attribute is applied: Here some maybe useful screenshots of the heap: With the android:theme attribute applied: With out the android:theme attribute applied: Is there anything we can do about this? |
|
@patrickpoe I think this is a different issue. The trace suggests that while Hilt's In particular, I'm looking at the direct path of the leak, which doesn't include Hilt: |
|
@bcorso thanks a lot for your quick answer! You are right, but it just happens in the LeakFragment (marked with I also forgot to mention, that it just happens after i accessed the LayoutInflater from a view context. I quickly forked the sample project and if you like you can check it out: patrickpoe/Hilt-Navigation-Leak@39def95 |
|
Interesting, thanks for the sample project! I'll try to take a look sometime this week. |
|
I have the same issue with 2.41. componentContext instance of dagger.hilt.android.internal.managers. |
|
@bcorso did you have a chance to look into that issue already? did you find anything? |
|
Yes, I was able to look into this, but unfortunately I wasn't able to track down the root cause of the issue. I can confirm that it only happens with the Hilt fragment and only when using navigation, but the cause of the leak really doesn't make sense to me. From your example project the leak is caused here: @AndroidEntryPoint
class LeakFragment : Fragment(R.layout.leaky_fragment) {
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
...
// leak caused by this line
LayoutInflater.from(view.context)
}
}However that line doesn't seem to store any obvious state so it's not clear to me what's actually causing the leak. Also, as I mentioned before, the leak doesn't actually point to anything contained within a Hilt class, it just states that the object leaked contains the context which is wrapped by Hilt, so it's not clear what, if anything, we could do to actually fix this leak. Sorry, I wish I had a better answer. |
|
Alright, thank you - looks like we came to the same conclusion. Really curious what the root cause here is, anyways pls just let me know in case you find something interesting. |
|
I have the same issue with 2.42. componentContext instance of dagger.hilt.android.internal.managers. Source code you can see SearchListFragment |
|
@bcorso I have the same issue with version 2.40. mBase instance of dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper, wrapping activity com.example.main.HomeActivity with mDestroyed = true |
|
@kaviskhandelwal, do you own the The AFAIK, I don't think there's much we could do on the Hilt side. The |
The following leak is reported when integrating Hilt with fragments and navigation components. A full project to reproduce the leak can be found Here
Basically you just need to add
@AndroidEntryPointto a fragment that updates the toolbar ( as suggested by the official documentation) and you'll get a leak when navigating back and forth.Since this issue does not happen if:
or
I´ll report it in both libraries
The text was updated successfully, but these errors were encountered: