Merge "Synchronize DeviceStateController on WindowManagerGlobalLock" into udc-dev

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

@@ -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) {
+     * Registers a callback to be notified when the device state changes. Callers should always
-            mDeviceStateCallbacks.add(callback);
+     * 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) {
+            // Make a copy here because it's possible that the consumer tries to remove a callback
-                for (Consumer<DeviceState> callback : mDeviceStateCallbacks) {
+            // while we're still iterating through the list, which would end up in a
-                    callback.accept(mCurrentDeviceState);
+            // 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

@@ -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

@@ -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

@@ -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 = new DeviceStateController(mMockContext, mockHandler,
-            mTarget.registerDeviceStateCallback(mDelegate);
+                    new WindowManagerGlobalLock());
+            mTarget.registerDeviceStateCallback(mDelegate, MoreExecutors.directExecutor());
         }
     }
 }