airbnb / epoxy

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

Nested Carousel Memory leak #1088

Open ThomasArtProcessors opened 4 years ago

ThomasArtProcessors commented 4 years ago

Hi there. I'm currently using epoxy a recyclerview that is hosting a set of views which I am creating directly via data binding.

 recyclerNearbyList.withModels {
    [...]
       groupCarousel {
               id(item.id)
                onBind { model, view, position -> view.setFullSpan(true)}
                data(item)
        }
}

groupCarousel is coming directly from a layout with databinding. I'm not using models directly nor am I using a controller directly, it's all done with databinding layouts and the .withModels The viewholder for groupCarousel looks like this:

<?xml version="1.0" encoding="utf-8"?>
<layout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto">

    <data>

        <variable
            name="data"
            type="some.path.to.GroupCarousel" />
    </data>

    <androidx.constraintlayout.widget.ConstraintLayout
        android:layout_width="match_parent"
        android:layout_height="200dp">

        <View android:id="@+id/separator"
            android:layout_width="match_parent"
            android:layout_height="@dimen/separator"
            android:background="?colorSecondary"
            app:layout_constraintTop_toTopOf="parent" />

        <some.path.VerticalStackView
            android:id="@+id/header"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            app:layout_constraintTop_toBottomOf="@+id/separator"
            app:content="@{data.header}" />

        <some.path.GroupCarousel
            android:id="@+id/carouselGroup"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:orientation="horizontal"
            app:layoutManager="LinearLayoutManager"
            app:layout_constraintTop_toBottomOf="@+id/header"
            app:data="@{data.rows}"/>

    </androidx.constraintlayout.widget.ConstraintLayout>
</layout>

the GroupCarousel is a custom view extending epoxy's Carousel. In it I have a setData function that receives the data binding data set in the layout above. The function looks like this:

fun setData(
        data: List<NearbyListViewModel.NearbyListItem.GroupCarousel.Row>,
    ) {
        items = data

        setModels(
            items.flatMap { element ->
                element.images.map {  image ->
                    val scaledImage = image.copy(
                        (element.height.toFloat() / image.getRatio()).toInt(),
                        element.height,
                        image.path
                    )
                    ImageBindingModel_().apply {
                        id(image.path)
                        data(scaledImage)
                    }
                }
            }
        )
    }
}

I have two issues: groupCarousel doesn't seem to remember its horizontal scroll position. But more importantly I get a memory leak, and I'm not sure how I could use the ol' trick of setting the adapter to null since those carousels are views inside the groupCarousel databinding model.

Here is the leak:

┬───
│ GC Root: Local variable in native code
│
├─ GB instance
│    Leaking: NO (PathClassLoader↓ is not leaking)
│    Thread name: 'CleanupReference'
│    ↓ GB.contextClassLoader
├─ 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[].[698]
├─ 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[].[2]
│               ~~~
├─ java.util.LinkedList instance
│    Leaking: UNKNOWN
│    ↓ LinkedList.last
│                 ~~~~
├─ 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 MainActivity with mDestroyed = false
│    View#mParent is null
│    View#mAttachInfo is null (view detached)
│    View.mWindowAttachCount = 44
│    ↓ ConstraintLayout.mLayoutParams
│                       ~~~~~~~~~~~~~
├─ androidx.recyclerview.widget.StaggeredGridLayoutManager$LayoutParams instance
│    Leaking: UNKNOWN
│    ↓ StaggeredGridLayoutManager$LayoutParams.mSpan
│                                              ~~~~~
├─ androidx.recyclerview.widget.StaggeredGridLayoutManager$Span instance
│    Leaking: UNKNOWN
│    ↓ StaggeredGridLayoutManager$Span.this$0
│                                      ~~~~~~
├─ androidx.recyclerview.widget.StaggeredGridLayoutManager instance
│    Leaking: UNKNOWN
│    ↓ StaggeredGridLayoutManager.mRecyclerView
│                                 ~~~~~~~~~~~~~
├─ com.airbnb.epoxy.EpoxyRecyclerView instance
│    Leaking: YES (View detached and has parent)
│    mContext instance of dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper, wrapping activity MainActivity with mDestroyed = false
│    View#mParent is set
│    View#mAttachInfo is null (view detached)
│    View.mID = R.id.recyclerNearbyList
│    View.mWindowAttachCount = 1
│    ↓ EpoxyRecyclerView.mParent
╰→ androidx.constraintlayout.widget.ConstraintLayout instance
​     Leaking: YES (ObjectWatcher was watching this because NearbyListFragment received Fragment#onDestroyView() callback (references to its views should be cleared to prevent leaks))
​     key = 20af51a4-0729-4ac0-814a-f4438622224d
​     watchDurationMillis = 12040
​     retainedDurationMillis = 7038
​     mContext instance of dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper, wrapping activity MainActivity with mDestroyed = false
​     View#mParent is null
​     View#mAttachInfo is null (view detached)
​     View.mWindowAttachCount = 1
eboudrant commented 3 years ago

I think it is similar to #1025. See this as a workaround (not ideal for nested carousel) : https://github.com/airbnb/epoxy/issues/1025#issuecomment-733159885

ThomasArtProcessors commented 3 years ago

I had to do a pretty heavy and inconvenient work around. First, I don't set the data in the databinding directly anymore. Instead I set it each time in onBind, and remove the adapter in onUnbind

                          onBind { model, view, position ->
                                val carousel = view.dataBinding.root
                                    .findViewById<GroupCarousel>(R.id.carouselGroup)
                                carousel.setData(item, carouselScroll[item.id],
                                    viewModel.shouldShowGroupTooltip())
                                view.setFullSpan(true)
                            }
                            onUnbind { model, view ->
                                val carousel = view.dataBinding.root
                                    .findViewById<GroupCarousel>(R.id.carouselGroup)
                                carousel.cleanUp() // clean up to avoid memory leaks from Epoxy's Carousel
                                carouselScroll[item.id] = carousel.scrollData
                                if(carousel.hasScrolled) viewModel.hasSeenGroupTooltip()
                            }

You might notice the cleanUp method, which is in the View itself, which extends Carousel. In that view I do a few things: I manually create a Controller and adapter if needed:

if (controller == null) {
            controller = SimpleEpoxyController()
            adapter = controller!!.adapter
[...]

And then I call that cleanUp method on onUnbind

fun cleanUp() {
        clearOnScrollListeners()
        controller?.cancelPendingModelBuild()
        controller = null
        swapAdapter(null, true)
    }

That plus I do some manual calculations to remember the horizontal scroll of the Carousel which is otherwise lost. All in all not ideal but this fixed the memory leak and it properly remembers the horizontal position.