CODEWITHSUNDEEP
androidmemory-leaksleakcanary
frankelot
frankelot

Reputation: 14449

Leak canary, Recyclerview leaking mAdapter

I decided it was high time I learned how to use Leak Canary to detect Leaks within my apps, and as I always do, I tried to implement it in my project to really understand how to use the tool. Implementing it was easy enough, the difficult part was reading what the tool is throwing back at me. I have a scrollview that seems to accumulate memory in the memory manager as I scroll up and down (even though It doesnt load any new data) so I thought that was a good candidate object to track leaks on, this is the result:

enter image description here

It looks like v7.widget.RecyclerView is leaking the adapter, and not my application. But that can't be right.... right?

Here's the code for the adapter and the class making use of it: https://gist.github.com/feresr/a53c7b68145d6414c40ec70b3b842f1e

I started a bounty for this question because it resurfaced after two years on a completely different application

Upvotes: 52

Views: 23210

Answers (5)

Brian Yencho
Brian Yencho

Reputation: 2958

If the adapter lives any longer than the RecyclerView does, you've got to clear the adapter reference in onDestroyView:

@Override
public void onDestroyView() {
    recyclerView.setAdapter(null);
    super.onDestroyView();
}

Otherwise the adapter is going to hold a reference to the RecyclerView which should have already gone out of memory.

Special note prior to androidx.fragment:fragment:1.3.0:

If the screen is involved in transition animations, you actually have to take this one step further and only clear the adapter when the view has become detached:

@Override
public void onDestroyView() {
    recyclerView.addOnAttachStateChangeListener(new View.OnAttachStateChangeListener() {
        @Override
        public void onViewAttachedToWindow(View v) {
            // no-op
        }

        @Override
        public void onViewDetachedFromWindow(View v) {
            recyclerView.setAdapter(null);
        }
    });
    super.onDestroyView();
}

With androidx.fragment:fragment:1.3.0 and above, onDestroyView happens after the view is detached and this extra code is no longer needed.

Upvotes: 89

m_katsifarakis
m_katsifarakis

Reputation: 1907

So, this might be a bit of an overkill solution, but our team was getting pretty tired of having to worry about every single RecyclerView adapter leaking.

Here's an abstract RecyclerView.Adapter subclass that takes care of the leak issue once and for all.

abstract class AbstractRecyclerViewAdapter<VH : RecyclerView.ViewHolder>(fragment: Fragment) :
    RecyclerView.Adapter<VH>() {
    private val fragmentRef = WeakReference(fragment)

    override fun onAttachedToRecyclerView(recyclerView: RecyclerView) {
        super.onAttachedToRecyclerView(recyclerView)
        setupLifecycleObserver(recyclerView)
    }

    private fun setupLifecycleObserver(recyclerView: RecyclerView) {
        val fragment = fragmentRef.get() ?: return
        val weakThis = WeakReference(this)
        val weakRecyclerView = WeakReference(recyclerView)

        // Observe the fragment's lifecycle events in order
        // to set the Recyclerview's Adapter when the Fragment is resumed
        // and unset it when the Fragment is destroyed.
        fragment.viewLifecycleOwner.lifecycle.addObserver(object : LifecycleEventObserver {
            override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) {
                val actualRecyclerView = weakRecyclerView.get() ?: return
                when (event.targetState) {
                    Lifecycle.State.DESTROYED -> actualRecyclerView.adapter = null
                    Lifecycle.State.RESUMED -> {
                        val self = weakThis.get() ?: return
                        if (actualRecyclerView.adapter != self) {
                            actualRecyclerView.adapter = self
                        }
                    }
                    else -> {
                    }
                }
            }
        })
    }
}

Basically the Adapter observes the lifecycle events of the Fragment that contains it and takes care of setting and unsetting itself as the RecyclerView's adapter.

All you need to do is subclass AbstractRecyclerViewAdapter in your own adapters, and provide the container fragment when you construct them.

Usage:

Your adapter

class MyAdapter(fragment: Fragment): AbstractRecyclerViewAdapter<MyViewHolder>(fragment) {
    // Your usual adapter code...
}

Your fragment

class MyFragment : Fragment {
    // Instantiate with the fragment.
    private val myAdapter = MyAdapter(this)

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        // Set the RecyclerView adapter as you would normally.
        myRecyclerView.adapter = myAdapter
    }
}

Upvotes: 1

savepopulation
savepopulation

Reputation: 11921

I can't open your image and see the actual leak but if you define a local variable for your RecyclerView in your Fragment and set your fragment's retainInstanceState true it can cause possible leaks with rotation.

When using a Fragment with retainInstance you should clear all your ui references in onDestroyView

@Override
public void onDestroyView() {
     yourRecyclerView = null;
     super.onDestroyView();
}

Here you can find a detailed information from this link: Retained Fragments with UI and memory leaks

Upvotes: 1

Bolein95
Bolein95

Reputation: 3056

I was able to fix this by overriding RecyclerView. It happens because RecyclerView never unregisters itself from AdapterDataObservable.

@Override protected void onDetachedFromWindow() {
    super.onDetachedFromWindow();
    if (getAdapter() != null) {
        setAdapter(null);
    }
}

Upvotes: 19

maciekjanusz
maciekjanusz

Reputation: 4775

First of all, I'm referencing this file.

It looks like v7.widget.RecyclerView is leaking the adapter, and not my application. But that can't be right.... right?

It's actually your adapter that's leaking the RecyclerView (and it's made pretty clear by the trace graph and the title of the LeakCanary activity). However, I'm not sure if it's the "parent" RecyclerView or the nested one in the HourlyViewHolder, or both. I think the culprits are your ViewHolders. By making them non-static inner classes, you explicitly provide them with the reference to the enclosing adapter class, ant this almost directly couples the adapter with the recycled views, since the parent of every itemView in your holders is the RecyclerView itself.

My first suggestion to fix this problem would be to decouple your ViewHolders and Adapter by making them static inner classes. That way they do not hold a reference to the adapter, so your context field will be inaccessible to them, and it's also a good thing, since context references should be passed and stored sparingly (also to avoid big memory leaks). When you need the context just to obtain the strings, do it somewhere else, for example in the adapter constructor, but do not store the context as a member. Finally, the DayForecastAdapter seems dangerous too: you pass the one, same instance of it to every HourlyViewHolder, which seems like a bug.

I think that fixing the design and decoupling these classes should get rid of this memory leak

Upvotes: 6

Related Questions