From 368b8b845f45b3380db7b0e1ab967b94c62ff642 Mon Sep 17 00:00:00 2001
From: Diego Vela <diegovela@google.com>
Date: Thu, 13 Apr 2023 21:01:11 +0000
Subject: [PATCH] Fix deadlock in BaseDataProducer.

Move calls to abstract method outside the synchronized block. When they
are in the synchronized block it can cause a deadlock in the following
way. If two classes have locks and they interact through callbacks the
aquiring the locks can have a mixed order.  The mixed order causes a
deadlock.

Bug: 276436535
Test: Run foldable samples.
Change-Id: Ie71ec56e7ba43976fee3e69f74ff386c79c96a7a
---
 ...iceStateManagerFoldingFeatureProducer.java | 12 +++++------
 .../common/RawFoldingFeatureProducer.java     |  9 ++++----
 .../window/util/BaseDataProducer.java         | 21 ++++++++++++++-----
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java b/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java
index 66f27f517ab30..a184dff5005b5 100644
--- a/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java
+++ b/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java
@@ -39,7 +39,6 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.Set;
 import java.util.function.Consumer;
 
 /**
@@ -167,14 +166,13 @@ public final class DeviceStateManagerFoldingFeatureProducer
     }
 
     @Override
-    protected void onListenersChanged(
-            @NonNull Set<Consumer<List<CommonFoldingFeature>>> callbacks) {
-        super.onListenersChanged(callbacks);
-        if (callbacks.isEmpty()) {
+    protected void onListenersChanged() {
+        super.onListenersChanged();
+        if (hasListeners()) {
+            mRawFoldSupplier.addDataChangedCallback(this::notifyFoldingFeatureChange);
+        } else {
             mCurrentDeviceState = INVALID_DEVICE_STATE;
             mRawFoldSupplier.removeDataChangedCallback(this::notifyFoldingFeatureChange);
-        } else {
-            mRawFoldSupplier.addDataChangedCallback(this::notifyFoldingFeatureChange);
         }
     }
 
diff --git a/libs/WindowManager/Jetpack/src/androidx/window/common/RawFoldingFeatureProducer.java b/libs/WindowManager/Jetpack/src/androidx/window/common/RawFoldingFeatureProducer.java
index 7906342d445d5..8906e6d3d02e6 100644
--- a/libs/WindowManager/Jetpack/src/androidx/window/common/RawFoldingFeatureProducer.java
+++ b/libs/WindowManager/Jetpack/src/androidx/window/common/RawFoldingFeatureProducer.java
@@ -31,7 +31,6 @@ import androidx.window.util.BaseDataProducer;
 import com.android.internal.R;
 
 import java.util.Optional;
-import java.util.Set;
 import java.util.function.Consumer;
 
 /**
@@ -86,11 +85,11 @@ public final class RawFoldingFeatureProducer extends BaseDataProducer<String> {
     }
 
     @Override
-    protected void onListenersChanged(Set<Consumer<String>> callbacks) {
-        if (callbacks.isEmpty()) {
-            unregisterObserversIfNeeded();
-        } else {
+    protected void onListenersChanged() {
+        if (hasListeners()) {
             registerObserversIfNeeded();
+        } else {
+            unregisterObserversIfNeeded();
         }
     }
 
diff --git a/libs/WindowManager/Jetpack/src/androidx/window/util/BaseDataProducer.java b/libs/WindowManager/Jetpack/src/androidx/window/util/BaseDataProducer.java
index 46c925aaf8a27..de52f0969fa80 100644
--- a/libs/WindowManager/Jetpack/src/androidx/window/util/BaseDataProducer.java
+++ b/libs/WindowManager/Jetpack/src/androidx/window/util/BaseDataProducer.java
@@ -51,10 +51,10 @@ public abstract class BaseDataProducer<T> implements DataProducer<T>,
     public final void addDataChangedCallback(@NonNull Consumer<T> callback) {
         synchronized (mLock) {
             mCallbacks.add(callback);
-            Optional<T> currentData = getCurrentData();
-            currentData.ifPresent(callback);
-            onListenersChanged(mCallbacks);
         }
+        Optional<T> currentData = getCurrentData();
+        currentData.ifPresent(callback);
+        onListenersChanged();
     }
 
     /**
@@ -67,11 +67,22 @@ public abstract class BaseDataProducer<T> implements DataProducer<T>,
     public final void removeDataChangedCallback(@NonNull Consumer<T> callback) {
         synchronized (mLock) {
             mCallbacks.remove(callback);
-            onListenersChanged(mCallbacks);
+        }
+        onListenersChanged();
+    }
+
+    /**
+     * Returns {@code true} if there are any registered callbacks {@code false} if there are no
+     * registered callbacks.
+     */
+    // TODO(b/278132889) Improve the structure of BaseDataProdcuer while avoiding known issues.
+    public final boolean hasListeners() {
+        synchronized (mLock) {
+            return !mCallbacks.isEmpty();
         }
     }
 
-    protected void onListenersChanged(Set<Consumer<T>> callbacks) {}
+    protected void onListenersChanged() {}
 
     /**
      * @return the current data if available and {@code Optional.empty()} otherwise.