airbnb / epoxy

Epoxy is an Android library for building complex screens in a RecyclerView
https://goo.gl/eIK82p
Apache License 2.0
8.47k stars 730 forks source link

Possible memory leak in ActivityRecyclerPool with Dagger Hilt #1025

Open almeric opened 3 years ago

almeric commented 3 years ago

I've just migrated one of our apps to Dagger Hilt and now LeakCanary 2 is finding leaks in our fragments that use Epoxy.

The issue seems very similar to #886, but instead of a ContextWrapper Hilt gives us an FragmentContextWrapper instead. See here: https://github.com/google/dagger/blob/71509b841b30909ddf6cb36ed366d4a609d84071/java/dagger/hilt/android/internal/managers/ViewComponentManager.java#L169

LeakCanary trace

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

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

    12178 bytes retained by leaking objects
    Signature: d32f1ebcb86e6f7f2a64673f70a37d6ad74352f3
    ┬───
    │ GC Root: Input or output parameters in native code
    │
    ├─ okio.AsyncTimeout class
    │    Leaking: NO (PathClassLoader↓ is not leaking and a class is never leaking)
    │    ↓ static AsyncTimeout.$class$classLoader
    ├─ dalvik.system.PathClassLoader instance
    │    Leaking: NO (EpoxyRecyclerView↓ is not leaking and A ClassLoader is never leaking)
    │    ↓ PathClassLoader.runtimeInternalObjects
    ├─ java.lang.Object[] array
    │    Leaking: NO (EpoxyRecyclerView↓ is not leaking)
    │    ↓ Object[].[1495]
    ├─ com.airbnb.epoxy.EpoxyRecyclerView class
    │    Leaking: NO (a class is never leaking)
    │    ↓ static EpoxyRecyclerView.ACTIVITY_RECYCLER_POOL
    │                               ~~~~~~~~~~~~~~~~~~~~~~
    ├─ com.airbnb.epoxy.ActivityRecyclerPool instance
    │    Leaking: UNKNOWN
    │    ↓ ActivityRecyclerPool.pools
    │                           ~~~~~
    ├─ java.util.ArrayList instance
    │    Leaking: UNKNOWN
    │    ↓ ArrayList.elementData
    │                ~~~~~~~~~~~
    ├─ java.lang.Object[] array
    │    Leaking: UNKNOWN
    │    ↓ Object[].[2]
    │               ~~~
    ├─ com.airbnb.epoxy.PoolReference instance
    │    Leaking: UNKNOWN
    │    ↓ PoolReference.viewPool
    │                    ~~~~~~~~
    ├─ com.airbnb.epoxy.UnboundedViewPool instance
    │    Leaking: UNKNOWN
    │    ↓ UnboundedViewPool.scrapHeaps
    │                        ~~~~~~~~~~
    ├─ android.util.SparseArray instance
    │    Leaking: UNKNOWN
    │    ↓ SparseArray.mValues
    │                  ~~~~~~~
    ├─ java.lang.Object[] array
    │    Leaking: UNKNOWN
    │    ↓ Object[].[0]
    │               ~~~
    ├─ java.util.LinkedList instance
    │    Leaking: UNKNOWN
    │    ↓ LinkedList.first
    │                 ~~~~~
    ├─ java.util.LinkedList$Node instance
    │    Leaking: UNKNOWN
    │    ↓ LinkedList$Node.item
    │                      ~~~~
    ├─ com.airbnb.epoxy.EpoxyViewHolder instance
    │    Leaking: UNKNOWN
    │    ↓ EpoxyViewHolder.itemView
    │                      ~~~~~~~~
    ├─ androidx.constraintlayout.widget.ConstraintLayout instance
    │    Leaking: UNKNOWN
    │    mContext instance of dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper, wrapping activity com.example.activity with mDestroyed = false
    │    View#mParent is null
    │    View#mAttachInfo is null (view detached)
    │    View.mWindowAttachCount = 1
    │    ↓ ConstraintLayout.mContext
    │                       ~~~~~~~~
    ├─ dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper instance
    │    Leaking: UNKNOWN
    │    ViewComponentManager$FragmentContextWrapper wraps an Activity with Activity.mDestroyed false
    │    ↓ ViewComponentManager$FragmentContextWrapper.fragment
    │                                                  ~~~~~~~~
    ╰→ com.example.fragment instance
    ​     Leaking: YES (ObjectWatcher was watching this because com.example.fragment received Fragment#onDestroy() callback and Fragment#mFragmentManager is null)
    ​     key = 92a1a7cd-0889-4655-a7f2-a3a7e5d06e77
    ​     watchDurationMillis = 20471
    ​     retainedDurationMillis = 15469
    ====================================
    0 LIBRARY LEAKS

This happens in both the latest 4.0 beta & latest stable version of Epoxy

One way I've found to fix it is using a different context when inflating the view.

Instead of using the supplied LayoutInflater, fetching it from the activity seems to solve the issue:

override fun onCreateView(
        inflater: LayoutInflater,
        container: ViewGroup?,
        savedInstanceState: Bundle?
    ): View? {
        binding = MyFragmentLayoutBinding.inflate(
            requireActivity().layoutInflater, // fix here
            container,
            false
        )
        return binding.root
    }
R4md4c commented 3 years ago

Facing the same problem as well with Hilt's FragmentContextWrapper, however I'm not sure if that's Epoxy's fault or Hilt's.

Maybe If Hilt somehow can provide a way to disable the usage of that wrapper since not all views needs to be injected, that would solve the problem.

R4md4c commented 3 years ago

Clearing the adapter in onDestroyView seems to fix the problem without the need to use the Activity's LayoutInflater.

override fun onDestroyView() {
        requireView().findViewById<RecyclerView>(R.id.epoxy_view).adapter = null
        super.onDestroyView()
    }
elihart commented 3 years ago

From https://github.com/google/dagger/issues/2070

Our initial assumption was that a view created by a fragment shouldn't outlive the fragment, so there should never be a leak. Keeping with that assumption, you can avoid the issue by explicitly using the activity context when inflating a view that is meant to outlive its parent fragment.

The point of EpoxyRecyclerView's shared view pool is to reuse views across all fragments in an Activity as an optimization. Hilt's design breaks this, as they are make a bad assumption (which they recognize is an issue).

Besides the work around of inflating your views with the activity inflater, you can also disable Epoxy's context wide view recycling - EpoxyRecyclerView#shouldShareViewPoolAcrossContext

If you care about this it may also be helpful for you to weigh in on the Hilt issue (https://github.com/google/dagger/issues/2070) and encourage them to fix it on their side.

SatyajeetMohalkar10 commented 1 year ago

@elihart can you please tell me how to disable Epoxy's context wide view recycling - EpoxyRecyclerView#shouldShareViewPoolAcrossContext???