Clean up GifDrawable resources more reliably.

Each time we call get() on a drawable resource, we
get a new Drawable. We call get() repeatedly on
resources when they are retrieved from either the
set of active resources or the in memory cache.
Each time we create a new GifDrawable it holds on
to one or two temporary Bitmaps outside it's
shared state to render the current frame and obey
the dispose_previous method. This change means we
more aggressively cleanup those resources when
we think each Drawable is no longer being used.
The side affect is that we may reset back to
the beginning of the Drawable in some
circumstances.

Cleanup in in memory resources makes it less
likely that frames would be retrieved from
in memory, so this also works toward #207.

7 files changed, 104 insertions, 19 deletions

diff --git a/library/src/androidTest/java/com/bumptech/glide/load/engine/cache/MemorySizeCalculatorTest.java b/library/src/androidTest/java/com/bumptech/glide/load/engine/cache/MemorySizeCalculatorTest.java
index 20b7f123..ed0c7d90 100644
--- a/library/src/androidTest/java/com/bumptech/glide/load/engine/cache/MemorySizeCalculatorTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/load/engine/cache/MemorySizeCalculatorTest.java
@@ -3,6 +3,7 @@ package com.bumptech.glide.load.engine.cache;
 import android.app.ActivityManager;
 import android.content.Context;
 import android.os.Build;
+import com.bumptech.glide.tests.Util;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -30,12 +31,9 @@ public class MemorySizeCalculatorTest {
 
     @After
     public void tearDown() {
-        setSdkVersionInt(initialSdkVersion);
+        Util.setSdkVersionInt(initialSdkVersion);
     }
 
-    private void setSdkVersionInt(int version) {
-        Robolectric.Reflection.setFinalStaticField(Build.VERSION.class, "SDK_INT", version);
-    }
 
     @Test
     public void testDefaultMemoryCacheSizeIsTwiceScreenSize() {
@@ -103,7 +101,7 @@ public class MemorySizeCalculatorTest {
         final int normalMemoryCacheSize = harness.getCalculator().getMemoryCacheSize();
         final int normalBitmapPoolSize = harness.getCalculator().getBitmapPoolSize();
 
-        setSdkVersionInt(10);
+        Util.setSdkVersionInt(10);
 
         final int smallMemoryCacheSize = harness.getCalculator().getMemoryCacheSize();
         final int smallBitmapPoolSize = harness.getCalculator().getBitmapPoolSize();
diff --git a/library/src/androidTest/java/com/bumptech/glide/load/resource/bitmap/BitmapResourceTest.java b/library/src/androidTest/java/com/bumptech/glide/load/resource/bitmap/BitmapResourceTest.java
index 28938404..19eefe83 100644
--- a/library/src/androidTest/java/com/bumptech/glide/load/resource/bitmap/BitmapResourceTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/load/resource/bitmap/BitmapResourceTest.java
@@ -3,11 +3,11 @@ package com.bumptech.glide.load.resource.bitmap;
 import android.graphics.Bitmap;
 import android.os.Build;
 import com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool;
+import com.bumptech.glide.tests.Util;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.robolectric.Robolectric;
 import org.robolectric.RobolectricTestRunner;
 
 import static org.junit.Assert.assertEquals;
@@ -32,7 +32,7 @@ public class BitmapResourceTest {
 
     @After
     public void tearDown() {
-        Robolectric.Reflection.setFinalStaticField(Build.VERSION.class, "SDK_INT", currentBuildVersion);
+        Util.setSdkVersionInt(currentBuildVersion);
     }
 
     @Test
@@ -42,7 +42,7 @@ public class BitmapResourceTest {
 
     @Test
     public void testSizeIsBasedOnDimensPreKitKat() {
-        Robolectric.Reflection.setFinalStaticField(Build.VERSION.class, "SDK_INT", 18);
+        Util.setSdkVersionInt(18);
         assertEquals(harness.bitmap.getWidth() * harness.bitmap.getHeight() * 4, harness.resource.getSize());
     }
 
diff --git a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java
index 3e66d83e..f5db285b 100644
--- a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java
@@ -6,12 +6,15 @@ import android.graphics.Paint;
 import android.graphics.PixelFormat;
 import android.graphics.Rect;
 import android.graphics.drawable.Drawable;
+import android.os.Build;
 import com.bumptech.glide.gifdecoder.GifDecoder;
 import com.bumptech.glide.gifdecoder.GifHeader;
 import com.bumptech.glide.load.Transformation;
 import com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool;
 import com.bumptech.glide.load.resource.drawable.GlideDrawable;
 import com.bumptech.glide.tests.GlideShadowLooper;
+import com.bumptech.glide.tests.Util;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -43,6 +46,7 @@ public class GifDrawableTest {
     private int frameWidth;
     private Bitmap firstFrame;
     private BitmapPool bitmapPool;
+    private int initialSdkVersion;
 
     @Before
     public void setUp() {
@@ -53,6 +57,12 @@ public class GifDrawableTest {
         bitmapPool = mock(BitmapPool.class);
         drawable = new GifDrawable(gifDecoder, frameManager, firstFrame, bitmapPool);
         drawable.setCallback(cb);
+        initialSdkVersion = Build.VERSION.SDK_INT;
+    }
+
+    @After
+    public void tearDown() {
+        Util.setSdkVersionInt(initialSdkVersion);
     }
 
     @Test
@@ -199,7 +209,7 @@ public class GifDrawableTest {
     }
 
     @Test
-    public void testStopsWhenCurrentFrameFinishesIfHasNoCallback() {
+    public void testStopsWhenCurrentFrameFinishesIfHasNoCallbackAndIsAtLeastAtHoneycomb() {
         drawable.setIsRunning(true);
         drawable.setCallback(null);
         drawable.onFrameRead(0);
@@ -208,6 +218,38 @@ public class GifDrawableTest {
     }
 
     @Test
+    public void testDoesNotStopWhenCurrentFrameFinishesIfHasNoCallbackAndIsPreHoneycomb() {
+        Util.setSdkVersionInt(10);
+
+        drawable.setIsRunning(true);
+        drawable.setCallback(null);
+        drawable.onFrameRead(0);
+
+        assertTrue(drawable.isRunning());
+    }
+
+    @Test
+    public void testResetsFrameManagerWhenCurrentFinishesIfHasNoCallbackAndIsAtLeastAtHoneycomb() {
+        drawable.setIsRunning(true);
+        drawable.setCallback(null);
+        drawable.onFrameRead(0);
+
+
+        verify(frameManager).clear();
+    }
+
+    @Test
+    public void testDoesNotResetFrameManagerWhenCurrentFinishesIfHasNoCallbackPreHoneycomb() {
+        Util.setSdkVersionInt(10);
+
+        drawable.setIsRunning(true);
+        drawable.setCallback(null);
+        drawable.onFrameRead(0);
+
+        verify(frameManager, never()).clear();
+    }
+
+    @Test
     public void testSetsIsRunningFalseOnStop() {
         drawable.start();
         drawable.stop();
@@ -225,6 +267,26 @@ public class GifDrawableTest {
     }
 
     @Test
+    public void testDoesNotResetOnStopIfAtLeastAtHoneycomb() {
+        drawable.start();
+        drawable.stop();
+
+        verify(frameManager, never()).clear();
+        // invalidate once from start.
+        verify(cb, times(1)).invalidateDrawable(eq(drawable));
+    }
+
+    @Test
+    public void testDoesResetOnStopIfPreHoneycomb() {
+        Util.setSdkVersionInt(10);
+        drawable.start();
+        drawable.stop();
+
+        verify(frameManager).clear();
+        verify(cb, times(2)).invalidateDrawable(eq(drawable));
+    }
+
+    @Test
     public void testStartsOnSetVisibleTrueIfRunning() {
         drawable.start();
         drawable.setVisible(false, false);
diff --git a/library/src/androidTest/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java b/library/src/androidTest/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java
index a5d4599b..90711322 100644
--- a/library/src/androidTest/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java
@@ -10,6 +10,7 @@ import android.support.v4.app.FragmentActivity;
 import com.bumptech.glide.RequestManager;
 import com.bumptech.glide.tests.BackgroundUtil;
 import com.bumptech.glide.tests.GlideShadowLooper;
+import com.bumptech.glide.tests.Util;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -50,17 +51,13 @@ public class RequestManagerRetrieverTest {
 
     @After
     public void tearDown() {
-        setSdkVersionInt(initialSdkVersion);
+        Util.setSdkVersionInt(initialSdkVersion);
 
         Robolectric.shadowOf(Looper.getMainLooper()).runToEndOfTasks();
         assertThat(retriever.pendingRequestManagerFragments.entrySet(), empty());
         assertThat(retriever.pendingSupportRequestManagerFragments.entrySet(), empty());
     }
 
-    private void setSdkVersionInt(int version) {
-        Robolectric.Reflection.setFinalStaticField(Build.VERSION.class, "SDK_INT", version);
-    }
-
     @Test
     public void testCreatesNewFragmentIfNoneExists() {
         for (RetrieverHarness harness : harnesses) {
@@ -261,7 +258,7 @@ public class RequestManagerRetrieverTest {
 
     @Test
     public void testDoesNotThrowIfAskedToGetManagerForActivityPreHoneycomb() {
-        setSdkVersionInt(Build.VERSION_CODES.GINGERBREAD_MR1);
+        Util.setSdkVersionInt(Build.VERSION_CODES.GINGERBREAD_MR1);
         Activity activity = mock(Activity.class);
         when(activity.getApplicationContext()).thenReturn(Robolectric.application);
         when(activity.getFragmentManager()).thenThrow(new NoSuchMethodError());
@@ -271,7 +268,7 @@ public class RequestManagerRetrieverTest {
 
     @Test
     public void testDoesNotThrowIfAskedToGetManagerForActivityPreJellYBeanMr1() {
-        setSdkVersionInt(Build.VERSION_CODES.JELLY_BEAN);
+        Util.setSdkVersionInt(Build.VERSION_CODES.JELLY_BEAN);
         Activity activity = Robolectric.buildActivity(Activity.class).create().start().resume().get();
         Activity spyActivity = Mockito.spy(activity);
         when(spyActivity.isDestroyed()).thenThrow(new NoSuchMethodError());
@@ -281,7 +278,7 @@ public class RequestManagerRetrieverTest {
 
     @Test
     public void testDoesNotThrowIfAskedToGetManagerForFragmentPreHoneyCombMr2() {
-        setSdkVersionInt(Build.VERSION_CODES.HONEYCOMB_MR1);
+        Util.setSdkVersionInt(Build.VERSION_CODES.HONEYCOMB_MR1);
         Activity activity = Robolectric.buildActivity(Activity.class).create().start().resume().get();
         android.app.Fragment fragment = new android.app.Fragment();
 
@@ -296,7 +293,7 @@ public class RequestManagerRetrieverTest {
 
     @Test
     public void testDoesNotThrowIfAskedToGetManagerForFragmentPreJellyBeanMr1() {
-        setSdkVersionInt(Build.VERSION_CODES.JELLY_BEAN);
+        Util.setSdkVersionInt(Build.VERSION_CODES.JELLY_BEAN);
         Activity activity = Robolectric.buildActivity(Activity.class).create().start().resume().get();
         android.app.Fragment fragment = new android.app.Fragment();
 
diff --git a/library/src/androidTest/java/com/bumptech/glide/tests/Util.java b/library/src/androidTest/java/com/bumptech/glide/tests/Util.java
index b69ac7f4..73220f45 100644
--- a/library/src/androidTest/java/com/bumptech/glide/tests/Util.java
+++ b/library/src/androidTest/java/com/bumptech/glide/tests/Util.java
@@ -1,7 +1,9 @@
 package com.bumptech.glide.tests;
 
+import android.os.Build;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
+import org.robolectric.Robolectric;
 
 import java.io.File;
 import java.io.FileInputStream;
@@ -70,4 +72,9 @@ public class Util {
             }
         };
     }
+
+    public static void setSdkVersionInt(int version) {
+        Robolectric.Reflection.setFinalStaticField(Build.VERSION.class, "SDK_INT", version);
+    }
+
 }
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java
index acf1058d..fd5ea782 100644
--- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java
+++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java
@@ -133,6 +133,22 @@ public class GifDrawable extends GlideDrawable implements GifFrameManager.FrameC
     public void stop() {
         isStarted = false;
         stopRunning();
+
+        // On APIs > honeycomb we know our drawable is not being displayed anymore when it's callback is cleared and so
+        // we can use the absence of a callback as an indication that it's ok to clear our temporary data. Prior to
+        // honeycomb we can't tell if our callback is null and instead eagerly reset to avoid holding on to resources we
+        // no longer need.
+        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB) {
+            reset();
+        }
+    }
+
+    /**
+     * Clears temporary data and resets the drawable back to the first frame.
+     */
+    private void reset() {
+        frameManager.clear();
+        invalidateSelf();
     }
 
     private void startRunning() {
@@ -222,8 +238,9 @@ public class GifDrawable extends GlideDrawable implements GifFrameManager.FrameC
     @TargetApi(Build.VERSION_CODES.HONEYCOMB)
     @Override
     public void onFrameRead(int frameIndex) {
-        if (Build.VERSION_CODES.HONEYCOMB <= Build.VERSION.SDK_INT && getCallback() == null) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB && getCallback() == null) {
             stop();
+            reset();
             return;
         }
         if (!isRunning) {
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java
index 42eea5d0..661e84d9 100644
--- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java
+++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java
@@ -100,11 +100,15 @@ class GifFrameManager {
         if (current != null) {
             mainHandler.removeCallbacks(current);
             Glide.clear(current);
+            current = null;
         }
         if (next != null) {
             mainHandler.removeCallbacks(next);
             Glide.clear(next);
+            next = null;
         }
+
+        decoder.resetFrameIndex();
     }
 
     class DelayTarget extends SimpleTarget<Bitmap> implements Runnable {