👋 Hi, this is P.Y., I work as an Android Distinguished Engineer at Block. Every month I organize an internal Ensemble Leak Hunting session where we look at the top leaks reported by LeakCanary to learn how to read and investigate leak traces. This is a write-up of our latest investigation which surfaced a common mistake when using Picasso.

Before we start, you might want to get a quick refresher on how to read leak traces. In this article, we'll find the root cause of the leak, discuss how to fix it, and what could change in Picasso to help avoid this common mistake.

Here's the leak trace:

┬───
│ GC Root: System class
│
├─ com.example.PicassoHolder class
│    Leaking: NO (a class is never leaking)
│    ↓ static PicassoHolder.singleton
├─ com.squareup.picasso3.Picasso instance
│    ↓ Picasso.targetToAction
│              ~~~~~~~~~~~~~~
├─ java.util.WeakHashMap instance
│    ↓ WeakHashMap.table
│                  ~~~~~
├─ java.util.WeakHashMap$Entry[] array
│    ↓ WeakHashMap$Entry[0]
│                       ~~~
├─ java.util.WeakHashMap$Entry instance
│    ↓ WeakHashMap$Entry.value
│                        ~~~~~
├─ com.squareup.picasso3.ImageViewAction instance
│    ↓ ImageViewAction.callback
│                      ~~~~~~~~
├─ com.example.ProfileLayout$loadImage$1 instance
│    Anonymous class implementing com.squareup.picasso3.Callback
│    ↓ ProfileController$loadImage$1.this$0
│                                    ~~~~~~
╰→ com.example.ProfileLayout instance
​     Leaking: YES (ObjectWatcher was watching this because ProfileLayout received View#onDetachedFromWindow() callback)

Picasso singleton

┬───
│ GC Root: System class
│
├─ com.example.PicassoHolder class
│    Leaking: NO (a class is never leaking)
│    ↓ static PicassoHolder.singleton
├─ com.squareup.picasso3.Picasso instance
...

At the top of the leak trace, a Picasso instance. We know that Picasso instances are long-lived singletons, meant to stay around as UI comes and goes. So we know this is legit and we can move the investigation further down in the leak trace.

Picasso.targetToAction

...
├─ com.squareup.picasso3.Picasso instance
│    ↓ Picasso.targetToAction
│              ~~~~~~~~~~~~~~
├─ java.util.WeakHashMap instance
│    ↓ WeakHashMap.table
│                  ~~~~~
├─ java.util.WeakHashMap$Entry[] array
│    ↓ WeakHashMap$Entry[0]
│                       ~~~
├─ java.util.WeakHashMap$Entry instance
│    ↓ WeakHashMap$Entry.value
│                        ~~~~~
├─ com.squareup.picasso3.ImageViewAction instance
...

Picasso.targetToAction is a WeakHashMap of target keys (i.e. views) to action values (i.e. what to load on those views). WeakHashMap has weak keys and strong values, i.e. keys are held by weak references, and when a weak reference to a key clears, its entry is removed. So Picasso.targetToAction holds weak references to the target views & strong references to the corresponding actions.

ImageViewAction.callback

...
├─ com.squareup.picasso3.ImageViewAction instance
│    ↓ ImageViewAction.callback
│                      ~~~~~~~~
├─ com.example.ProfileLayout$loadImage$1 instance
│    Anonymous class implementing com.squareup.picasso3.Callback
│    ↓ ProfileController$loadImage$1.this$0
│                                    ~~~~~~
╰→ com.example.ProfileLayout instance
​     Leaking: YES (ObjectWatcher was watching this because ProfileLayout received View#onDetachedFromWindow() callback)

ImageViewAction.callback is a Picasso action that holds a custom callback created in ProfileLayout.loadImage(). We can see that this custom Picasso callback is an anonymous class that has a reference to its outer class, ProfileLayout. We know ProfileLayout has been detached for at least 5 seconds as that's LeakCanary's minimum trigger duration. The ProfileLayout.loadImage() code shows that the callback sets a custom background color when the image fails to load:

fun loadImage() {
  picasso.load(imageUri)
    .into(
      imageView,
      object : Callback {
        override fun onSuccess() = Unit

        override fun onError(t: Throwable) {
          // Set dark background if image fails to load
          imageView.setBackgroundColor(backgroundColor)
        }
      }
    )
}

Looking at the Picasso source code we can see that Picasso.targetToAction entries are removed when a request completes, so we can conclude that the image loading request was still in flight and that ProfileLayout was detached and prevented from being garbage collected by that request in flight.

Fix: cancelation

It's a common mistake when using Picasso: image-loading requests should be canceled if the UI goes away before the request completes, otherwise, they'll keep going and consume resources until success or failure, which could take a long time on a slow network.

We forgot to cancel the request! Let's fix that:

override fun onDetachedFromWindow() {
  super.onDetachedFromWindow()
  picasso.cancelRequest(imageView)
}

This fixes the leak, yay! Alternatively, we could also use a tag.

Leaks everywhere?

Forgetting to cancel Picasso requests when the UI goes away is a fairly common mistake. How comes leaks don't show up more often?

First, Picasso automatically cancels any previous request when loading a new image on the same image view. This helps with adapter views: when a list item view gets recycled and bound to a new row, a new request is started for that view and the previous one is automatically canceled.

Second, even if we forget to cancel a Picasso request, that will only trigger a leak when using a custom callback with a strong reference to the detached ImageView. How comes?

WeakHashMap

Picasso.targetToAction is a WeakHashMap that holds weak references to the target views & strong references to the corresponding actions.

ImageViewAction itself (source) holds a weak reference to its target ImageView, and a strong reference to its optional Callback. So when the custom callback isn't set, ImageViewAction doesn't hold any strong reference to the view:

internal class ImageViewAction(
  picasso: Picasso,
  target: ImageView,
  data: Request,
  var callback: Callback?
) : Action(picasso, data) {
  private val targetReference = WeakReference(target)

As you can see, so far there are no strong references to any view. This means that the default usage of Picasso will not introduce leaks, even if we forget to cancel requests when UI goes away.

Now let's add a custom callback with a strong reference to a view:

Note that if we replaced the callback strong reference with a weak reference, there would be no strong reference holding the callback in memory and it would be garbage collected before the request completes, even when the view is still attached. That's not what we want. So we do need the callback strong reference, which unfortunately means we have a strong reference path to ProfileLayout and its associated view hierarchy, which causes a leak.

This is exactly what the WeakHashMap documentation warns us about:

The value objects in a WeakHashMap are held by ordinary strong references. Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded.

View.setTag(int, Object)

Ideally Picasso would have a consistent memory behavior whether or not callback is set. One way to do that is to store the ImageViewAction directly in the ImageView, as a view tag. That way, Picasso.targetToAction would only hold a weak reference to the ImageView and as soon as the view is detached it becomes unreachable:

Auto cancel on detach

Alternatively, Picasso could set a detach listener on the ImageView and auto cancel the requests on detach. This would be best for saving resources but might create surprises if e.g. the app is preloading an image into a detached view.

That's all for now, hope you enjoyed reading this!

Header image generated by DALL-E, prompt: "image inspired by the style of Pablo Picasso, depicting a leaky faucet with bold, abstract shapes and bold colors".