PointerChoreographer: Do not call the policy with the lock held

Since there is already precedent for InputReader to interact with the
policy with its lock held, we must not do the same from other
components, since it can result in deadlocks.

In this CL, we ensure that the PointerChoreographer does not interact
with the policy while holding its lock. The exception is the use of the
factory method for PointerController, which is only part of the policy
to work around dependency issues with graphics libraries.

Bug: 327717240
Test: atest inputflinger_tests
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:5a51a2280743605a51e2f9d632077e9297276520)
Merged-In: Ib81d72898a212275d95f9d84d89a16e7172e108e
Change-Id: Ib81d72898a212275d95f9d84d89a16e7172e108e

3 files changed, 91 insertions, 42 deletions

diff --git a/services/inputflinger/PointerChoreographer.cpp b/services/inputflinger/PointerChoreographer.cpp
index 3e7c1c71ef..9db3574389 100644
--- a/services/inputflinger/PointerChoreographer.cpp
+++ b/services/inputflinger/PointerChoreographer.cpp
@@ -26,6 +26,7 @@
 namespace android {
 
 namespace {
+
 bool isFromMouse(const NotifyMotionArgs& args) {
     return isFromSource(args.source, AINPUT_SOURCE_MOUSE) &&
             args.pointerProperties[0].toolType == ToolType::MOUSE;
@@ -44,13 +45,23 @@ bool isHoverAction(int32_t action) {
 bool isStylusHoverEvent(const NotifyMotionArgs& args) {
     return isStylusEvent(args.source, args.pointerProperties) && isHoverAction(args.action);
 }
+
+inline void notifyPointerDisplayChange(std::optional<std::tuple<int32_t, FloatPoint>> change,
+                                       PointerChoreographerPolicyInterface& policy) {
+    if (!change) {
+        return;
+    }
+    const auto& [displayId, cursorPosition] = *change;
+    policy.notifyPointerDisplayIdChanged(displayId, cursorPosition);
+}
+
 } // namespace
 
 // --- PointerChoreographer ---
 
 PointerChoreographer::PointerChoreographer(InputListenerInterface& listener,
                                            PointerChoreographerPolicyInterface& policy)
-      : mTouchControllerConstructor([this]() REQUIRES(mLock) {
+      : mTouchControllerConstructor([this]() {
             return mPolicy.createPointerController(
                     PointerControllerInterface::ControllerType::TOUCH);
         }),
@@ -62,10 +73,16 @@ PointerChoreographer::PointerChoreographer(InputListenerInterface& listener,
         mStylusPointerIconEnabled(false) {}
 
 void PointerChoreographer::notifyInputDevicesChanged(const NotifyInputDevicesChangedArgs& args) {
-    std::scoped_lock _l(mLock);
+    PointerDisplayChange pointerDisplayChange;
+
+    { // acquire lock
+        std::scoped_lock _l(mLock);
+
+        mInputDeviceInfos = args.inputDeviceInfos;
+        pointerDisplayChange = updatePointerControllersLocked();
+    } // release lock
 
-    mInputDeviceInfos = args.inputDeviceInfos;
-    updatePointerControllersLocked();
+    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);
     mNextListener.notify(args);
 }
 
@@ -329,7 +346,7 @@ bool PointerChoreographer::canUnfadeOnDisplay(int32_t displayId) {
     return mDisplaysWithPointersHidden.find(displayId) == mDisplaysWithPointersHidden.end();
 }
 
-void PointerChoreographer::updatePointerControllersLocked() {
+PointerChoreographer::PointerDisplayChange PointerChoreographer::updatePointerControllersLocked() {
     std::set<int32_t /*displayId*/> mouseDisplaysToKeep;
     std::set<DeviceId> touchDevicesToKeep;
     std::set<DeviceId> stylusDevicesToKeep;
@@ -378,11 +395,12 @@ void PointerChoreographer::updatePointerControllersLocked() {
                 mInputDeviceInfos.end();
     });
 
-    // Notify the policy if there's a change on the pointer display ID.
-    notifyPointerDisplayIdChangedLocked();
+    // Check if we need to notify the policy if there's a change on the pointer display ID.
+    return calculatePointerDisplayChangeToNotify();
 }
 
-void PointerChoreographer::notifyPointerDisplayIdChangedLocked() {
+PointerChoreographer::PointerDisplayChange
+PointerChoreographer::calculatePointerDisplayChangeToNotify() {
     int32_t displayIdToNotify = ADISPLAY_ID_NONE;
     FloatPoint cursorPosition = {0, 0};
     if (const auto it = mMousePointersByDisplay.find(mDefaultMouseDisplayId);
@@ -394,38 +412,49 @@ void PointerChoreographer::notifyPointerDisplayIdChangedLocked() {
         displayIdToNotify = pointerController->getDisplayId();
         cursorPosition = pointerController->getPosition();
     }
-
     if (mNotifiedPointerDisplayId == displayIdToNotify) {
-        return;
+        return {};
     }
-    mPolicy.notifyPointerDisplayIdChanged(displayIdToNotify, cursorPosition);
     mNotifiedPointerDisplayId = displayIdToNotify;
+    return {{displayIdToNotify, cursorPosition}};
 }
 
 void PointerChoreographer::setDefaultMouseDisplayId(int32_t displayId) {
-    std::scoped_lock _l(mLock);
+    PointerDisplayChange pointerDisplayChange;
+
+    { // acquire lock
+        std::scoped_lock _l(mLock);
 
-    mDefaultMouseDisplayId = displayId;
-    updatePointerControllersLocked();
+        mDefaultMouseDisplayId = displayId;
+        pointerDisplayChange = updatePointerControllersLocked();
+    } // release lock
+
+    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);
 }
 
 void PointerChoreographer::setDisplayViewports(const std::vector<DisplayViewport>& viewports) {
-    std::scoped_lock _l(mLock);
-    for (const auto& viewport : viewports) {
-        const int32_t displayId = viewport.displayId;
-        if (const auto it = mMousePointersByDisplay.find(displayId);
-            it != mMousePointersByDisplay.end()) {
-            it->second->setDisplayViewport(viewport);
-        }
-        for (const auto& [deviceId, stylusPointerController] : mStylusPointersByDevice) {
-            const InputDeviceInfo* info = findInputDeviceLocked(deviceId);
-            if (info && info->getAssociatedDisplayId() == displayId) {
-                stylusPointerController->setDisplayViewport(viewport);
+    PointerDisplayChange pointerDisplayChange;
+
+    { // acquire lock
+        std::scoped_lock _l(mLock);
+        for (const auto& viewport : viewports) {
+            const int32_t displayId = viewport.displayId;
+            if (const auto it = mMousePointersByDisplay.find(displayId);
+                it != mMousePointersByDisplay.end()) {
+                it->second->setDisplayViewport(viewport);
+            }
+            for (const auto& [deviceId, stylusPointerController] : mStylusPointersByDevice) {
+                const InputDeviceInfo* info = findInputDeviceLocked(deviceId);
+                if (info && info->getAssociatedDisplayId() == displayId) {
+                    stylusPointerController->setDisplayViewport(viewport);
+                }
             }
         }
-    }
-    mViewports = viewports;
-    notifyPointerDisplayIdChangedLocked();
+        mViewports = viewports;
+        pointerDisplayChange = calculatePointerDisplayChangeToNotify();
+    } // release lock
+
+    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);
 }
 
 std::optional<DisplayViewport> PointerChoreographer::getViewportForPointerDevice(
@@ -449,21 +478,33 @@ FloatPoint PointerChoreographer::getMouseCursorPosition(int32_t displayId) {
 }
 
 void PointerChoreographer::setShowTouchesEnabled(bool enabled) {
-    std::scoped_lock _l(mLock);
-    if (mShowTouchesEnabled == enabled) {
-        return;
-    }
-    mShowTouchesEnabled = enabled;
-    updatePointerControllersLocked();
+    PointerDisplayChange pointerDisplayChange;
+
+    { // acquire lock
+        std::scoped_lock _l(mLock);
+        if (mShowTouchesEnabled == enabled) {
+            return;
+        }
+        mShowTouchesEnabled = enabled;
+        pointerDisplayChange = updatePointerControllersLocked();
+    } // release lock
+
+    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);
 }
 
 void PointerChoreographer::setStylusPointerIconEnabled(bool enabled) {
-    std::scoped_lock _l(mLock);
-    if (mStylusPointerIconEnabled == enabled) {
-        return;
-    }
-    mStylusPointerIconEnabled = enabled;
-    updatePointerControllersLocked();
+    PointerDisplayChange pointerDisplayChange;
+
+    { // acquire lock
+        std::scoped_lock _l(mLock);
+        if (mStylusPointerIconEnabled == enabled) {
+            return;
+        }
+        mStylusPointerIconEnabled = enabled;
+        pointerDisplayChange = updatePointerControllersLocked();
+    } // release lock
+
+    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);
 }
 
 bool PointerChoreographer::setPointerIcon(
diff --git a/services/inputflinger/PointerChoreographer.h b/services/inputflinger/PointerChoreographer.h
index f46038ef73..db1488b546 100644
--- a/services/inputflinger/PointerChoreographer.h
+++ b/services/inputflinger/PointerChoreographer.h
@@ -109,8 +109,10 @@ public:
     void dump(std::string& dump) override;
 
 private:
-    void updatePointerControllersLocked() REQUIRES(mLock);
-    void notifyPointerDisplayIdChangedLocked() REQUIRES(mLock);
+    using PointerDisplayChange =
+            std::optional<std::tuple<int32_t /*displayId*/, FloatPoint /*cursorPosition*/>>;
+    [[nodiscard]] PointerDisplayChange updatePointerControllersLocked() REQUIRES(mLock);
+    [[nodiscard]] PointerDisplayChange calculatePointerDisplayChangeToNotify() REQUIRES(mLock);
     const DisplayViewport* findViewportByIdLocked(int32_t displayId) const REQUIRES(mLock);
     int32_t getTargetMouseDisplayLocked(int32_t associatedDisplayId) const REQUIRES(mLock);
     std::pair<int32_t /*displayId*/, PointerControllerInterface&> ensureMouseControllerLocked(
diff --git a/services/inputflinger/include/PointerChoreographerPolicyInterface.h b/services/inputflinger/include/PointerChoreographerPolicyInterface.h
index 8b47b555e5..462aedc539 100644
--- a/services/inputflinger/include/PointerChoreographerPolicyInterface.h
+++ b/services/inputflinger/include/PointerChoreographerPolicyInterface.h
@@ -25,6 +25,9 @@ namespace android {
  *
  * This is the interface that PointerChoreographer uses to talk to Window Manager and other
  * system components.
+ *
+ * NOTE: In general, the PointerChoreographer must not interact with the policy while
+ * holding any locks.
  */
 class PointerChoreographerPolicyInterface {
 public:
@@ -37,6 +40,9 @@ public:
      * for and runnable on the host, the PointerController implementation must be in a separate
      * library, libinputservice, that has the additional dependencies. The PointerController
      * will be mocked when testing PointerChoreographer.
+     *
+     * Since this is a factory method used to work around dependencies, it will not interact with
+     * other input components and may be called with the PointerChoreographer lock held.
      */
     virtual std::shared_ptr<PointerControllerInterface> createPointerController(
             PointerControllerInterface::ControllerType type) = 0;