Register RequestManagers before adding Lifecycle listeners.

RequestManagers are registered to a singleton. If they're not unregistered, we'll leak the RequestManager and the corresponding Activity or Fragment.

Unregistration is done via the destroyed Lifecycle event. While we assert that the Activity is not destroyed at a higher level, that assertion will not trigger during onDestroy calls on API 29+.

As a result on API 29+, if we add a listener and then register the RequestManager statically, we'll end up triggering an assertion because the RequestManager will be unregistered before it is registered. Pre API 29 we'd have crashed earlier due to the Activity having been destroyed.

To fix this on API 29+, we'll move the registration earlier so that we always register before there's a chance that the Lifecycle will trigger unregistration.

This behavior changed a bit when we swapped over to using Android's Lifecycle. However I think the old behavior would simply have registered the RequestManager and never unregistered it, leading to a memory leak.

PiperOrigin-RevId: 500018277

Author: sjudd

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

@@ -2,7 +2,10 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
 
+import android.os.Build;
 import android.os.Bundle;
 import android.view.LayoutInflater;
 import android.view.View;
@@ -13,14 +16,19 @@
 import androidx.fragment.app.FragmentActivity;
 import androidx.fragment.app.FragmentManager;
 import androidx.fragment.app.testing.FragmentScenario;
+import androidx.lifecycle.Lifecycle.Event;
 import androidx.lifecycle.Lifecycle.State;
+import androidx.lifecycle.LifecycleObserver;
+import androidx.lifecycle.LifecycleOwner;
+import androidx.lifecycle.OnLifecycleEvent;
 import androidx.test.core.app.ActivityScenario;
 import androidx.test.core.app.ActivityScenario.ActivityAction;
 import androidx.test.ext.junit.rules.ActivityScenarioRule;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import com.bumptech.glide.instrumentation.R;
 import com.bumptech.glide.test.DefaultFragmentActivity;
 import com.bumptech.glide.testutil.TearDownGlide;
+import java.util.concurrent.atomic.AtomicReference;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -59,6 +67,70 @@ public void get_withActivityBeforeCreate_startsRequestManager() {
     scenario.onActivity(activity -> assertThat(Glide.with(activity).isPaused()).isFalse());
   }
 
+  // See b/262668610
+  @SuppressWarnings("OnLifecycleEvent")
+  @Test
+  public void get_withActivityOnDestroy_QPlus_doesNotCrash() {
+    // Activity#isDestroyed's behavior seems to have changed in Q. On Q+, isDestroyed returns false
+    // during onDestroy, so we have to handle that case explicitly.
+    assumeTrue(Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q);
+    scenario.moveToState(State.CREATED);
+
+    class GetOnDestroy implements LifecycleObserver {
+      private final FragmentActivity activity;
+
+      GetOnDestroy(FragmentActivity activity) {
+        this.activity = activity;
+      }
+
+      @OnLifecycleEvent(Event.ON_DESTROY)
+      public void onDestroy(@NonNull LifecycleOwner owner) {
+        Glide.with(activity);
+      }
+    }
+    scenario.onActivity(
+        activity -> activity.getLifecycle().addObserver(new GetOnDestroy(activity)));
+    scenario.moveToState(State.DESTROYED);
+  }
+
+  @SuppressWarnings("OnLifecycleEvent")
+  @Test
+  public void get_withActivityOnDestroy_afterJellyBeanAndbeforeQ_doesNotCrash() {
+    // Activity#isDestroyed's behavior seems to have changed in Q. On <Q, isDestroyed returns true
+    // during onDestroy, triggering an assertion in Glide. < Jelly bean, isDestroyed is not
+    // available as a method.
+    assumeTrue(
+        Build.VERSION.SDK_INT > Build.VERSION_CODES.JELLY_BEAN
+            && Build.VERSION.SDK_INT < Build.VERSION_CODES.Q);
+    AtomicReference<Exception> thrownException = new AtomicReference<>();
+    scenario.moveToState(State.CREATED);
+
+    class GetOnDestroy implements LifecycleObserver {
+      private final FragmentActivity activity;
+
+      GetOnDestroy(FragmentActivity activity) {
+        this.activity = activity;
+      }
+
+      @OnLifecycleEvent(Event.ON_DESTROY)
+      public void onDestroy(@NonNull LifecycleOwner owner) {
+        try {
+          Glide.with(activity);
+          fail("Failed to throw expected exception");
+        } catch (Exception e) {
+          thrownException.set(e);
+        }
+      }
+    }
+    scenario.onActivity(
+        activity -> activity.getLifecycle().addObserver(new GetOnDestroy(activity)));
+    scenario.moveToState(State.DESTROYED);
+
+    assertThat(thrownException.get())
+        .hasMessageThat()
+        .contains("You cannot start a load for a destroyed activity");
+  }
+
   @Test
   public void get_withFragment_beforeFragmentIsAdded_throws() {
     Fragment fragment = new Fragment();

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

@@ -129,6 +129,10 @@ public RequestManager(
             context.getApplicationContext(),
             new RequestManagerConnectivityListener(requestTracker));
 
+    // Order matters, this might be unregistered by teh listeners below, so we need to be sure to
+    // register first to prevent both assertions and memory leaks.
+    glide.registerRequestManager(this);
+
     // If we're the application level request manager, we may be created on a background thread.
     // In that case we cannot risk synchronously pausing or resuming requests, so we hack around the
     // issue by delaying adding ourselves as a lifecycle listener by posting to the main thread.
@@ -143,8 +147,6 @@ public RequestManager(
     defaultRequestListeners =
         new CopyOnWriteArrayList<>(glide.getGlideContext().getDefaultRequestListeners());
     setRequestOptions(glide.getGlideContext().getDefaultRequestOptions());
-
-    glide.registerRequestManager(this);
   }
 
   protected synchronized void setRequestOptions(@NonNull RequestOptions toSet) {