diff --git a/library/src/main/java/com/bumptech/glide/Glide.java b/library/src/main/java/com/bumptech/glide/Glide.java index 5c75c4f1d8..9b9f3fb59a 100644 --- a/library/src/main/java/com/bumptech/glide/Glide.java +++ b/library/src/main/java/com/bumptech/glide/Glide.java @@ -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; } } diff --git a/library/src/main/java/com/bumptech/glide/RequestManager.java b/library/src/main/java/com/bumptech/glide/RequestManager.java index d6bade3cbb..9f7eaf6b42 100644 --- a/library/src/main/java/com/bumptech/glide/RequestManager.java +++ b/library/src/main/java/com/bumptech/glide/RequestManager.java @@ -374,7 +374,7 @@ public synchronized void onStart() { public synchronized void onStop() { targetTracker.onStop(); if (clearOnStop) { - clearRequests(); + clearRequests(true); } else { pauseRequests(); } @@ -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); @@ -621,10 +621,10 @@ public RequestBuilder 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); } /** @@ -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 @@ -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; @@ -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(); } diff --git a/library/src/main/java/com/bumptech/glide/manager/RequestTracker.java b/library/src/main/java/com/bumptech/glide/manager/RequestTracker.java index 7f51edfb49..eca4b2a8b2 100644 --- a/library/src/main/java/com/bumptech/glide/manager/RequestTracker.java +++ b/library/src/main/java/com/bumptech/glide/manager/RequestTracker.java @@ -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. @@ -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; } @@ -124,10 +134,21 @@ public void resumeRequests() { *

After this call requests cannot be restarted. */ public void clearRequests() { + clearRequests(false); + } + + /** + * Cancels all requests and clears their resources. + * + *

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(); } diff --git a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java index 851acf2cdf..706b3f9380 100644 --- a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java +++ b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java @@ -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); } } } diff --git a/library/src/main/java/com/bumptech/glide/request/Request.java b/library/src/main/java/com/bumptech/glide/request/Request.java index 622ad6c38f..9837d65e1d 100644 --- a/library/src/main/java/com/bumptech/glide/request/Request.java +++ b/library/src/main/java/com/bumptech/glide/request/Request.java @@ -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. + * + *

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 diff --git a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java index bcd48161c1..96f44de787 100644 --- a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java +++ b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java @@ -320,7 +320,7 @@ private void assertNotCallingCallbacks() { * @see #cancel() */ @Override - public void clear() { + public void clear(boolean skipPlaceholder) { Resource toRelease = null; synchronized (requestLock) { assertNotCallingCallbacks(); @@ -335,7 +335,7 @@ public void clear() { resource = null; } if (canNotifyCleared()) { - target.onLoadCleared(getPlaceholderDrawable()); + target.onLoadCleared(skipPlaceholder ? null : getPlaceholderDrawable()); } GlideTrace.endSectionAsync(TAG, cookie); diff --git a/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java index 22943d0980..8cbb144f41 100644 --- a/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java +++ b/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java @@ -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); } } diff --git a/library/test/src/test/java/com/bumptech/glide/RequestManagerTest.java b/library/test/src/test/java/com/bumptech/glide/RequestManagerTest.java index b1bf1dd9c3..600a7b3e2f 100644 --- a/library/test/src/test/java/com/bumptech/glide/RequestManagerTest.java +++ b/library/test/src/test/java/com/bumptech/glide/RequestManagerTest.java @@ -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; @@ -129,7 +130,7 @@ public void testResumesRequestsOnStart() { public void testClearsRequestsOnDestroy() { manager.onDestroy(); - verify(requestTracker).clearRequests(); + verify(requestTracker).clearRequests(true); } @Test @@ -263,6 +264,103 @@ public Set getDescendants() { parent.clear(target); } + /** + * Reproduces the ANR reported in https://github.com/bumptech/glide/issues/5398. + * + *

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 targetWithPlaceholderId = + new CustomTarget<>() { + @Override + public void onResourceReady( + @NonNull Drawable resource, @Nullable Transition 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 t = + new CustomTarget<>() { + @Override + public void onResourceReady( + @NonNull Drawable resource, @Nullable Transition 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 = diff --git a/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java b/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java index bdadcaa6e6..2b7088b20d 100644 --- a/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java +++ b/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java @@ -393,7 +393,7 @@ public void begin() { } @Override - public void clear() { + public void clear(boolean skipPlaceholder) { if (isCleared) { throw new IllegalStateException(); } diff --git a/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java b/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java index c434e6d06a..62de24f679 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java +++ b/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java @@ -46,7 +46,7 @@ public void begin_whenPrimaryIsAlreadyRunning_doesNotStartPrimaryAgain() { @Test public void clear_whenPrimaryHasNotFailed_clearsPrimary() { coordinator.clear(); - verify(primary).clear(); + verify(primary).clear(false); } @Test @@ -59,14 +59,14 @@ public void clear_whenPrimaryHasNotFailed_doesNotClearError() { public void clear_whenPrimaryHasFailed_errorIsRunning_clearsError() { coordinator.onRequestFailed(primary); coordinator.clear(); - verify(error).clear(); + verify(error).clear(false); } @Test public void clear_whenPrimaryHasFailed_clearsPrimary() { coordinator.onRequestFailed(primary); coordinator.clear(); - verify(primary).clear(); + verify(primary).clear(false); } @Test @@ -74,7 +74,7 @@ public void clear_whenErrorIsRunning_clearsError() { coordinator.onRequestFailed(primary); coordinator.clear(); - verify(error).clear(); + verify(error).clear(false); } @Test diff --git a/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java b/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java index 72fb121a6c..2bb0b7c7dd 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java +++ b/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java @@ -132,8 +132,8 @@ public Void answer(InvocationOnMock invocation) throws Throwable { public void testCallsClearOnRequestsWhenCleared() { coordinator.clear(); InOrder order = inOrder(thumb, full); - order.verify(thumb).clear(); - order.verify(full).clear(); + order.verify(thumb).clear(false); + order.verify(full).clear(false); } @Test