Merge "IMS: Refactor additional display input properties to avoid deadlock" into tm-dev am: 3c232f03f3

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/18487052

Change-Id: I7712d6136aeb4069ec0d41d3873c08e9919f4d21
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

services/core/java/com/android/server/input/InputManagerService.java

@@ -146,6 +146,7 @@ import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.OptionalInt;
+import java.util.function.Consumer;
 
 /** The system implementation of {@link IInputManager} that manages input devices. */
 public class InputManagerService extends IInputManager.Stub
@@ -168,6 +169,8 @@ public class InputManagerService extends IInputManager.Stub
     private static final int MSG_POINTER_DISPLAY_ID_CHANGED = 7;
 
     private static final int DEFAULT_VIBRATION_MAGNITUDE = 192;
+    private static final AdditionalDisplayInputProperties
+            DEFAULT_ADDITIONAL_DISPLAY_INPUT_PROPERTIES = new AdditionalDisplayInputProperties();
 
     /**
      * We know the issue and are working to fix it, so suppressing the toast to not annoy
@@ -281,6 +284,7 @@ public class InputManagerService extends IInputManager.Stub
     // Guards per-display input properties and properties relating to the mouse pointer.
     // Threads can wait on this lock to be notified the next time the display on which the mouse
     // pointer is shown has changed.
+    // WARNING: Do not call other services outside of input while holding this lock.
     private final Object mAdditionalDisplayInputPropertiesLock = new Object();
 
     // Forces the PointerController to target a specific display id.
@@ -299,6 +303,11 @@ public class InputManagerService extends IInputManager.Stub
     @GuardedBy("mAdditionalDisplayInputPropertiesLock")
     private final SparseArray<AdditionalDisplayInputProperties> mAdditionalDisplayInputProperties =
             new SparseArray<>();
+    // This contains the per-display properties that are currently applied by native code. It should
+    // be kept in sync with the properties for mRequestedPointerDisplayId.
+    @GuardedBy("mAdditionalDisplayInputPropertiesLock")
+    private final AdditionalDisplayInputProperties mCurrentDisplayProperties =
+            new AdditionalDisplayInputProperties();
     @GuardedBy("mAdditionalDisplayInputPropertiesLock")
     private int mIconType = PointerIcon.TYPE_NOT_SPECIFIED;
     @GuardedBy("mAdditionalDisplayInputPropertiesLock")
@@ -571,28 +580,20 @@ public class InputManagerService extends IInputManager.Stub
     }
 
     private void setDisplayViewportsInternal(List<DisplayViewport> viewports) {
-        synchronized (mAdditionalDisplayInputPropertiesLock) {
         final DisplayViewport[] vArray = new DisplayViewport[viewports.size()];
         for (int i = viewports.size() - 1; i >= 0; --i) {
             vArray[i] = viewports.get(i);
         }
         mNative.setDisplayViewports(vArray);
-            // Always attempt to update the pointer display when viewports change.
-            updatePointerDisplayId();
 
-            if (mOverriddenPointerDisplayId != Display.INVALID_DISPLAY) {
+        // Attempt to update the pointer display when viewports change when there is no override.
-                final AdditionalDisplayInputProperties properties =
+        // Take care to not make calls to window manager while holding internal locks.
-                        mAdditionalDisplayInputProperties.get(mOverriddenPointerDisplayId);
+        final int pointerDisplayId = mWindowManagerCallbacks.getPointerDisplayId();
-                if (properties != null) {
+        synchronized (mAdditionalDisplayInputPropertiesLock) {
-                    updatePointerIconVisibleLocked(properties.pointerIconVisible);
+            if (mOverriddenPointerDisplayId == Display.INVALID_DISPLAY) {
-                    updatePointerAccelerationLocked(properties.pointerAcceleration);
+                updatePointerDisplayIdLocked(pointerDisplayId);
-                    return;
             }
         }
-            updatePointerIconVisibleLocked(
-                    AdditionalDisplayInputProperties.DEFAULT_POINTER_ICON_VISIBLE);
-            updatePointerAccelerationLocked(IInputConstants.DEFAULT_POINTER_ACCELERATION);
-        }
     }
 
     /**
@@ -1743,12 +1744,7 @@ public class InputManagerService extends IInputManager.Stub
             mPointerIconDisplayContext = null;
         }
 
-        synchronized (mAdditionalDisplayInputPropertiesLock) {
+        updateAdditionalDisplayInputProperties(displayId, AdditionalDisplayInputProperties::reset);
-            setPointerIconVisible(AdditionalDisplayInputProperties.DEFAULT_POINTER_ICON_VISIBLE,
-                    displayId);
-            setPointerAcceleration(AdditionalDisplayInputProperties.DEFAULT_POINTER_ACCELERATION,
-                    displayId);
-        }
 
         mNative.displayRemoved(displayId);
     }
@@ -1835,57 +1831,13 @@ public class InputManagerService extends IInputManager.Stub
     }
 
     private void setPointerAcceleration(float acceleration, int displayId) {
-        synchronized (mAdditionalDisplayInputPropertiesLock) {
+        updateAdditionalDisplayInputProperties(displayId,
-            AdditionalDisplayInputProperties properties =
+                properties -> properties.pointerAcceleration = acceleration);
-                    mAdditionalDisplayInputProperties.get(displayId);
-            if (properties == null) {
-                properties = new AdditionalDisplayInputProperties();
-                mAdditionalDisplayInputProperties.put(displayId, properties);
-            }
-            properties.pointerAcceleration = acceleration;
-            if (properties.allDefaults()) {
-                mAdditionalDisplayInputProperties.remove(displayId);
-            }
-            if (mOverriddenPointerDisplayId == displayId) {
-                updatePointerAccelerationLocked(acceleration);
-            }
-        }
-    }
-
-    @GuardedBy("mAdditionalDisplayInputPropertiesLock")
-    private void updatePointerAccelerationLocked(float acceleration) {
-        mNative.setPointerAcceleration(acceleration);
     }
 
     private void setPointerIconVisible(boolean visible, int displayId) {
-        synchronized (mAdditionalDisplayInputPropertiesLock) {
+        updateAdditionalDisplayInputProperties(displayId,
-            AdditionalDisplayInputProperties properties =
+                properties -> properties.pointerIconVisible = visible);
-                    mAdditionalDisplayInputProperties.get(displayId);
-            if (properties == null) {
-                properties = new AdditionalDisplayInputProperties();
-                mAdditionalDisplayInputProperties.put(displayId, properties);
-            }
-            properties.pointerIconVisible = visible;
-            if (properties.allDefaults()) {
-                mAdditionalDisplayInputProperties.remove(displayId);
-            }
-            if (mOverriddenPointerDisplayId == displayId) {
-                updatePointerIconVisibleLocked(visible);
-            }
-        }
-    }
-
-    @GuardedBy("mAdditionalDisplayInputPropertiesLock")
-    private void updatePointerIconVisibleLocked(boolean visible) {
-        if (visible) {
-            if (mIconType == PointerIcon.TYPE_CUSTOM) {
-                mNative.setCustomPointerIcon(mIcon);
-            } else {
-                mNative.setPointerIconType(mIconType);
-            }
-        } else {
-            mNative.setPointerIconType(PointerIcon.TYPE_NULL);
-        }
     }
 
     private void registerPointerSpeedSettingObserver() {
@@ -2023,23 +1975,19 @@ public class InputManagerService extends IInputManager.Stub
 
     /**
      * Update the display on which the mouse pointer is shown.
-     * If there is an overridden display for the mouse pointer, use that. Otherwise, query
-     * WindowManager for the pointer display.
      *
      * @return true if the pointer displayId changed, false otherwise.
      */
-    private boolean updatePointerDisplayId() {
+    @GuardedBy("mAdditionalDisplayInputPropertiesLock")
-        synchronized (mAdditionalDisplayInputPropertiesLock) {
+    private boolean updatePointerDisplayIdLocked(int pointerDisplayId) {
-            final int pointerDisplayId = mOverriddenPointerDisplayId != Display.INVALID_DISPLAY
-                    ? mOverriddenPointerDisplayId : mWindowManagerCallbacks.getPointerDisplayId();
         if (mRequestedPointerDisplayId == pointerDisplayId) {
             return false;
         }
         mRequestedPointerDisplayId = pointerDisplayId;
         mNative.setPointerDisplayId(pointerDisplayId);
+        applyAdditionalDisplayInputProperties();
         return true;
     }
-    }
 
     private void handlePointerDisplayIdChanged(PointerDisplayIdChangedArgs args) {
         synchronized (mAdditionalDisplayInputPropertiesLock) {
@@ -2051,25 +1999,23 @@ public class InputManagerService extends IInputManager.Stub
                 args.mPointerDisplayId, args.mXPosition, args.mYPosition);
     }
 
-    private boolean setVirtualMousePointerDisplayIdBlocking(int displayId) {
+    private boolean setVirtualMousePointerDisplayIdBlocking(int overrideDisplayId) {
-        // Indicates whether this request is for removing the override.
+        final boolean isRemovingOverride = overrideDisplayId == Display.INVALID_DISPLAY;
-        final boolean removingOverride = displayId == Display.INVALID_DISPLAY;
+
+        // Take care to not make calls to window manager while holding internal locks.
+        final int resolvedDisplayId = isRemovingOverride
+                ? mWindowManagerCallbacks.getPointerDisplayId()
+                : overrideDisplayId;
 
         synchronized (mAdditionalDisplayInputPropertiesLock) {
-            mOverriddenPointerDisplayId = displayId;
+            mOverriddenPointerDisplayId = overrideDisplayId;
-            if (!removingOverride) {
+
-                final AdditionalDisplayInputProperties properties =
+            if (!updatePointerDisplayIdLocked(resolvedDisplayId)
-                        mAdditionalDisplayInputProperties.get(displayId);
+                    && mAcknowledgedPointerDisplayId == resolvedDisplayId) {
-                if (properties != null) {
-                    updatePointerAccelerationLocked(properties.pointerAcceleration);
-                    updatePointerIconVisibleLocked(properties.pointerIconVisible);
-                }
-            }
-            if (!updatePointerDisplayId() && mAcknowledgedPointerDisplayId == displayId) {
                 // The requested pointer display is already set.
                 return true;
             }
-            if (removingOverride && mAcknowledgedPointerDisplayId == Display.INVALID_DISPLAY) {
+            if (isRemovingOverride && mAcknowledgedPointerDisplayId == Display.INVALID_DISPLAY) {
                 // The pointer display override is being removed, but the current pointer display
                 // is already invalid. This can happen when the PointerController is destroyed as a
                 // result of the removal of all input devices that can control the pointer.
@@ -2087,7 +2033,7 @@ public class InputManagerService extends IInputManager.Stub
             //   reported new displayId is the one we requested. This check ensures that if two
             //   competing overrides are requested in succession, the caller can be notified if one
             //   of them fails.
-            return  removingOverride || mAcknowledgedPointerDisplayId == displayId;
+            return  isRemovingOverride || mAcknowledgedPointerDisplayId == overrideDisplayId;
         }
     }
 
@@ -2393,16 +2339,11 @@ public class InputManagerService extends IInputManager.Stub
         synchronized (mAdditionalDisplayInputPropertiesLock) {
             mIcon = null;
             mIconType = iconType;
-            if (mOverriddenPointerDisplayId != Display.INVALID_DISPLAY) {
+
-                final AdditionalDisplayInputProperties properties =
+            if (!mCurrentDisplayProperties.pointerIconVisible) return;
-                        mAdditionalDisplayInputProperties.get(mOverriddenPointerDisplayId);
+
-                if (properties == null || properties.pointerIconVisible) {
             mNative.setPointerIconType(mIconType);
         }
-            } else {
-                mNative.setPointerIconType(mIconType);
-            }
-        }
     }
 
     // Binder call
@@ -2412,18 +2353,11 @@ public class InputManagerService extends IInputManager.Stub
         synchronized (mAdditionalDisplayInputPropertiesLock) {
             mIconType = PointerIcon.TYPE_CUSTOM;
             mIcon = icon;
-            if (mOverriddenPointerDisplayId != Display.INVALID_DISPLAY) {
+
-                final AdditionalDisplayInputProperties properties =
+            if (!mCurrentDisplayProperties.pointerIconVisible) return;
-                        mAdditionalDisplayInputProperties.get(mOverriddenPointerDisplayId);
+
-                if (properties == null || properties.pointerIconVisible) {
-                    // Only set the icon if it is not currently hidden; otherwise, it will be set
-                    // once it's no longer hidden.
             mNative.setCustomPointerIcon(mIcon);
         }
-            } else {
-                mNative.setCustomPointerIcon(mIcon);
-            }
-        }
     }
 
     /**
@@ -3852,14 +3786,79 @@ public class InputManagerService extends IInputManager.Stub
                 (float) IInputConstants.DEFAULT_POINTER_ACCELERATION;
 
         // The pointer acceleration for this display.
-        public float pointerAcceleration = DEFAULT_POINTER_ACCELERATION;
+        public float pointerAcceleration;
 
         // Whether the pointer icon should be visible or hidden on this display.
-        public boolean pointerIconVisible = DEFAULT_POINTER_ICON_VISIBLE;
+        public boolean pointerIconVisible;
+
+        AdditionalDisplayInputProperties() {
+            reset();
+        }
 
         public boolean allDefaults() {
             return Float.compare(pointerAcceleration, DEFAULT_POINTER_ACCELERATION) == 0
                     && pointerIconVisible == DEFAULT_POINTER_ICON_VISIBLE;
         }
+
+        public void reset() {
+            pointerAcceleration = DEFAULT_POINTER_ACCELERATION;
+            pointerIconVisible = DEFAULT_POINTER_ICON_VISIBLE;
+        }
+    }
+
+    private void applyAdditionalDisplayInputProperties() {
+        synchronized (mAdditionalDisplayInputPropertiesLock) {
+            AdditionalDisplayInputProperties properties =
+                    mAdditionalDisplayInputProperties.get(mRequestedPointerDisplayId);
+            if (properties == null) properties = DEFAULT_ADDITIONAL_DISPLAY_INPUT_PROPERTIES;
+            applyAdditionalDisplayInputPropertiesLocked(properties);
+        }
+    }
+
+    @GuardedBy("mAdditionalDisplayInputPropertiesLock")
+    private void applyAdditionalDisplayInputPropertiesLocked(
+            AdditionalDisplayInputProperties properties) {
+        // Handle changes to each of the individual properties.
+        if (properties.pointerIconVisible != mCurrentDisplayProperties.pointerIconVisible) {
+            mCurrentDisplayProperties.pointerIconVisible = properties.pointerIconVisible;
+            if (properties.pointerIconVisible) {
+                if (mIconType == PointerIcon.TYPE_CUSTOM) {
+                    Objects.requireNonNull(mIcon);
+                    mNative.setCustomPointerIcon(mIcon);
+                } else {
+                    mNative.setPointerIconType(mIconType);
+                }
+            } else {
+                mNative.setPointerIconType(PointerIcon.TYPE_NULL);
+            }
+        }
+
+        if (properties.pointerAcceleration != mCurrentDisplayProperties.pointerAcceleration) {
+            mCurrentDisplayProperties.pointerAcceleration = properties.pointerAcceleration;
+            mNative.setPointerAcceleration(properties.pointerAcceleration);
+        }
+    }
+
+    private void updateAdditionalDisplayInputProperties(int displayId,
+            Consumer<AdditionalDisplayInputProperties> updater) {
+        synchronized (mAdditionalDisplayInputPropertiesLock) {
+            AdditionalDisplayInputProperties properties =
+                    mAdditionalDisplayInputProperties.get(displayId);
+            if (properties == null) {
+                properties = new AdditionalDisplayInputProperties();
+                mAdditionalDisplayInputProperties.put(displayId, properties);
+            }
+            updater.accept(properties);
+            if (properties.allDefaults()) {
+                mAdditionalDisplayInputProperties.remove(displayId);
+            }
+            if (displayId != mRequestedPointerDisplayId) {
+                Log.i(TAG, "Not applying additional properties for display " + displayId
+                        + " because the pointer is currently targeting display "
+                        + mRequestedPointerDisplayId + ".");
+                return;
+            }
+            applyAdditionalDisplayInputPropertiesLocked(properties);
+        }
     }
 }

services/tests/servicestests/src/com/android/server/input/InputManagerServiceTests.kt

@@ -36,6 +36,7 @@ import org.mockito.ArgumentMatchers.anyInt
 import org.mockito.ArgumentMatchers.eq
 import org.mockito.Mock
 import org.mockito.Mockito.`when`
+import org.mockito.Mockito.clearInvocations
 import org.mockito.Mockito.doAnswer
 import org.mockito.Mockito.never
 import org.mockito.Mockito.spy
@@ -226,7 +227,8 @@ class InputManagerServiceTests {
 
     @Test
     fun onDisplayRemoved_resetAllAdditionalInputProperties() {
-        localService.setVirtualMousePointerDisplayId(10)
+        setVirtualMousePointerDisplayIdAndVerify(10)
+
         localService.setPointerIconVisible(false, 10)
         verify(native).setPointerIconType(eq(PointerIcon.TYPE_NULL))
         localService.setPointerAcceleration(5f, 10)
@@ -237,9 +239,66 @@ class InputManagerServiceTests {
         verify(native).setPointerIconType(eq(PointerIcon.TYPE_NOT_SPECIFIED))
         verify(native).setPointerAcceleration(
             eq(IInputConstants.DEFAULT_POINTER_ACCELERATION.toFloat()))
+        verifyNoMoreInteractions(native)
 
+        // This call should not block because the virtual mouse pointer override was never removed.
         localService.setVirtualMousePointerDisplayId(10)
+
         verify(native).setPointerDisplayId(eq(10))
         verifyNoMoreInteractions(native)
     }
+
+    @Test
+    fun updateAdditionalInputPropertiesForOverrideDisplay() {
+        setVirtualMousePointerDisplayIdAndVerify(10)
+
+        localService.setPointerIconVisible(false, 10)
+        verify(native).setPointerIconType(eq(PointerIcon.TYPE_NULL))
+        localService.setPointerAcceleration(5f, 10)
+        verify(native).setPointerAcceleration(eq(5f))
+
+        localService.setPointerIconVisible(true, 10)
+        verify(native).setPointerIconType(eq(PointerIcon.TYPE_NOT_SPECIFIED))
+        localService.setPointerAcceleration(1f, 10)
+        verify(native).setPointerAcceleration(eq(1f))
+
+        // Verify that setting properties on a different display is not propagated until the
+        // pointer is moved to that display.
+        localService.setPointerIconVisible(false, 20)
+        localService.setPointerAcceleration(6f, 20)
+        verifyNoMoreInteractions(native)
+
+        clearInvocations(native)
+        setVirtualMousePointerDisplayIdAndVerify(20)
+
+        verify(native).setPointerIconType(eq(PointerIcon.TYPE_NULL))
+        verify(native).setPointerAcceleration(eq(6f))
+    }
+
+    @Test
+    fun setAdditionalInputPropertiesBeforeOverride() {
+        localService.setPointerIconVisible(false, 10)
+        localService.setPointerAcceleration(5f, 10)
+
+        verifyNoMoreInteractions(native)
+
+        setVirtualMousePointerDisplayIdAndVerify(10)
+
+        verify(native).setPointerIconType(eq(PointerIcon.TYPE_NULL))
+        verify(native).setPointerAcceleration(eq(5f))
+    }
+
+    private fun setVirtualMousePointerDisplayIdAndVerify(overrideDisplayId: Int) {
+        val thread = Thread { localService.setVirtualMousePointerDisplayId(overrideDisplayId) }
+        thread.start()
+
+        // Allow some time for the set override call to park while waiting for the native callback.
+        Thread.sleep(100 /*millis*/)
+        verify(native).setPointerDisplayId(overrideDisplayId)
+
+        service.onPointerDisplayIdChanged(overrideDisplayId, 0f, 0f)
+        testLooper.dispatchNext()
+        verify(wmCallbacks).notifyPointerDisplayIdChanged(overrideDisplayId, 0f, 0f)
+        thread.join(100 /*millis*/)
+    }
 }