From 4affb8d2d9f4ca15c7953ca7322c52fb1e2dab1b Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Thu, 5 Jan 2023 15:05:27 -0800 Subject: [PATCH] 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 --- .../glide/RequestManagerLifecycleTest.java | 72 +++++++++++++++++++ .../com/bumptech/glide/RequestManager.java | 6 +- 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java index 1fd227fb86..58c3b2fe94 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/RequestManagerLifecycleTest.java +++ b/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,7 +16,11 @@ 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; @@ -21,6 +28,7 @@ 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 Build.VERSION_CODES.JELLY_BEAN + && Build.VERSION.SDK_INT < Build.VERSION_CODES.Q); + AtomicReference 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(); diff --git a/library/src/main/java/com/bumptech/glide/RequestManager.java b/library/src/main/java/com/bumptech/glide/RequestManager.java index a4d2319788..ba9b33ef11 100644 --- a/library/src/main/java/com/bumptech/glide/RequestManager.java +++ b/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) {