Memory leak in ViewPositionHolder

[mr-thierry]

It seem there is a memory leak when using GestureViews.

ViewPositionAnimator contains a fromPosHolder and a toPosHolder. But toPosHolder.clear() is never called. As such, view.getViewTreeObserver().removeOnPreDrawListener is never called, and the OnPreDrawListener keep a reference to the GestureView.

[alexvasilkov]

Do you actually experience memory leaks or it is only a theoretical observation?

What you say actually is that GestureView keeps reference to itself, since toPosHolder is a position tracker for GestureView itself. But it is not a definition of a memory leak :)

[mr-thierry]

I have an actual leak. :)
The leak is actually toPosHolder that register itself to the OnPreDrawListener, but is never removed from (clear() is never call for toPosHolder, only fromPosHolder). So the global layout tree OnPreDrawListener list keep a reference to toPosHolder indefinitely. :)

[alexvasilkov]

Non-removed ViewTreeObserver listeners itself does not introduce memory leak, this can be checked both in Android sources and e.g. in this SO question.

If you have a real memory leak then please describe how I can reproduce it and how you understand that it is a leak.

[mr-thierry]

You are absolutely correct. Non-removed ViewTreeObserver listeners are fine since the view hierarchy is destroyed when the activity is destroyed.

My problem is that I have a single activity, but with multiple fragments. FragmentA contains a small view and FragmentB contains my fullscreenview.

In FragmentB, toPosHolder register itself in the OnPreDrawListener. When I close the FragmentB, FragmentB is removed from the fragment stack, but OnPreDrawListener keeps a handle on FragmentB view, and FragmentB view is not garbage collected. And as such "leaks" as long as the activity is running.

I have found this issue while using my app (not yet available, sorry) using LeakCanary (I have LeakWatchers for Fragment).

[alexvasilkov]

Hm, can you share logs from LeakCanary here?

As I understand the leak can occur only if you'll keep reference to toPosHolder in you activity somehow. Otherwise we should just have 2 circular links: GestureView -> ViewPositionAnimator -> ViewPosHolder -> GestureView and also GestureView -> ViewTreeObserver -> ViewPosHolder -> GestureView, and ViewPosHolder. This kind of circular link should not be a problem for GC, all ViewTreeObserver listeners (as I can see in Android source code) are stored in the view itself and are not leaked to parent view or activity. ViewPosHolder is not referenced from anywhere else, so probably GestureView or ViewPositionAnimator are leaked somehow, but I don't see how it can happen as well.

So I can't really see how ViewPosHolder instance can be leaked, and I can't fix the issue until I have a clear understanding of what is going on here, so hopefully LeakCanary logs will contain some clues.

[mr-thierry]

Here is the leak stack from LeakCanary:

`* PhotoFullscreenFragment has leaked:

• static WindowManagerGlobal.sDefaultWindowManager
• ↳ WindowManagerGlobal.mRoots
• ↳ ArrayList.elementData
• ↳ array Object[].[0]
• ↳ ViewRootImpl.!(mAttachInfo)!
• ↳ View$AttachInfo.!(mTreeObserver)!
• ↳ ViewTreeObserver.!(mOnPreDrawListeners)!
• ↳ ViewTreeObserver$CopyOnWriteArray.!(mData)!
• ↳ ArrayList.!(elementData)!
• ↳ array Object[].!([1])!
• ↳ ViewPositionHolder.!(view)!
• ↳ GestureImageView.mLayoutParams
• ↳ RecyclerView$LayoutParams.mViewHolder
• ↳ HorizontalItemViewHolder.fragment
• ↳ PhotoFullscreenFragment`

PhotoFullscreenFragment is FragmentB in my previous example.

This leak warning occurs after PhotoFullscreenFragment was closed and removed from the fragments stack. The activity still has a link to PhotoFullscreenFragment since mOnPreDrawListeners keep a reference to ViewPositionHolder.

I think the root cause is due to the fact that ViewTreeObserver are in fact stored in the root of the view hierarchy of the activity, and not just on the view itself, as per your understanding.

Hope this help! :)

[mr-thierry]

By the way, I tried this code, and the leak went away (not sure of the impact though :) )

In the code that starts the animation:

animator.setToView(photoUrl, animatorView)
hackClearToPosHolder(animatorView.positionAnimator)

fun hackClearToPosHolder(viewPositionAnimator: ViewPositionAnimator) {
    accessViewPositionHolderByReflection(viewPositionAnimator).clear()
}

private fun accessViewPositionHolderByReflection(viewPositionAnimator: ViewPositionAnimator): ViewPositionHolder {
    val f = ViewPositionAnimator::class.java.getDeclaredField("toPosHolder")
    f.isAccessible = true
    return f.get(viewPositionAnimator) as ViewPositionHolder
}

[alexvasilkov]

Thanks, I think I understand the problem better now, but I'll need to think about a proper fix, since just calling .clear() on toPosHolder may not solve it for 100%.

As of your fix, it may work as it is for you, but it will not detect the source view (GestureView) position changes if they'll ever happen. It is not a very common use case, so you may not experience it in your app.

[leyvien]

I had a same proplem @ThierryD

[murtraja]

┬───
│ GC Root: System class
│
├─ android.provider.FontsContract class
│    Leaking: NO (AlMuhaffizApplication↓ is not leaking and a class is never leaking)
│    ↓ static FontsContract.sContext
├─ com.aliasgarmurtaza.hifzalquran.AlMuhaffizApplication instance
│    Leaking: NO (PlayerActivity↓ is not leaking and Application is a singleton)
│    AlMuhaffizApplication does not wrap an activity context
│    ↓ AlMuhaffizApplication.playerInfo
├─ com.aliasgarmurtaza.hifzalquran.models.PlayerInfo instance
│    Leaking: NO (PlayerActivity↓ is not leaking)
│    ↓ PlayerInfo.playerInfoChangeListener
├─ com.aliasgarmurtaza.hifzalquran.activity.PlayerActivity instance
│    Leaking: NO (AppCompatImageView↓ is not leaking and Activity#mDestroyed is false)
│    ↓ PlayerActivity.backIcon
├─ android.support.v7.widget.AppCompatImageView instance
│    Leaking: NO (View attached)
│    mContext instance of com.aliasgarmurtaza.hifzalquran.activity.PlayerActivity with mDestroyed = false
│    View.parent android.widget.RelativeLayout attached as well
│    View#mParent is set
│    View#mAttachInfo is not null (view attached)
│    View.mID = R.id.back_icon
│    View.mWindowAttachCount = 1
│    ↓ AppCompatImageView.mAttachInfo
│                         ~~~~~
├─ android.view.View$AttachInfo instance
│    Leaking: UNKNOWN
│    ↓ View$AttachInfo.mTreeObserver
│                      ~~~~~
├─ android.view.ViewTreeObserver instance
│    Leaking: UNKNOWN
│    ↓ ViewTreeObserver.mOnPreDrawListeners
│                       ~~~~~~~
├─ android.view.ViewTreeObserver$CopyOnWriteArray instance
│    Leaking: UNKNOWN
│    ↓ ViewTreeObserver$CopyOnWriteArray.mData
│                                        ~~~
├─ java.util.ArrayList instance
│    Leaking: UNKNOWN
│    ↓ ArrayList.elementData
│                ~~~~~
├─ java.lang.Object[] array
│    Leaking: UNKNOWN
│    ↓ Object[].[0]
│               ~
├─ com.alexvasilkov.gestures.animation.ViewPositionHolder instance
│    Leaking: UNKNOWN
│    ↓ ViewPositionHolder.view
│                         ~~
├─ com.alexvasilkov.gestures.views.GestureFrameLayout instance
│    Leaking: YES (View detached and has parent)
│    mContext instance of com.aliasgarmurtaza.hifzalquran.activity.PlayerActivity with mDestroyed = false
│    View#mParent is set
│    View#mAttachInfo is null (view detached)
│    View.mID = R.id.gestureFrameLayout
│    View.mWindowAttachCount = 1
│    ↓ GestureFrameLayout.mListenerInfo
├─ android.view.View$ListenerInfo instance
│    Leaking: YES (GestureFrameLayout↑ is leaking)
│    ↓ View$ListenerInfo.mOnClickListener
├─ com.aliasgarmurtaza.hifzalquran.activity.PageFragment_ViewBinding$3 instance
│    Leaking: YES (GestureFrameLayout↑ is leaking)
│    Anonymous subclass of butterknife.internal.DebouncingOnClickListener
│    ↓ PageFragment_ViewBinding$3.val$target
╰→ com.aliasgarmurtaza.hifzalquran.activity.PageFragment instance
​     Leaking: YES (ObjectWatcher was watching this because com.aliasgarmurtaza.hifzalquran.activity.PageFragment received Fragment#onDestroy() callback and Fragment#mFragmentManager is null)
​     key = 90d0ad05-4c7a-459d-a387-5c5a7fbeaa3b
​     watchDurationMillis = 152877
​     retainedDurationMillis = 147877

METADATA

Build.VERSION.SDK_INT: 27
Build.MANUFACTURER: samsung
LeakCanary version: 2.2
App process name: com.aliasgarmurtaza.hifzalquran.debug
Analysis duration: 58252 ms

I am facing a leak too. Not sure if it is the same problem

[skliba]

@alexvasilkov is there a fix for this leak in sight? It's been reported almost 1.5 year ago?

@ThierryD could you elaborate your fix a bit further?

[extmkv]

@alexvasilkov any new on this one?

[alexvasilkov]

Sorry everybody for the long wait, I finally added a fix for this memory leak.
I did some testing from my side but I need you verify if the issue is gone for you in version 2.7.1 now.