๐ 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".