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) {