Memory leak at LottieView#setAnimationFromUrl

[mateuszkwiecinski]

Lottie is supported and developed on nights and weekends. Issues from Lottie sponsors will be prioritized.

If you don't use this template, your issue will be closed. Delete this text once completed.

Checklist

1. Fork this repository into your account.
2. Reproduce your issue in the issue-repro module. Issues without a repro in the issue-repro
   module may be auto-closed.

Link to fork with a repro in the issue-repro module
https://github.com/airbnb/lottie-android/compare/master...mateuszkwiecinski:lottie-android:memory_leak?expand=1 or d925e25

Describe the bug
👋
I think I spotted a memory leak happening when you call setAnimationFromUrl and destroy the view/activity before the resource finishes loading. If the resource fails to load or completes successfully, all resources are properly disposed, I think.

Steps To Reproduce
Steps to reproduce the behavior:

1. Go to Developer settings, enable don't keep activities option, (to make sure activities are immediately destroyed)
2. Update emulator settings to have slow network connection.
3. Click Open button
4. Navigate back before image completes
5. Repeat points 2. and 3. couple of times until LeakCanary leak notification appears, or tap the Dump button

Couple of notes on the repro:

1. don't keep activities option is required because that's how Android framework manages activity lifecycle, but it's unrelated to the issue. Popping up Fragments, or removing views reproduces the issue the same way. It was just easier to adapt to predefines shape of the repro
2. I'm calling binding.root.tag = this@IssueReproActivity to rely on default Leakcanary watchers, but again, it's unrelated to the source issue. If you start watching LottieAnimationView instance, it will also live after its activity is destroyed.
   In my original issue, the project uses databinding which sets binding.lifecycleOwner = this which is somewhat the same behavior I did in the reproducer.
3. Ignore the commented out code, the build doesn't seem to be compatible with configuration cache nor Java 20 so it was needed to make the repro project compile

LeakCanary dump

┬───
│ GC Root: Thread object
│
├─ android.os.HandlerThread instance
│    Leaking: NO (PathClassLoader↓ is not leaking)
│    Thread name: 'plumber-android-leaks'
│    ↓ Thread.contextClassLoader
├─ dalvik.system.PathClassLoader instance
│    Leaking: NO (LottieCompositionFactory↓ is not leaking and A ClassLoader is
│    never leaking)
│    ↓ ClassLoader.runtimeInternalObjects
├─ java.lang.Object[] array
│    Leaking: NO (LottieCompositionFactory↓ is not leaking)
│    ↓ Object[12]
├─ com.airbnb.lottie.LottieCompositionFactory class
│    Leaking: NO (a class is never leaking)
│    ↓ static LottieCompositionFactory.taskCache
│                                      ~~~~~~~~~
├─ java.util.HashMap instance
│    Leaking: UNKNOWN
│    Retaining 128 B in 3 objects
│    ↓ HashMap["url_https://fastupload.
│             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
│    io/download002/N2p3DJQZdGMa5/r8HaDIRvlULrvB7/34462-html-url.json"]
├─ com.airbnb.lottie.LottieTask instance
│    Leaking: UNKNOWN
│    Retaining 239.0 kB in 3342 objects
│    ↓ LottieTask.failureListeners
│                 ~~~~~~~~~~~~~~~~
├─ java.util.LinkedHashSet instance
│    Leaking: UNKNOWN
│    Retaining 301 B in 10 objects
│    ↓ LinkedHashSet[element()]
│                   ~~~~~~~~~~~
├─ com.airbnb.lottie.LottieAnimationView$1 instance
│    Leaking: UNKNOWN
│    Retaining 12 B in 1 objects
│    Anonymous class implementing com.airbnb.lottie.LottieListener
│    ↓ LottieAnimationView$1.this$0
│                            ~~~~~~
├─ com.airbnb.lottie.LottieAnimationView instance
│    Leaking: YES (View.mContext references a destroyed activity)
│    Retaining 58.5 kB in 818 objects
│    View is part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mID = R.id.animation_view
│    View.mWindowAttachCount = 1
│    mContext instance of com.airbnb.lottie.issues.IssueReproActivity with
│    mDestroyed = true
│    ↓ View.mContext
╰→ com.airbnb.lottie.issues.IssueReproActivity instance
​     Leaking: YES (ObjectWatcher was watching this because com.airbnb.lottie.
​     issues.IssueReproActivity received Activity#onDestroy() callback and
​     Activity#mDestroyed is true)
​     Retaining 11.0 kB in 287 objects
​     key = 9171f52f-6623-498d-9adc-832666fc0664
​     watchDurationMillis = 5283
​     retainedDurationMillis = 241
​     mApplication instance of android.app.Application
​     mBase instance of androidx.appcompat.view.ContextThemeWrapper

METADATA

Build.VERSION.SDK_INT: 28
Build.MANUFACTURER: Google
LeakCanary version: 2.10
App process name: com.airbnb.lottie.issues
Class count: 12281
Instance count: 132614
Primitive array count: 108537
Object array count: 10948
Thread count: 20
Heap total bytes: 13096344
Bitmap count: 0
Bitmap total bytes: 0
Large bitmap count: 0
Large bitmap total bytes: 0
Stats: LruCache[maxSize=3000,hits=30929,misses=58107,hitRate=34%]
RandomAccess[bytes=2948224,reads=58107,travel=17425481048,range=17291233,size=21
495639]
Analysis duration: 5910 ms

[gpeal]

My guess is that this is transient and not really a leak. Lottie doesn't have a reference to the calling activity's lifecycle so it may hold a reference to its listeners until the request finishes but it should be okay once it completes. I may not have time to look at this right away and suspect that it would have come up before if it was more than a transient reference. However, if you could determine if that's the case, it would be helpful.

[mateuszkwiecinski]

Yeah, you're absolutely right - it is a transient state, but at the same time this is textbook example of a memory leak since it keeps reference to a view after the view should be garbage collected 😅

I'm not that familiar with the implementation, but I'd expect a mix of:

1. Having a lifecycle-aware API to "scope" lottie background work with a lifecycle-aware component
2. Having a dedicated api to manually cancel all background work. Right now the only workaround I was able to identify is to call (IIRC) ImageVIew#setImageDrawable with null argument which cancels the loader task. There is no "official" way to cancel ongoing background work to release all references, right?
3. Storing the view reference as a WeakReference, just to allow garbage collector do their job.
4. Register a OnAttachStateChangeListener and do a clean-up when view is detached from window. I recall Glide does something similar 👀

may hold a reference to its listeners until the request finishes

I might be wrong here, but if someone tried to react on a completion event after the owning parent had been destroyed (i.e. to commit a fragment transaction) it would lead to a runtime crash. So the fact it's just a temporary state doesn't fully justify keeping dead listeners 👀

suspect that it would have come up before

Just for context, I think this was reported already here: #1753

[gpeal]

A WeakRef may be useful here. This will be fairly low on my priority list since it is transient and unlikely to cause actual issues, just some LeakCanary noise. If you want to put up a PR, I could take a look. I would consider the WeakRef solution first because it requires no API changes and should cover any case.

[mateuszkwiecinski]

I took a stab at it in #2293
I'm not observing the leak in the reproducer project I shared above 🤞