square / leakcanary

A memory leak detection library for Android.
https://square.github.io/leakcanary
Apache License 2.0
29.25k stars 3.96k forks source link

Compose Only Based Navigation: View Model Leaks Not Auto Detected? #2678

Open ryanholden8 opened 1 month ago

ryanholden8 commented 1 month ago

We have an app with Leak Canary integrated within our instrumentation tests. Works great for activity leaks, as we manually created a leak and it fails the test. Auto detection of activities working.

However, we manually created a leaking view model (a strong global reference is held on to the view model within its init) and the test continues to pass. Auto detection of view models not working.

Is there additional setup that is needed to detect view models created from Compose when using Compose Navigation instead of Activity/Fragment navigation? The docs say no. Is there documentation on how this auto view model detection works?

Dimezis commented 1 month ago

So what are you doing exactly to trigger the leak? Configuration change? Just storing the reference is not enough if the corresponding nav controller is still alive.

ryanholden8 commented 1 month ago

So what are you doing exactly to trigger the leak detection? Configuration change? Just storing the reference is not enough if the corresponding nav controller is still alive.

NavControllerViewModel seems to release/clear view models correctly as things navigate around but it's an internal implementation class we can only see while debugging. It's a navigation view model that has its own view model store for view models within its navigation scope. It's a parent view model and it is a view model store for its children view models. Each navigation stack item/screen is a child view model connected to NavControllerViewModel, it seems from our debugging. We have also asserted, manually, that NavControllerViewModel itself is cleaned up when the test finishes by adding it to objects to watch. However, NavControllerViewModel's view model store for it's children view model is a private property we can't access.

Keeping a reference around for our view model when NavControllerViewModel clears it and it itself is cleaned up would causes a leak if auto leak detection for view models worked, no?

pyricau commented 1 month ago

I haven't tested view model leak detection on compose projects.

View model leak detection is kicked off by leakcanary.internal.AndroidXFragmentDestroyWatcher, in 2 places:

1) On LeakCanary init, if androidx.fragment.app.Fragment is in the classpath, AND the activity is a FragmentActivity, then we hook into the activity view model store 2) On fragment creation, we hook into the fragment's view model store.

I suspect that the issue here is one of:

I wrote all this before Compose was a thing, so it's very possible we have a bug. Let me know what you find. Don't hesitate to put breakpoints in leakcanary.internal.AndroidXFragmentDestroyWatcher.invoke() and leakcanary.internal.ViewModelClearedWatcher.install() to figure out what's up.

ryanholden8 commented 1 month ago

I haven't tested view model leak detection on compose projects.

View model leak detection is kicked off by leakcanary.internal.AndroidXFragmentDestroyWatcher, in 2 places:

  1. On LeakCanary init, if androidx.fragment.app.Fragment is in the classpath, AND the activity is a FragmentActivity, then we hook into the activity view model store
  2. On fragment creation, we hook into the fragment's view model store.

I suspect that the issue here is one of:

  • NavControllerViewModel isn't tied to an activity or fragment view model store (?). Not sure exactly how this is setup.
  • Your activity isn't a FragmentActivity
  • You don't have the androidx.fragment.app.Fragment class in your classpath.

I wrote all this before Compose was a thing, so it's very possible we have a bug. Let me know what you find. Don't hesitate to put breakpoints in leakcanary.internal.AndroidXFragmentDestroyWatcher.invoke() and leakcanary.internal.ViewModelClearedWatcher.install() to figure out what's up.

Thank you for this info! This allows me to investigate further..

pyricau commented 1 month ago

Cool. Did you find anything?