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

Commit 383b57ab authored by Kevin Chyn's avatar Kevin Chyn
Browse files

Synchronize DeviceStateController on WindowManagerGlobalLock 

The existing way of synchronizing on DeviceStateController.this is
prone to deadlocks. For example, this sequence of events will result in
a deadlock.

ThreadA: 1) Acquire WindowManagerGlobalLock
         4) Acquire DeviceStateController.this

ThreadB) 2) Acquire DeviceStateController.this
         3) Acquire WindowManagerGlobalLock

Instead of using DeviceStateController.this as a lock, we should just
use the existing WindowManagerGlobalLock like the rest of WM code.
This way, since all locking is done on the same global lock, there
shouldn't be a way for this deadlock to occur anymore.

Also, put work onto WindowManagerService's handler so that
DeviceStateController's lock when invoking callbacks is short.

Fixes: 281653024
Test: presubmit
Change-Id: I3c0086856c245a01c951d295a040c93c82c8a485
parent 0f6b4e23

services/core/java/com/android/server/wm/DeviceStateController.java

+35 −10
Original line number Diff line number Diff line
@@ -16,11 +16,13 @@ 


package com.android.server.wm;



import android.annotation.CallbackExecutor;

import android.annotation.NonNull;

import android.content.Context;

import android.hardware.devicestate.DeviceStateManager;

import android.os.Handler;

import android.os.HandlerExecutor;

import android.util.ArrayMap;



import com.android.internal.R;

import com.android.internal.annotations.GuardedBy;
@@ -29,6 +31,8 @@ import com.android.internal.util.ArrayUtils; 


import java.util.ArrayList;

import java.util.List;

import java.util.Map;

import java.util.concurrent.Executor;

import java.util.function.Consumer;



/**
@@ -37,6 +41,9 @@ import java.util.function.Consumer; 
 */

final class DeviceStateController implements DeviceStateManager.DeviceStateCallback {



    // Used to synchronize WindowManager services call paths with DeviceStateManager's callbacks.

    @NonNull

    private final WindowManagerGlobalLock mWmLock;

    @NonNull

    private final DeviceStateManager mDeviceStateManager;

    @NonNull
@@ -50,10 +57,10 @@ final class DeviceStateController implements DeviceStateManager.DeviceStateCallb 
    private final int mConcurrentDisplayDeviceState;

    @NonNull

    private final int[] mReverseRotationAroundZAxisStates;

    @GuardedBy("this")

    @GuardedBy("mWmLock")

    @NonNull

    @VisibleForTesting

    final List<Consumer<DeviceState>> mDeviceStateCallbacks = new ArrayList<>();

    final Map<Consumer<DeviceState>, Executor> mDeviceStateCallbacks = new ArrayMap<>();



    private final boolean mMatchBuiltInDisplayOrientationToDefaultDisplay;
@@ -70,7 +77,9 @@ final class DeviceStateController implements DeviceStateManager.DeviceStateCallb 
        CONCURRENT,

    }



    DeviceStateController(@NonNull Context context, @NonNull Handler handler) {

    DeviceStateController(@NonNull Context context, @NonNull Handler handler,

            @NonNull WindowManagerGlobalLock wmLock) {

        mWmLock = wmLock;

        mDeviceStateManager = context.getSystemService(DeviceStateManager.class);



        mOpenDeviceStates = context.getResources()
@@ -94,14 +103,20 @@ final class DeviceStateController implements DeviceStateManager.DeviceStateCallb 
        }

    }



    void registerDeviceStateCallback(@NonNull Consumer<DeviceState> callback) {

        synchronized (this) {

            mDeviceStateCallbacks.add(callback);

    /**

     * Registers a callback to be notified when the device state changes. Callers should always

     * post the work onto their own worker thread to avoid holding the WindowManagerGlobalLock for

     * an extended period of time.

     */

    void registerDeviceStateCallback(@NonNull Consumer<DeviceState> callback,

            @NonNull @CallbackExecutor Executor executor) {

        synchronized (mWmLock) {

            mDeviceStateCallbacks.put(callback, executor);

        }

    }



    void unregisterDeviceStateCallback(@NonNull Consumer<DeviceState> callback) {

        synchronized (this) {

        synchronized (mWmLock) {

            mDeviceStateCallbacks.remove(callback);

        }

    }
@@ -144,11 +159,21 @@ final class DeviceStateController implements DeviceStateManager.DeviceStateCallb 
        if (mCurrentDeviceState == null || !mCurrentDeviceState.equals(deviceState)) {

            mCurrentDeviceState = deviceState;



            synchronized (this) {

                for (Consumer<DeviceState> callback : mDeviceStateCallbacks) {

                    callback.accept(mCurrentDeviceState);

            // Make a copy here because it's possible that the consumer tries to remove a callback

            // while we're still iterating through the list, which would end up in a

            // ConcurrentModificationException.

            final List<Map.Entry<Consumer<DeviceState>, Executor>> entries = new ArrayList<>();

            synchronized (mWmLock) {

                for (Map.Entry<Consumer<DeviceState>, Executor> entry

                        : mDeviceStateCallbacks.entrySet()) {

                    entries.add(entry);

                }

            }



            for (int i = 0; i < entries.size(); i++) {

                Map.Entry<Consumer<DeviceState>, Executor> entry = entries.get(i);

                entry.getValue().execute(() -> entry.getKey().accept(mCurrentDeviceState));

            }

        }

    }

}

services/core/java/com/android/server/wm/DisplayContent.java

+3 −1
Original line number Diff line number Diff line
@@ -188,6 +188,7 @@ import android.metrics.LogMaker; 
import android.os.Bundle;

import android.os.Debug;

import android.os.Handler;

import android.os.HandlerExecutor;

import android.os.IBinder;

import android.os.Message;

import android.os.PowerManager;
@@ -1182,7 +1183,8 @@ class DisplayContent extends RootDisplayArea implements WindowManagerPolicy.Disp 
                    mDisplaySwitchTransitionLauncher.foldStateChanged(newFoldState);

                    mDisplayRotation.foldStateChanged(newFoldState);

                };

        mDeviceStateController.registerDeviceStateCallback(mDeviceStateConsumer);

        mDeviceStateController.registerDeviceStateCallback(mDeviceStateConsumer,

                new HandlerExecutor(mWmService.mH));



        mCloseToSquareMaxAspectRatio = mWmService.mContext.getResources().getFloat(

                R.dimen.config_closeToSquareDisplayMaxAspectRatio);

services/core/java/com/android/server/wm/RootWindowContainer.java

+2 −1
Original line number Diff line number Diff line
@@ -438,7 +438,8 @@ class RootWindowContainer extends WindowContainer<DisplayContent> 
        mTaskSupervisor = mService.mTaskSupervisor;

        mTaskSupervisor.mRootWindowContainer = this;

        mDisplayOffTokenAcquirer = mService.new SleepTokenAcquirerImpl(DISPLAY_OFF_SLEEP_TOKEN_TAG);

        mDeviceStateController = new DeviceStateController(service.mContext, service.mH);

        mDeviceStateController = new DeviceStateController(service.mContext, service.mH,

                service.mGlobalLock);

        mDisplayRotationCoordinator = new DisplayRotationCoordinator();

    }

services/tests/wmtests/src/com/android/server/wm/DeviceStateControllerTests.java

+6 −3
Original line number Diff line number Diff line
@@ -34,6 +34,8 @@ import androidx.test.filters.SmallTest; 


import com.android.internal.R;



import com.google.common.util.concurrent.MoreExecutors;



import org.junit.Before;

import org.junit.Test;
@@ -117,7 +119,7 @@ public class DeviceStateControllerTests { 
    public void testUnregisterDeviceStateCallback() {

        initialize(true /* supportFold */, true /* supportHalfFolded */);

        assertEquals(1, mTarget.mDeviceStateCallbacks.size());

        assertEquals(mDelegate, mTarget.mDeviceStateCallbacks.get(0));

        assertTrue(mTarget.mDeviceStateCallbacks.containsKey(mDelegate));



        mTarget.onStateChanged(mOpenDeviceStates[0]);

        assertEquals(DeviceStateController.DeviceState.OPEN, mCurrentState);
@@ -194,8 +196,9 @@ public class DeviceStateControllerTests { 
            when(mMockContext.getResources()).thenReturn((mockRes));

            mockFold(mSupportFold, mSupportHalfFold);

            Handler mockHandler = mock(Handler.class);

            mTarget = new DeviceStateController(mMockContext, mockHandler);

            mTarget.registerDeviceStateCallback(mDelegate);

            mTarget = new DeviceStateController(mMockContext, mockHandler,

                    new WindowManagerGlobalLock());

            mTarget.registerDeviceStateCallback(mDelegate, MoreExecutors.directExecutor());

        }

    }

}