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

Commit 5a51a228 authored by Prabir Pradhan's avatar Prabir Pradhan
Browse files

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
Change-Id: Ib81d72898a212275d95f9d84d89a16e7172e108e
parent 990d8713

services/inputflinger/PointerChoreographer.cpp

+81 −40
Original line number Diff line number Diff line
@@ -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) {

    PointerDisplayChange pointerDisplayChange;



    { // acquire lock

        std::scoped_lock _l(mLock);



        mInputDeviceInfos = args.inputDeviceInfos;

    updatePointerControllersLocked();

        pointerDisplayChange = updatePointerControllersLocked();

    } // release lock



    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,22 +412,30 @@ 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) {

    PointerDisplayChange pointerDisplayChange;



    { // acquire lock

        std::scoped_lock _l(mLock);



        mDefaultMouseDisplayId = displayId;

    updatePointerControllersLocked();

        pointerDisplayChange = updatePointerControllersLocked();

    } // release lock



    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);

}



void PointerChoreographer::setDisplayViewports(const std::vector<DisplayViewport>& viewports) {

    PointerDisplayChange pointerDisplayChange;



    { // acquire lock

        std::scoped_lock _l(mLock);

        for (const auto& viewport : viewports) {

            const int32_t displayId = viewport.displayId;
@@ -425,7 +451,10 @@ void PointerChoreographer::setDisplayViewports(const std::vector<DisplayViewport 
            }

        }

        mViewports = viewports;

    notifyPointerDisplayIdChangedLocked();

        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) {

    PointerDisplayChange pointerDisplayChange;



    { // acquire lock

        std::scoped_lock _l(mLock);

        if (mShowTouchesEnabled == enabled) {

            return;

        }

        mShowTouchesEnabled = enabled;

    updatePointerControllersLocked();

        pointerDisplayChange = updatePointerControllersLocked();

    } // release lock



    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);

}



void PointerChoreographer::setStylusPointerIconEnabled(bool enabled) {

    PointerDisplayChange pointerDisplayChange;



    { // acquire lock

        std::scoped_lock _l(mLock);

        if (mStylusPointerIconEnabled == enabled) {

            return;

        }

        mStylusPointerIconEnabled = enabled;

    updatePointerControllersLocked();

        pointerDisplayChange = updatePointerControllersLocked();

    } // release lock



    notifyPointerDisplayChange(pointerDisplayChange, mPolicy);

}



bool PointerChoreographer::setPointerIcon(

services/inputflinger/PointerChoreographer.h

+4 −2
Original line number Diff line number Diff line
@@ -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(

services/inputflinger/include/PointerChoreographerPolicyInterface.h

+6 −0
Original line number Diff line number Diff line
@@ -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;