Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LeakCanary stopped detecting leaked ViewModels #2496

Closed
Dimezis opened this issue Apr 3, 2023 · 4 comments · Fixed by #2512
Closed

LeakCanary stopped detecting leaked ViewModels #2496

Dimezis opened this issue Apr 3, 2023 · 4 comments · Fixed by #2512
Milestone

Comments

@Dimezis
Copy link

Dimezis commented Apr 3, 2023

Description

LeakCanary doesn't detect leaked ViewModels when using lifecycle-viewmodel-ktx dependency.
androidx.lifecycle:lifecycle-viewmodel-ktx (version 2.6.1)
The code it uses in ViewModelClearedWatcher for retrieving the ViewModel map always throws, because the -ktx version uses map field, not mMap.

java.lang.NoSuchFieldException: No field mMap in class Landroidx/lifecycle/ViewModelStore; (declaration of 'androidx.lifecycle.ViewModelStore' appears in /...)
private val viewModelMap: Map<String, ViewModel>? = try {
    val mMapField = ViewModelStore::class.java.getDeclaredField("mMap")
    mMapField.isAccessible = true
    @Suppress("UNCHECKED_CAST")
    mMapField[storeOwner.viewModelStore] as Map<String, ViewModel>
  } catch (ignored: Exception) {
    null
  }

Version Information

  • LeakCanary version: 2.1.0
  • Android OS version: SDK 30
  • Gradle version: 8.0.2
@pyricau
Copy link
Member

pyricau commented Apr 3, 2023

damn we probably should have logged this to get it reported earlier. Thanks!

The issue doesn't seem to be about ktx vs no ktx but simply that ViewModelStore was converted from Java to Kotlin and the map name changed: https://cs.android.com/androidx/platform/frameworks/support/+/8aa6ca1c924ab10d263b21b99b8790d5f0b50cc6:lifecycle/lifecycle-viewmodel/src/main/java/androidx/lifecycle/ViewModelStore.kt;dlc=401acbbce9e14745594b8d3d7a5b3f25e5462dd4

PR welcome!

@Dimezis
Copy link
Author

Dimezis commented Apr 3, 2023

@pyricau
I can make a PR, but there's one pesky problem I'm not sure how to approach.

I wanted to make a test against this new version of lifecycle-viewmodel, but it doesn't compile unless we bump LeakCanary's Kotlin version to 1.8+ (because of some ABI incompatibility).

Bumping the Kotlin version forces when expression to be always exhaustive, which affects quite a bit of places in the codebase, plus that would probably force LeakCanary users to update their Kotlin version as well, though I'm not sure.

We can

  1. Skip the test part
  2. Update Kotlin and all when statements, plus most likely force clients to update

What's your take on this?

@pyricau
Copy link
Member

pyricau commented Apr 4, 2023

Could you make a local release then test it out against another app that's not sharing the gradle config (e.g. your app) ?

@Dimezis
Copy link
Author

Dimezis commented Apr 7, 2023

I actually can't build a valid .aar (without any gradle changes)
./gradlew build produces some empty 4kb aar on Zulu JDK8 on M1 mac.
On JDK 11 and 17 the project is not buildable at all.

I've read this section but couldn't find hints there

@pyricau pyricau added this to the 2.11 milestone May 17, 2023
pyricau added a commit that referenced this issue May 17, 2023
@pyricau pyricau changed the title LeakCanary doesn't detect leaked ViewModels with -ktx dependency LeakCanary stopped detecting leaked ViewModels May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants