Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions library/src/main/java/com/bumptech/glide/Glide.java
Original file line number Diff line number Diff line change
Expand Up @@ -663,10 +663,10 @@ public Registry getRegistry() {
return glideContext.getRegistry();
}

boolean removeFromManagers(@NonNull Target<?> target) {
boolean removeFromManagers(@NonNull Target<?> target, boolean skipPlaceholder) {
synchronized (managers) {
for (RequestManager requestManager : managers) {
if (requestManager.untrack(target)) {
if (requestManager.untrack(target, skipPlaceholder)) {
return true;
}
}
Expand Down
38 changes: 24 additions & 14 deletions library/src/main/java/com/bumptech/glide/RequestManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public synchronized void onStart() {
public synchronized void onStop() {
targetTracker.onStop();
if (clearOnStop) {
clearRequests();
clearRequests(true);
} else {
pauseRequests();
}
Expand All @@ -387,8 +387,8 @@ public synchronized void onStop() {
@Override
public synchronized void onDestroy() {
targetTracker.onDestroy();
clearRequests();
requestTracker.clearRequests();
clearRequests(true);
requestTracker.clearRequests(true);
lifecycle.removeListener(this);
lifecycle.removeListener(connectivityMonitor);
Util.removeCallbacksOnUiThread(addSelfToLifecycle);
Expand Down Expand Up @@ -621,10 +621,10 @@ public <ResourceType> RequestBuilder<ResourceType> as(
* @param view The view to cancel loads and free resources for.
* @throws IllegalArgumentException if an object other than Glide's metadata is put as the view's
* tag.
* @see #clear(Target)
* @see #clear(Target, boolean)
*/
public void clear(@NonNull View view) {
clear(new ClearTarget(view));
clear(new ClearTarget(view), false);
}

/**
Expand All @@ -634,15 +634,25 @@ public void clear(@NonNull View view) {
* @param target The Target to cancel loads for.
*/
public void clear(@Nullable final Target<?> target) {
clear(target, false);
}

/**
* Cancel any pending loads Glide may have for the target and free any resources (such as {@link
* Bitmap}s) that may have been loaded for the target so they may be reused.
*
* @param target The Target to cancel loads for.
*/
public void clear(@Nullable final Target<?> target, boolean skipPlaceholder) {
if (target == null) {
return;
}

untrackOrDelegate(target);
untrackOrDelegate(target, skipPlaceholder);
}

private void untrackOrDelegate(@NonNull Target<?> target) {
boolean isOwnedByUs = untrack(target);
private void untrackOrDelegate(@NonNull Target<?> target, boolean skipPlaceholder) {
boolean isOwnedByUs = untrack(target, skipPlaceholder);
// 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
Expand All @@ -661,20 +671,20 @@ private void untrackOrDelegate(@NonNull Target<?> target) {
// 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.
Request request = target.getRequest();
if (!isOwnedByUs && !glide.removeFromManagers(target) && request != null) {
if (!isOwnedByUs && !glide.removeFromManagers(target, skipPlaceholder) && request != null) {
target.setRequest(null);
request.clear();
request.clear(skipPlaceholder);
}
}

synchronized boolean untrack(@NonNull Target<?> target) {
synchronized boolean untrack(@NonNull Target<?> target, boolean skipPlaceholder) {
Request request = target.getRequest();
// If the Target doesn't have a request, it's already been cleared.
if (request == null) {
return true;
}

if (requestTracker.clearAndRemove(request)) {
if (requestTracker.clearAndRemove(request, skipPlaceholder)) {
targetTracker.untrack(target);
target.setRequest(null);
return true;
Expand Down Expand Up @@ -718,9 +728,9 @@ public void onLowMemory() {
// Nothing to add conditionally. See Glide#onTrimMemory for unconditional behavior.
}

private synchronized void clearRequests() {
private synchronized void clearRequests(boolean skipPlaceholder) {
for (Target<?> target : targetTracker.getAll()) {
clear(target);
clear(target, skipPlaceholder);
}
targetTracker.clear();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ void addRequest(Request request) {
* request was removed or invalid or {@code false} if the request was not found.
*/
public boolean clearAndRemove(@Nullable Request request) {
return clearAndRemove(request, false);
}

/**
* Stops tracking the given request, clears, and recycles it, and returns {@code true} if the
* request was removed or invalid or {@code false} if the request was not found.
*
* @param skipPlaceholder does not load the placeholder if a resource id is provided
*/
public boolean clearAndRemove(@Nullable Request request, boolean skipPlaceholder) {
if (request == null) {
// If the Request is null, the request is already cleared and we don't need to search further
// for its owner.
Expand All @@ -67,7 +77,7 @@ public boolean clearAndRemove(@Nullable Request request) {
// Avoid short circuiting.
isOwnedByUs = pendingRequests.remove(request) || isOwnedByUs;
if (isOwnedByUs) {
request.clear();
request.clear(skipPlaceholder);
}
return isOwnedByUs;
}
Expand Down Expand Up @@ -124,10 +134,21 @@ public void resumeRequests() {
* <p>After this call requests cannot be restarted.
*/
public void clearRequests() {
clearRequests(false);
}

/**
* Cancels all requests and clears their resources.
*
* <p>After this call requests cannot be restarted.
*
* @param skipPlaceholder does not load the placeholder if a resource id is provided
*/
public void clearRequests(boolean skipPlaceholder) {
for (Request request : Util.getSnapshot(requests)) {
// It's unsafe to recycle the Request here because we don't know who might else have a
// reference to it.
clearAndRemove(request);
clearAndRemove(request, skipPlaceholder);
}
pendingRequests.clear();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,15 @@ public void begin() {
}

@Override
public void clear() {
public void clear(boolean skipPlaceholder) {
synchronized (requestLock) {
primaryState = RequestState.CLEARED;
primary.clear();
primary.clear(skipPlaceholder);
// Don't check primary's failed state here because it will have been reset by the clear call
// immediately before this.
if (errorState != RequestState.CLEARED) {
errorState = RequestState.CLEARED;
error.clear();
error.clear(skipPlaceholder);
}
}
}
Expand Down
15 changes: 14 additions & 1 deletion library/src/main/java/com/bumptech/glide/request/Request.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,21 @@ public interface Request {
* Prevents any bitmaps being loaded from previous requests, releases any resources held by this
* request, displays the current placeholder if one was provided, and marks the request as having
* been cancelled.
*
* <p>The default implementation delegates to {@link #clear(boolean)}.
*/
default void clear() {
clear(false);
}

/**
* Prevents any bitmaps being loaded from previous requests, releases any resources held by this
* request, displays the current placeholder if one was provided, and marks the request as having
* been cancelled.
*
* @param skipPlaceholder does not load the placeholder if a resource id is provided
*/
void clear();
void clear(boolean skipPlaceholder);

/**
* Similar to {@link #clear} for in progress requests (or portions of a request), but does nothing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ private void assertNotCallingCallbacks() {
* @see #cancel()
*/
@Override
public void clear() {
public void clear(boolean skipPlaceholder) {
Resource<R> toRelease = null;
synchronized (requestLock) {
assertNotCallingCallbacks();
Expand All @@ -335,7 +335,7 @@ public void clear() {
resource = null;
}
if (canNotifyCleared()) {
target.onLoadCleared(getPlaceholderDrawable());
target.onLoadCleared(skipPlaceholder ? null : getPlaceholderDrawable());
}

GlideTrace.endSectionAsync(TAG, cookie);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,13 @@ public void begin() {
}

@Override
public void clear() {
public void clear(boolean skipPlaceholder) {
synchronized (requestLock) {
isRunningDuringBegin = false;
fullState = RequestState.CLEARED;
thumbState = RequestState.CLEARED;
thumb.clear();
full.clear();
thumb.clear(skipPlaceholder);
full.clear(skipPlaceholder);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static com.bumptech.glide.RobolectricConstants.ROBOLECTRIC_SDK;
import static com.bumptech.glide.tests.BackgroundUtil.testInBackground;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.eq;
Expand Down Expand Up @@ -129,7 +130,7 @@ public void testResumesRequestsOnStart() {
public void testClearsRequestsOnDestroy() {
manager.onDestroy();

verify(requestTracker).clearRequests();
verify(requestTracker).clearRequests(true);
}

@Test
Expand Down Expand Up @@ -263,6 +264,103 @@ public Set<RequestManager> getDescendants() {
parent.clear(target);
}

/**
* Reproduces the ANR reported in https://github.com/bumptech/glide/issues/5398.
*
* <p>When RequestManager.onDestroy() is called, it iterates all tracked targets and calls
* clear() on each. For requests configured with a placeholder resource ID (not a pre-loaded
* Drawable), SingleRequest.clear() triggers synchronous drawable resource decoding via
* getPlaceholderDrawable() -> loadDrawable() -> DrawableDecoderCompat.getDrawable(). On a real
* device, this involves native image decoding (ImageDecoder.nDecodeBitmap) on the main thread.
* With multiple requests, the cumulative cost causes ANRs.
*/
@Test
public void onDestroy_withPlaceholderResourceId_loadsDrawableSynchronouslyDuringClear() {
final Drawable[] clearedPlaceholder = {null};
CustomTarget<Drawable> targetWithPlaceholderId =
new CustomTarget<>() {
@Override
public void onResourceReady(
@NonNull Drawable resource, @Nullable Transition<? super Drawable> transition) {}

@Override
public void onLoadCleared(@Nullable Drawable placeholder) {
clearedPlaceholder[0] = placeholder;
}
};

RequestManager managerWithRealTracker =
new RequestManager(
Glide.get(context),
lifecycle,
Collections::emptySet,
context);

// Load with a placeholder RESOURCE ID (not a Drawable object).
// This is the key condition: the placeholder drawable is lazily loaded during clear().
managerWithRealTracker
.load(new File("fake"))
.placeholder(android.R.drawable.ic_delete)
.into(targetWithPlaceholderId);

// Simulate Activity destruction. This calls clearRequests() which calls clear() on each
// tracked target, triggering synchronous drawable loading via DrawableDecoderCompat.
managerWithRealTracker.onDestroy();

// During onDestroy(), there is no need to load a placeholder drawable since the Activity is
// being destroyed. Loading it synchronously causes ANRs (see #5398). The fix should skip
// placeholder loading during destruction, so onLoadCleared() should receive null.
assertThat(clearedPlaceholder[0]).isNull();
}

/**
* Simulates the ANR multiplication effect: RequestManager.onDestroy() with N requests that each
* have a placeholder resource ID results in N synchronous drawable decode operations on the main
* thread.
*/
@Test
public void onDestroy_multipleRequestsWithPlaceholderResourceId_allLoadDrawablesSynchronously() {
int numRequests = 10;
final Drawable[][] clearedPlaceholders = new Drawable[numRequests][1];

RequestManager managerWithRealTracker =
new RequestManager(
Glide.get(context),
lifecycle,
Collections::emptySet,
context);

for (int i = 0; i < numRequests; i++) {
final int index = i;
CustomTarget<Drawable> t =
new CustomTarget<>() {
@Override
public void onResourceReady(
@NonNull Drawable resource, @Nullable Transition<? super Drawable> transition) {}

@Override
public void onLoadCleared(@Nullable Drawable placeholder) {
clearedPlaceholders[index][0] = placeholder;
}
};

managerWithRealTracker
.load(new File("fake"))
.placeholder(android.R.drawable.ic_delete)
.into(t);
}

// onDestroy iterates all N targets, calling clear() on each.
// Each clear() synchronously loads the placeholder drawable.
managerWithRealTracker.onDestroy();

// During onDestroy(), placeholder drawables should NOT be loaded (causes ANRs, see #5398).
// The fix should skip placeholder loading during destruction.
for (int i = 0; i < numRequests; i++) {
assertThat(clearedPlaceholders[i][0]).isNull();
}
}

@Test
public void clear_withRequestStartedInParentManager_doesNotThrow() {
final RequestManager child =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ public void begin() {
}

@Override
public void clear() {
public void clear(boolean skipPlaceholder) {
if (isCleared) {
throw new IllegalStateException();
}
Expand Down
Loading
Loading