Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4b428bea authored by Lee Shombert's avatar Lee Shombert
Browse files

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
parent e302462c

services/core/java/com/android/server/display/DisplayManagerService.java

+39 −28
Original line number Diff line number Diff line
@@ -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 "
@@ -4268,6 +4262,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.

         */
@@ -4376,6 +4382,7 @@ 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() {

            try {

                synchronized (mCallback) {

                    if (mPendingEvents == null || mPendingEvents.isEmpty() || !mAlive) {

                        return true;
@@ -4390,14 +4397,18 @@ public final class DisplayManagerService extends SystemService { 
                                    + displayEvent.displayId + "/"

                                    + displayEvent.event + " to " + mUid + "/" + mPid);

                        }

                    if (!transmitDisplayEvent(displayEvent.displayId, displayEvent.event)) {

                        Slog.d(TAG, "Drop pending events for dead process " + mPid);

                        break;

                    }

                        transmitDisplayEvent(displayEvent.displayId, displayEvent.event);

                    }

                    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;



            }

        }



        // Return a string suitable for dumpsys.