Fix deadlock in EngineJob.

Sometimes we acquire the Request lock, then the EngineJob lock (like
when the request is cancelled) and sometimes the other way around (like
when the request completes successfully). By always acquiring the
callback/request lock first, we avoid the deadlock.

PiperOrigin-RevId: 255225339

library/findbugs-exclude.xml

@@ -60,4 +60,10 @@
       <Bug pattern="IS2_INCONSISTENT_SYNC" />
     </Match>
 
+     <!-- Inconsistent synchronization is due to synchronizing on Engine/listener to avoid deadlock only. -->
+    <Match>
+      <Class name="com.bumptech.glide.load.engine.EngineJob" />
+      <Bug pattern="IS2_INCONSISTENT_SYNC" />
+    </Match>
+
 </FindBugsFilter>

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();
       }
     }
   }