Skip to content

Commit

Permalink
Register RequestManagers before adding Lifecycle listeners.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sjudd authored and glide-copybara-robot committed Jan 6, 2023
1 parent 0f75576 commit 4affb8d
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
6 changes: 4 additions & 2 deletions library/src/main/java/com/bumptech/glide/RequestManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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) {
Expand Down

0 comments on commit 4affb8d

Please sign in to comment.