Fix deadlock in binder death handling

CallbackRecord.binderDied() triggers a new thread that tries to lock
CallbackRecord.mCallbacks.  Thus, binderDied() may not be called while
holding mCallbacks.

This change moves the exception handler for RemoteException
(indicating that the remote end has died) out of synchronization
blocks on mCallbacks.

Flag: com.android.server.am.defer_display_events_when_frozen
Bug: 388848221
Test: atest
 * DisplayServiceTests
 * CtsDisplayTestCases
Change-Id: Ib4a62ccc99725eb0c8f97ff2934fd19fc22417d3

1 files changed, 39 insertions, 28 deletions

diff --git a/services/core/java/com/android/server/display/DisplayManagerService.java b/services/core/java/com/android/server/display/DisplayManagerService.java
index b530da2a5f5e..ca001b9c7e6d 100644
--- a/services/core/java/com/android/server/display/DisplayManagerService.java
+++ b/services/core/java/com/android/server/display/DisplayManagerService.java
@@ -4188,6 +4188,9 @@ public final class DisplayManagerService extends SystemService {
             }
         }
 
+        /**
+         * This method must not be called with mCallback held or deadlock will ensue.
+         */
         @Override
         public void binderDied() {
             synchronized (mCallback) {
@@ -4248,17 +4251,8 @@ public final class DisplayManagerService extends SystemService {
                 }
             }
 
-            return transmitDisplayEvent(displayId, event);
-        }
-
-        /**
-         * Transmit a single display event.  The client is presumed ready.  Return true on success
-         * and false if the client died.
-         */
-        private boolean transmitDisplayEvent(int displayId, @DisplayEvent int event) {
-            // The client is ready to receive the event.
             try {
-                mCallback.onDisplayEvent(displayId, event);
+                transmitDisplayEvent(displayId, event);
                 return true;
             } catch (RemoteException ex) {
                 Slog.w(TAG, "Failed to notify process "
@@ -4269,6 +4263,18 @@ public final class DisplayManagerService extends SystemService {
         }
 
         /**
+         * Transmit a single display event.  The client is presumed ready.  This throws if the
+         * client has died; callers must catch and handle the exception.  The exception cannot be
+         * handled directly here because {@link #binderDied()} must not be called whilst holding
+         * the mCallback lock.
+         */
+        private void transmitDisplayEvent(int displayId, @DisplayEvent int event)
+                throws RemoteException {
+            // The client is ready to receive the event.
+            mCallback.onDisplayEvent(displayId, event);
+        }
+
+        /**
          * Return true if the client is interested in this event.
          */
         private boolean shouldSendDisplayEvent(@DisplayEvent int event) {
@@ -4376,27 +4382,32 @@ public final class DisplayManagerService extends SystemService {
         // would be unusual to do so.  The method returns true on success.
         // This is only used if {@link deferDisplayEventsWhenFrozen()} is true.
         public boolean dispatchPending() {
-            synchronized (mCallback) {
-                if (mPendingEvents == null || mPendingEvents.isEmpty() || !mAlive) {
-                    return true;
-                }
-                if (!isReadyLocked()) {
-                    return false;
-                }
-                for (int i = 0; i < mPendingEvents.size(); i++) {
-                    Event displayEvent = mPendingEvents.get(i);
-                    if (DEBUG) {
-                        Slog.d(TAG, "Send pending display event #" + i + " "
-                                + displayEvent.displayId + "/"
-                                + displayEvent.event + " to " + mUid + "/" + mPid);
+            try {
+                synchronized (mCallback) {
+                    if (mPendingEvents == null || mPendingEvents.isEmpty() || !mAlive) {
+                        return true;
+                    }
+                    if (!isReadyLocked()) {
+                        return false;
                     }
-                    if (!transmitDisplayEvent(displayEvent.displayId, displayEvent.event)) {
-                        Slog.d(TAG, "Drop pending events for dead process " + mPid);
-                        break;
+                    for (int i = 0; i < mPendingEvents.size(); i++) {
+                        Event displayEvent = mPendingEvents.get(i);
+                        if (DEBUG) {
+                            Slog.d(TAG, "Send pending display event #" + i + " "
+                                    + displayEvent.displayId + "/"
+                                    + displayEvent.event + " to " + mUid + "/" + mPid);
+                        }
+                        transmitDisplayEvent(displayEvent.displayId, displayEvent.event);
                     }
+                    mPendingEvents.clear();
+                    return true;
                 }
-                mPendingEvents.clear();
-                return true;
+            } catch (RemoteException ex) {
+                Slog.w(TAG, "Failed to notify process "
+                        + mPid + " that display topology changed, assuming it died.", ex);
+                binderDied();
+                return false;
+
             }
         }