From 890712279a997ded1b22d64ea6059c911b2f8a55 Mon Sep 17 00:00:00 2001
From: Sam Judd <judds@google.com>
Date: Wed, 26 Jun 2019 11:10:14 -0700
Subject: [PATCH] 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                  |  6 +++
 .../bumptech/glide/load/engine/EngineJob.java | 39 ++++++++++++-------
 2 files changed, 31 insertions(+), 14 deletions(-)

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