square / leakcanary

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

Fragment view leak detected by LeakCanary with ViewBinding/Databinding #2341

Closed mazzouzi closed 2 years ago

mazzouzi commented 2 years ago

Description

Hi LeakCanary team. First of all, a big thank you for this great library !

The issue is super simple to reproduce : I run my app which displays a fragment called "MainFragment" in which I use DataBinding. When the end-user clicks on the button to display a second fragment, I replace current fragment by selected one using FragmentTransactionApi and "replace()" along with "addToBackStack()". So at this point, "MainFragment" gets its view destroyed only. Then second fragment is displayed and if I wait a couple of seconds, LeakCanary reports a leak saying that "MainFragment" view is leaking while I do set "_binding" to null in onDestroyView() :

override fun onDestroyView() { _binding = null super.onDestroyView() }

Steps to Reproduce

Sample app : https://github.com/mazzouzi/MemoryLeakApp

  1. Open the app
  2. Tap on any button
  3. Wait a couple of seconds.
  4. LeakCanary dumps the heap (LeakCanary.config overridden : LeakCanary.config.copy(retainedVisibleThreshold = 1).

Expected behavior: There should be no leak detected as I'm cleaning up "_binding" correctly in onDestroyView()

Version Information

Leak Trace :

====================================
HEAP ANALYSIS RESULT
====================================
1 APPLICATION LEAKS

References underlined with "~~~" are likely causes.
Learn more at https://squ.re/leaks.

3616 bytes retained by leaking objects
Signature: 44d037b667ba0ef18ffa7c961c1765d3220bc66a
┬───
│ GC Root: Global variable in native code
│
├─ android.graphics.animation.RenderNodeAnimator instance
│    Leaking: UNKNOWN
│    Retaining 471,5 kB in 10047 objects
│    mListeners[0] = android.graphics.drawable.RippleAnimationSession$3
│    ↓ RenderNodeAnimator.mTarget
│                         ~~~~~~~
├─ android.graphics.RenderNode instance
│    Leaking: UNKNOWN
│    Retaining 420,7 kB in 9666 objects
│    ↓ RenderNode.mAnimationHost
│                 ~~~~~~~~~~~~~~
├─ android.view.ViewAnimationHostBridge instance
│    Leaking: UNKNOWN
│    Retaining 420,7 kB in 9665 objects
│    ↓ ViewAnimationHostBridge.mView
│                              ~~~~~
├─ com.google.android.material.button.MaterialButton instance
│    Leaking: UNKNOWN
│    Retaining 420,7 kB in 9664 objects
│    View not part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mID = R.id.holding_reference_from_background_solution
│    View.mWindowAttachCount = 1
│    mContext instance of com.mazzouzi.memoryleak.ui.MainActivity with mDestroyed = false
│    ↓ View.mParent
│           ~~~~~~~
├─ androidx.constraintlayout.widget.ConstraintLayout instance
│    Leaking: UNKNOWN
│    Retaining 2,1 kB in 31 objects
│    View not part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mID = R.id.firstBlock
│    View.mWindowAttachCount = 1
│    mContext instance of com.mazzouzi.memoryleak.ui.MainActivity with mDestroyed = false
│    ↓ View.mParent
│           ~~~~~~~
├─ androidx.constraintlayout.widget.ConstraintLayout instance
│    Leaking: UNKNOWN
│    Retaining 1,3 kB in 20 objects
│    View not part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mID = R.id.main
│    View.mWindowAttachCount = 1
│    mContext instance of com.mazzouzi.memoryleak.ui.MainActivity with mDestroyed = false
│    ↓ View.mParent
│           ~~~~~~~
╰→ android.widget.ScrollView instance
​     Leaking: YES (ObjectWatcher was watching this because com.mazzouzi.memoryleak.ui.MainFragment received
​     Fragment#onDestroyView() callback (references to its views should be cleared to prevent leaks))
​     Retaining 3,6 kB in 68 objects
​     key = e8f19633-cca8-4275-9d6e-e74a8d78418f
​     watchDurationMillis = 5668
​     retainedDurationMillis = 660
​     View not part of a window view hierarchy
​     View.mAttachInfo is null (view detached)
​     View.mWindowAttachCount = 1
​     mContext instance of com.mazzouzi.memoryleak.ui.MainActivity with mDestroyed = false
====================================
0 LIBRARY LEAKS

A Library Leak is a leak caused by a known bug in 3rd party code that you do not have control over.
See https://square.github.io/leakcanary/fundamentals-how-leakcanary-works/#4-categorizing-leaks
====================================
0 UNREACHABLE OBJECTS

An unreachable object is still in memory but LeakCanary could not find a strong reference path
from GC roots.
====================================
METADATA

Please include this in bug reports and Stack Overflow questions.

Build.VERSION.SDK_INT: 31
Build.MANUFACTURER: Google
LeakCanary version: 2.8.1
App process name: com.mazzouzi.memoryleak
Stats: LruCache[maxSize=3000,hits=108920,misses=194655,hitRate=35%]
RandomAccess[bytes=9557724,reads=194655,travel=64601167621,range=29771575,size=36691897]
Heap dump reason: 1 retained objects, app is visible
Analysis duration: 9094 ms
Heap dump file path: /storage/emulated/0/Download/leakcanary-com.mazzouzi.memoryleak/2022-04-13_12-04-09_994.hprof

2022-04-13 12:04:24.098 16642-16724/com.mazzouzi.memoryleak D/LeakCanary: Heap dump timestamp: 1649844264045 Heap dump duration: 3191 ms

pyricau commented 2 years ago

There should be no leak detected as I'm cleaning up "_binding" correctly in onDestroyView()

You might be doing the right thing with your bindings, but there's still a leak caused by something else. You should believe LeakCanary when it tells you there's a leak, there's nothing to fix in LeakCanary here.

From what I can tell, your app has a MaterialButton with id holding_reference_from_background_solution (interesting name!). That button is detached and should likely be garbage collected, but it can't because there's an animator looping a ripple animation on it. You need to cancel that animator. More details: https://square.github.io/leakcanary/changelog/#objectanimator-leaks

Moradious commented 2 years ago

Thank you for leading me to the root cause. I think I understand what's going on :

I'm not using any animator but MaterialButton does it internally for the ripple effect. On my app, once the user taps on a button it starts a new fragment right way and the ripple effect does not end before the fragment transaction occurs.

In the same time, I found this issue on Google Issue Tracker (reported by you maybe?) : https://issuetracker.google.com/issues/212993949

I think this is the same root cause. If a user clicks a button and the ripple effect animation does not end before the fragment is replaced then the view leaks probably because Android does not cancel the animation.

If I set a delay of 2 seconds to let the button complete its ripple animation before the fragment transaction -> the view is not leaking anymore

So that's probably an ASOP issue.

PS : to reproduce the issue, we simply need to click a button and "replace" (+ "addToBackStack") current fragment by another one =/

pyricau commented 2 years ago

I did file that issue, and looking more closely now I actually think they're different issues. It looks like you might have found a core leak issue with ripple animators in AOSP.. Do you think you could create a small project with LeakCanary enabled that reproduces the issue and share it here? If this is still not fixed in the latest versions of AOSP we should file a ticket, which means we need a repro.

mazzouzi commented 2 years ago

There you go : link

pyricau commented 2 years ago

Thanks! I can repro on an emulator API 31 and on Android 13 DP 2 as well (had to click & hit back several times for that 2nd one though)

Moradious commented 2 years ago

Thank you for testing on latest version of Android. Honestly this issue is so annoying when you have LeakCanary installed on your debug app, especially when you decrease "retainedVisibleThreshold" property to less than 5. Besides, it might confuse some people using the library for not understanding where this leak comes from. Not to mention this issue deteriorates app performance by retaining memory. If the issue is still there, we should file a ticket. I'll do it if you like or if you wanna do it, be my guest.

pyricau commented 2 years ago

Filed here: https://issuetracker.google.com/issues/229136453 (sorry just saw your response)

pyricau commented 2 years ago

We should also add a pattern to our db.

pyricau commented 2 years ago

Note: reproduced on API 31 and API 32, but not API 29 or API 30.

pyricau commented 2 years ago

Note: this bug what just marked as fixed on the Google side!