Fix exception clearing after onDestroy in a second RequestManager.

Sequence is either:
1. Obtain a RequestManager for a Fragment
2. Wait for Fragment to be destroyed
3. Start a new load with the RequestManager for the destroyed Fragment
4. Clear the new load with the application RequestManager (or any
RequestManager other than the one the load was started with).

Or:
1. Obtain a RequestManager for a Fragment
2. Obtain a RequestBuilder from the Fragment RequestManager
3. Pass the RequestBuilder to a background thread
4. Wait for the Fragment to be destroyed
5. Start a load with the RequestBuilder on the background thread
6. Clear the target returned from submit using the application manager.

instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerTest.java

@@ -0,0 +1,95 @@
+package com.bumptech.glide;
+
+import android.content.Context;
+import android.graphics.drawable.Drawable;
+import android.support.test.InstrumentationRegistry;
+import android.support.test.runner.AndroidJUnit4;
+import android.widget.ImageView;
+import com.bumptech.glide.manager.Lifecycle;
+import com.bumptech.glide.manager.LifecycleListener;
+import com.bumptech.glide.manager.RequestManagerTreeNode;
+import com.bumptech.glide.request.FutureTarget;
+import com.bumptech.glide.test.ConcurrencyHelper;
+import com.bumptech.glide.test.ResourceIds;
+import com.bumptech.glide.test.ResourceIds.raw;
+import com.bumptech.glide.test.TearDownGlide;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+@RunWith(AndroidJUnit4.class)
+public class RequestManagerTest {
+  @Rule public TearDownGlide tearDownGlide = new TearDownGlide();
+  @Mock private RequestManagerTreeNode treeNode;
+
+  private ConcurrencyHelper concurrency = new ConcurrencyHelper();
+  private RequestManager requestManager;
+  private Context context;
+
+  @Before
+  public void setUp() {
+    MockitoAnnotations.initMocks(this);
+    context = InstrumentationRegistry.getTargetContext();
+    Glide glide = Glide.get(context);
+    requestManager = new RequestManager(glide, new Lifecycle() {
+      @Override
+      public void addListener(LifecycleListener listener) {
+        listener.onStart();
+      }
+
+      @Override
+      public void removeListener(LifecycleListener listener) {
+        // Do nothing.
+      }
+    }, treeNode, context);
+  }
+
+  /**
+   * Tests #2262.
+   */
+  @Test
+  public void clear_withNonOwningRequestManager_afterOwningManagerIsDestroyed_doesNotThrow() {
+    // First destroy our Fragment/Activity RequestManager.
+    requestManager.onDestroy();
+
+    final ImageView imageView = new ImageView(context);
+    imageView.measure(100, 100);
+    imageView.layout(0, 0, 100, 100);
+    // Then start a new load with our now destroyed RequestManager.
+    concurrency.loadOnMainThread(requestManager.load(ResourceIds.raw.canonical), imageView);
+
+    // Finally clear our new load with any RequestManager other than the one we used to start it.
+    concurrency.runOnMainThread(new Runnable() {
+      @Override
+      public void run() {
+        Glide.with(context).clear(imageView);
+      }
+    });
+  }
+
+  /**
+   * Tests b/69361054.
+   */
+  @Test
+  public void clear_withNonOwningRequestManager_onBackgroundTHread_doesNotThrow() {
+    concurrency.runOnMainThread(new Runnable() {
+      @Override
+      public void run() {
+        requestManager.onDestroy();
+      }
+    });
+
+    final FutureTarget<Drawable> target =
+        concurrency.wait(requestManager.load(raw.canonical).submit());
+
+    concurrency.runOnMainThread(new Runnable() {
+      @Override
+      public void run() {
+        Glide.with(context).clear(target);
+      }
+    });
+  }
+}

instrumentation/src/androidTest/java/com/bumptech/glide/test/ConcurrencyHelper.java

@@ -1,8 +1,17 @@
 package com.bumptech.glide.test;
 
 
+import android.graphics.drawable.Drawable;
 import android.os.Handler;
 import android.os.Looper;
+import android.support.annotation.Nullable;
+import android.widget.ImageView;
+import com.bumptech.glide.RequestBuilder;
+import com.bumptech.glide.request.Request;
+import com.bumptech.glide.request.target.DrawableImageViewTarget;
+import com.bumptech.glide.request.target.SizeReadyCallback;
+import com.bumptech.glide.request.target.Target;
+import com.bumptech.glide.request.transition.Transition;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
@@ -27,6 +36,87 @@ public <T> T get(Future<T> future) {
     }
   }
 
+  public <T, Y extends Future<T>> Y wait(Y  future) {
+    get(future);
+    return future;
+  }
+
+  public Target<Drawable> loadOnMainThread(
+      final RequestBuilder<Drawable> builder, ImageView imageView) {
+    return loadOnMainThread(builder, new DrawableImageViewTarget(imageView));
+  }
+
+  public <T> Target<T> loadOnMainThread(final RequestBuilder<T> builder, final Target<T> target) {
+    return callOnMainThread(new Callable<Target<T>>() {
+      @Override
+      public Target<T> call() throws Exception {
+        final CountDownLatch latch = new CountDownLatch(1);
+        builder.into(new Target<T>() {
+          @Override
+          public void onStart() {
+            target.onStart();
+          }
+
+          @Override
+          public void onStop() {
+            target.onStop();
+          }
+
+          @Override
+          public void onDestroy() {
+            target.onDestroy();
+          }
+
+          @Override
+          public void onResourceReady(T resource, Transition<? super T> transition) {
+            target.onResourceReady(resource, transition);
+            latch.countDown();
+          }
+
+          @Override
+          public void onLoadCleared(@Nullable Drawable placeholder) {
+            target.onLoadCleared(placeholder);
+          }
+
+          @Override
+          public void onLoadStarted(@Nullable Drawable placeholder) {
+            target.onLoadStarted(placeholder);
+          }
+
+          @Override
+          public void onLoadFailed(@Nullable Drawable errorDrawable) {
+            target.onLoadFailed(errorDrawable);
+            latch.countDown();
+          }
+
+          @Override
+          public void getSize(SizeReadyCallback cb) {
+            target.getSize(cb);
+          }
+
+          @Override
+          public void removeCallback(SizeReadyCallback cb) {
+            target.removeCallback(cb);
+          }
+
+          @Override
+          public void setRequest(@Nullable Request request) {
+            target.setRequest(request);
+
+          }
+
+          @Nullable
+          @Override
+          public Request getRequest() {
+            return target.getRequest();
+          }
+        });
+        latch.await(TIMEOUT_MS, TIMEOUT_UNIT);
+        return target;
+      }
+    });
+  }
+
   public void runOnMainThread(final Runnable runnable) {
     callOnMainThread(new Callable<Void>() {
       @Override

library/src/main/java/com/bumptech/glide/Glide.java

@@ -705,15 +705,16 @@ public Registry getRegistry() {
     return registry;
   }
 
-  void removeFromManagers(Target<?> target) {
+  boolean removeFromManagers(Target<?> target) {
     synchronized (managers) {
       for (RequestManager requestManager : managers) {
         if (requestManager.untrack(target)) {
-          return;
+          return true;
         }
       }
     }
-    throw new IllegalStateException("Failed to remove target from managers");
+
+    return false;
   }
 
   void registerRequestManager(RequestManager requestManager) {

library/src/main/java/com/bumptech/glide/RequestManager.java

@@ -447,8 +447,27 @@ public void run() {
 
   private void untrackOrDelegate(Target<?> target) {
     boolean isOwnedByUs = untrack(target);
-    if (!isOwnedByUs) {
-      glide.removeFromManagers(target);
+    // We'll end up here if the Target was cleared after the RequestManager that started the request
+    // is destroyed. That can happen for at least two reasons:
+    // 1. We call clear() on a background thread using something other than Application Context
+    // RequestManager.
+    // 2. The caller retains a reference to the RequestManager after the corresponding Activity or
+    // Fragment is destroyed, starts a load with it, and then clears that load with a different
+    // RequestManager. Callers seem especially likely to do this in retained Fragments (#2262).
+    //
+    // #1 is always an error. At best the caller is leaking memory briefly in something like an
+    // AsyncTask. At worst the caller is leaking an Activity or Fragment for a sustained period of
+    // time if they do something like reference the Activity RequestManager in a long lived
+    // background thread or task.
+    //
+    // #2 is always an error. Callers shouldn't be starting new loads using RequestManagers after
+    // the corresponding Activity or Fragment is destroyed because retaining any reference to the
+    // RequestManager leaks memory. It's possible that there's some brief period of time during or
+    // immediately after onDestroy where this is reasonable, but I can't think of why.
+    if (!isOwnedByUs && !glide.removeFromManagers(target) && target.getRequest() != null) {
+      Request request = target.getRequest();
+      target.setRequest(null);
+      request.clear();
     }
   }
 

library/src/main/java/com/bumptech/glide/manager/RequestTracker.java

@@ -53,16 +53,25 @@ void addRequest(Request request) {
    * request was removed or invalid or {@code false} if the request was not found.
    */
   public boolean clearRemoveAndRecycle(@Nullable Request request) {
-    if (request == null) {
-      // Nothing to do for null requests.
+    // It's safe for us to recycle because this is only called when the user is explicitly clearing
+    // a Target so we know that there are no remaining references to the Request.
+    return clearRemoveAndMaybeRecycle(request, /*isSafeToRecycle=*/ true);
+  }
+
+  private boolean clearRemoveAndMaybeRecycle(@Nullable Request request, boolean isSafeToRecycle) {
+     if (request == null) {
+       // If the Request is null, the request is already cleared and we don't need to search further
+       // for its owner.
       return true;
     }
     boolean isOwnedByUs = requests.remove(request);
     // Avoid short circuiting.
     isOwnedByUs = pendingRequests.remove(request) || isOwnedByUs;
     if (isOwnedByUs) {
       request.clear();
-      request.recycle();
+      if (isSafeToRecycle) {
+        request.recycle();
+      }
     }
     return isOwnedByUs;
   }
@@ -107,7 +116,9 @@ public void resumeRequests() {
    */
   public void clearRequests() {
     for (Request request : Util.getSnapshot(requests)) {
-      clearRemoveAndRecycle(request);
+      // It's unsafe to recycle the Request here because we don't know who might else have a
+      // reference to it.
+      clearRemoveAndMaybeRecycle(request, /*isSafeToRecycle=*/ false);
     }
     pendingRequests.clear();
   }

library/src/main/java/com/bumptech/glide/request/SingleRequest.java

@@ -307,6 +307,7 @@ private void assertNotCallingCallbacks() {
   public void clear() {
     Util.assertMainThread();
     assertNotCallingCallbacks();
+    stateVerifier.throwIfRecycled();
     if (status == Status.CLEARED) {
       return;
     }
@@ -459,6 +460,13 @@ public void onSizeReady(int width, int height) {
         requestOptions.getUseAnimationPool(),
         requestOptions.getOnlyRetrieveFromCache(),
         this);
+
+    // This is a hack that's only useful for testing right now where loads complete synchronously
+    // even though under any executor running on any thread but the main thread, the load would
+    // have completed asynchronously.
+    if (status != Status.RUNNING) {
+      loadStatus = null;
+    }
     if (Log.isLoggable(TAG, Log.VERBOSE)) {
       logV("finished onSizeReady in " + LogTime.getElapsedMillis(startTime));
     }

library/src/test/java/com/bumptech/glide/GlideTest.java

@@ -51,6 +51,7 @@
 import com.bumptech.glide.request.Request;
 import com.bumptech.glide.request.RequestListener;
 import com.bumptech.glide.request.RequestOptions;
+import com.bumptech.glide.request.target.SimpleTarget;
 import com.bumptech.glide.request.target.SizeReadyCallback;
 import com.bumptech.glide.request.target.Target;
 import com.bumptech.glide.request.transition.Transition;
@@ -679,6 +680,23 @@ public void testByteData() {
     requestManager.load(data).into(target);
   }
 
+  @Test
+  public void removeFromManagers_afterRequestManagerRemoved_clearsRequest() {
+    target = requestManager.load(mockUri("content://uri")).into(new SimpleTarget<Drawable>() {
+      @Override
+      public void onResourceReady(Drawable resource, Transition<? super Drawable> transition) {
+        // Do nothing.
+      }
+    });
+    Request request = target.getRequest();
+
+    requestManager.onDestroy();
+    requestManager.clear(target);
+
+    assertThat(target.getRequest()).isNull();
+    assertThat(request.isCancelled()).isTrue();
+  }
+
   @Test
   public void testClone() throws IOException {
     Target<Drawable> firstTarget = mock(Target.class);

library/src/test/java/com/bumptech/glide/RequestManagerTest.java

@@ -28,6 +28,7 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -93,6 +94,11 @@ public void removeCallback(SizeReadyCallback cb) {
             context);
   }
 
+  @After
+  public void tearDown() {
+    Glide.tearDown();
+  }
+
   @Test
   public void testPauseRequestsPausesRequests() {
     manager.pauseRequests();