diff --git a/library/findbugs-exclude.xml b/library/findbugs-exclude.xml index dee0fdbe5d..919868ba91 100644 --- a/library/findbugs-exclude.xml +++ b/library/findbugs-exclude.xml @@ -60,4 +60,10 @@ + + + + + + diff --git a/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java b/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java index 34bb020187..bd34729c8a 100644 --- a/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java +++ b/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java @@ -1,5 +1,6 @@ package com.bumptech.glide.load.engine; +import androidx.annotation.GuardedBy; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; import androidx.core.util.Pools; @@ -148,7 +149,8 @@ synchronized void addCallback(final ResourceCallback cb, Executor callbackExecut @SuppressWarnings("WeakerAccess") @Synthetic - synchronized void callCallbackOnResourceReady(ResourceCallback cb) { + @GuardedBy("this") + void callCallbackOnResourceReady(ResourceCallback cb) { try { // This is overly broad, some Glide code is actually called here, but it's much // simpler to encapsulate here than to do so at the actual call point in the @@ -161,7 +163,8 @@ synchronized void callCallbackOnResourceReady(ResourceCallback cb) { @SuppressWarnings("WeakerAccess") @Synthetic - synchronized void callCallbackOnLoadFailed(ResourceCallback cb) { + @GuardedBy("this") + void callCallbackOnLoadFailed(ResourceCallback cb) { // This is overly broad, some Glide code is actually called here, but it's much // simpler to encapsulate here than to do so at the actual call point in the Request // implementation. @@ -382,12 +385,16 @@ private class CallLoadFailed implements Runnable { @Override public void run() { - synchronized (EngineJob.this) { - if (cbs.contains(cb)) { - callCallbackOnLoadFailed(cb); + // Make sure we always acquire the request lock, then the EngineJob lock to avoid deadlock + // (b/136032534). + synchronized (cb) { + synchronized (EngineJob.this) { + if (cbs.contains(cb)) { + callCallbackOnLoadFailed(cb); + } + + decrementPendingCallbacks(); } - - decrementPendingCallbacks(); } } } @@ -402,14 +409,18 @@ private class CallResourceReady implements Runnable { @Override public void run() { - synchronized (EngineJob.this) { - if (cbs.contains(cb)) { - // Acquire for this particular callback. - engineResource.acquire(); - callCallbackOnResourceReady(cb); - removeCallback(cb); + // Make sure we always acquire the request lock, then the EngineJob lock to avoid deadlock + // (b/136032534). + synchronized (cb) { + synchronized (EngineJob.this) { + if (cbs.contains(cb)) { + // Acquire for this particular callback. + engineResource.acquire(); + callCallbackOnResourceReady(cb); + removeCallback(cb); + } + decrementPendingCallbacks(); } - decrementPendingCallbacks(); } } }