Glide: ViewTarget MemoryLeak

[DenBond7]

Glide Version: com.github.bumptech.glide:glide:3.7.0
Integration libraries:

    debugCompile 'com.squareup.leakcanary:leakcanary-android:1.4-beta1'
    compile fileTree(dir: 'libs', include: ['*.jar'])
    compile 'com.android.support:appcompat-v7:23.1.1'
    compile 'com.github.bumptech.glide:glide:3.7.0'
    compile 'com.github.bumptech.glide:okhttp-integration:1.3.1@aar'
    compile 'com.google.code.gson:gson:2.6.2'
    compile 'com.squareup.retrofit:retrofit:1.9.0'
    compile 'com.squareup.okhttp:okhttp:2.5.0'
    compile 'com.squareup.okhttp:okhttp-urlconnection:2.5.0'
    compile 'com.android.support:recyclerview-v7:23.1.1'
    compile 'com.android.support:cardview-v7:23.1.1'
    compile 'com.android.support:design:23.1.1'

Device/Android Version: Android 4.1.1 tested, Genemotion
Issue details / Repro steps / Use case background:

1. Open my app
2. Open an activity where showing RecycelerView with images
3. Then press Back -> Back -> .... Home Screen.
4. After I wait about 1 minute and press to run GC.
5. I have leakcanary. I saw next memory leak.

1. Then I collected glide.hprof and saw a memory leak.
   _(SELECT * FROM instanceof android.support.v7.app.AppCompatActivity)_

Glide load line:
Next code running in RecyclerView.Adapter.

Glide.with(context)
        .load(images.get(i))
        .error(android.R.drawable.ic_delete)
        .centerCrop()
        .into(viewHolder.imageViewCar);

context - GalleryFragment -> original methot getContext() from android.support.v4.app.Fragment;

Layout XML:

<?xml version="1.0" encoding="utf-8"?>
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    android:layout_width="match_parent"
    android:layout_height="match_parent">

    <android.support.v7.widget.CardView
        android:id="@+id/cardViewGalleryItem"
        android:layout_width="match_parent"
        android:layout_height="wrap_content">

        <ImageView
            android:id="@+id/imageViewCar"
            android:layout_width="match_parent"
            android:layout_height="150dp"
            android:scaleType="centerCrop"
            tools:src="@drawable/notification_template_icon_bg" />
    </android.support.v7.widget.CardView>
</RelativeLayout>

Link to test project

[TWiStErRob]

Sorry for the late reply. I got stuck in my queue.
I cloned your sample app and fixed it so it can run: DenBond7/GlideViewTargetMemoryLeakProject#1.

I tried on my physical Galaxy S4 4.4.2 and I couldn't reproduce. I noticed on this device that canary was leaking too, which was weird:

probably that's how they report stuff, it has to stay in memory. Anyway, I removed the canary dependency to not interfere. Still no repro, but I noticed on S4 that the Main activity is leaked, but that's probably a vendor issue, because it happens a lot when I look at dumps.

Then I fired up my HTC One X 4.1.1 on Genymotion and I was able to repro it there. At first it looked like for some reason the queue still had an unprocessed item in it, but the PriorityBlockingQueue is not returning it, but that was because my source (API 23) didn't match the running code (API 16).

I made some changes during the investigation which helped me dig deeper:

• Renamed the package because the heap analyzer couldn't fit the names.
• Downgraded to support-18 and targetApi=16 so I can see more real source code
  (e.g. FutureTask$Sync shown in heap dump was non-existent in sources/android-23)
• Remove CardView/design for API 16 compatibility
• Inlined some classes so I can modify their code for debugging

The thing that finally triggered me to look in the right direction is the non-understanding of this data:

If you look at it it's clear that SecondActivity is being held by MyExecutor$LoadTask which has depth=0. The reference chain is clearly readable, but that depth value didn't make sense. The Android plugin in IDEA doesn't show the full truth, but it was easier to acquire and refresh than in MAT. After I checked in MAT I saw that <Java Local> (seen on your shot as well) which lead me to http://stackoverflow.com/a/22491546/253468 explaining what that means. So now we know that the LoadTask instance is held by a local variable in thread fifo-pool-thread-1. The question now: "Which stack frame?". I walked around the stack in Debug after pausing that thread, but couldn't find the local variable that may be holding it. LoadTask is a Comparable, FutureTask, RunnableFuture, Runnable, Future. The frames look like this:

Candidates are:

• task in ThreadPoolExecutor.runWorker:
  task is null, otherwise getTask wouldn't be called
• r in ThreadPoolExecutor.getTask:
  r is uninitialized because take is called in the initializer of that variable
• result in PriorityBlockingQueue<Runnable>:
  result is null because extract() must have returned null in order to go for notEmpty.await()

My debugger doesn't work most of the time: I can't step over, step into or step out; breakpoints are hit but sometimes even that hangs IDEA/Genymotion. Based on my semi-static analysis Glide cannot do anything about this.

_The question is still open_: Which stack frame is holding the reference?

[TWiStErRob]

At first it looked like for some reason the queue still had an unprocessed item in it, but the PriorityBlockingQueue is not returning it, but that was because my source (API 23) didn't match the running code (API 16).

This statement was really bugging me so I continued the investigation, I reset my understanding and re-read all the references:

All the expanded ones are ok, references, mostly circular ones, but the selected on is queue[0] which means it's still in the queue. The trick here is that the queue's size() is 0, but the Heap-backing array still has this Runnable at position 0. To find out how this can happen we need to look at extract and siftDownComparable when the last take() is executed (when there's actually still one item available):

private E extract() {
    E result;
    int n = size - 1; // size == 1, n == 0
    if (n < 0)
        result = null;
    else { // this branch executes
        Object[] array = queue;
        result = (E) array[0]; // this is our leaked LoadTask
        E x = (E) array[n]; // this is our leaked LoadTask too
        array[n] = null; // cleared last LoadTask, so far so good
        Comparator<? super E> cmp = comparator;
        if (cmp == null) // we're using comparable
            siftDownComparable(0, x, array, n);
        else
            siftDownUsingComparator(0, x, array, n, cmp);
        size = n; // ... but we remember that this LoadTask is now removed
    }
    return result;
}

private static <T> void siftDownComparable(int k, T x, Object[] array, int n) {
    // siftDownComparable(0, LoadTask to be returned in extract, queue, 0)
    Comparable<? super T> key = (Comparable<? super T>)x; // key == leaked LoadTask
    int half = n >>> 1;           // half of 0 is 0
    while (k < half) { // 0 < 0 is false
        // ommited because irrelevant
    }
    array[k] = key; // whops, leaked LoadTask is put back into the array...
    // return to extract()
}

After discovering this I diffed sdk/sources/android-16/java/util/concurrent/PriorityBlockingQueue.java and the same file android-19 and found this beauty: https://android.googlesource.com/platform/libcore/+/91770798d8b9280d48d30df2ed7f63b3ed9b036f%5E%21/#F34 Notice the added if (n > 0) guard. This was first relesed in API 19. I managed to work around this in DenBond7/GlideViewTargetMemoryLeakProject#2, but the leak is still comes back because those <Java Local> references are still there.

There is a reference to FutureTask$Sync which has this$0 to our leaked LoadTask and directly to LoadTask.

[Oyashirox]

Im having the exact same stacktrace when testing on 4.1.1 with Genymotion.

Is there any workaround ?

[TWiStErRob]

https://github.com/DenBond7/GlideViewTargetMemoryLeakProject/pull/2/files#diff-2e3a2c1512e83284e3f9f499e9ea0aa0 is as far as I went. Based on my last comment it's not possible to fix it, just accept that the activity will be temporarily leaked on old devices, until the next load which should overwrite the reference and the local variables.