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

Possible memory leak in ActivityRecyclerPool with Dagger Hilt #1025

Open
almeric opened this issue Jul 31, 2020 · 4 comments
Open

Possible memory leak in ActivityRecyclerPool with Dagger Hilt #1025

almeric opened this issue Jul 31, 2020 · 4 comments

Comments

@almeric
Copy link

almeric commented Jul 31, 2020

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
Copy link

R4md4c commented Aug 6, 2020

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
Copy link

R4md4c commented Aug 7, 2020

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
Copy link
Contributor

elihart commented Nov 24, 2020

From google/dagger#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 (google/dagger#2070) and encourage them to fix it on their side.

@SatyajeetMohalkar10
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants