From 5693cee2f91e72ac2cbbb958cbb7cd6b58a479f3 Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Fri, 31 Dec 2021 06:51:15 -0800 Subject: Allow its WindowInfosListener to outlive PointerController A strong pointer to PointerController::DisplayInfoListener is given away when the listener is registered, so PC cannot guarantee that the listener is destroyed when it is destroyed. This means the listener can outlive PC, so there is a race condition between PC's destruction and an update to the listener. While it could be argued that it is the caller's responsibility to ensure that the listener is not updated after it is unregistered, there is no way to guarantee that using strong pointers. Here, we can be defensive and protect against this case. Bug: 212672261 Test: atest libinputservice_test Change-Id: I358a725980cc8c7d6ad0483a9b2a8b8715a03424 --- libs/input/PointerController.cpp | 78 ++++++++++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 23 deletions(-) (limited to 'libs/input/PointerController.cpp') diff --git a/libs/input/PointerController.cpp b/libs/input/PointerController.cpp index f43586f8d9d0..1dc74e5f7740 100644 --- a/libs/input/PointerController.cpp +++ b/libs/input/PointerController.cpp @@ -18,11 +18,13 @@ //#define LOG_NDEBUG 0 #include "PointerController.h" -#include "PointerControllerContext.h" #include #include #include +#include + +#include "PointerControllerContext.h" namespace android { @@ -36,8 +38,18 @@ const ui::Transform kIdentityTransform; void PointerController::DisplayInfoListener::onWindowInfosChanged( const std::vector&, - const std::vector& displayInfo) { - mPointerController.onDisplayInfosChanged(displayInfo); + const std::vector& displayInfos) { + std::scoped_lock lock(mLock); + if (mPointerController == nullptr) return; + + // PointerController uses DisplayInfoListener's lock. + base::ScopedLockAssertion assumeLocked(mPointerController->getLock()); + mPointerController->onDisplayInfosChangedLocked(displayInfos); +} + +void PointerController::DisplayInfoListener::onPointerControllerDestroyed() { + std::scoped_lock lock(mLock); + mPointerController = nullptr; } // --- PointerController --- @@ -68,16 +80,36 @@ std::shared_ptr PointerController::create( PointerController::PointerController(const sp& policy, const sp& looper, const sp& spriteController) + : PointerController( + policy, looper, spriteController, + [](const sp& listener) { + SurfaceComposerClient::getDefault()->addWindowInfosListener(listener); + }, + [](const sp& listener) { + SurfaceComposerClient::getDefault()->removeWindowInfosListener(listener); + }) {} + +PointerController::PointerController(const sp& policy, + const sp& looper, + const sp& spriteController, + WindowListenerConsumer registerListener, + WindowListenerConsumer unregisterListener) : mContext(policy, looper, spriteController, *this), mCursorController(mContext), - mDisplayInfoListener(new DisplayInfoListener(*this)) { - std::scoped_lock lock(mLock); + mDisplayInfoListener(new DisplayInfoListener(this)), + mUnregisterWindowInfosListener(std::move(unregisterListener)) { + std::scoped_lock lock(getLock()); mLocked.presentation = Presentation::SPOT; - SurfaceComposerClient::getDefault()->addWindowInfosListener(mDisplayInfoListener); + registerListener(mDisplayInfoListener); } PointerController::~PointerController() { - SurfaceComposerClient::getDefault()->removeWindowInfosListener(mDisplayInfoListener); + mDisplayInfoListener->onPointerControllerDestroyed(); + mUnregisterWindowInfosListener(mDisplayInfoListener); +} + +std::mutex& PointerController::getLock() const { + return mDisplayInfoListener->mLock; } bool PointerController::getBounds(float* outMinX, float* outMinY, float* outMaxX, @@ -89,7 +121,7 @@ void PointerController::move(float deltaX, float deltaY) { const int32_t displayId = mCursorController.getDisplayId(); vec2 transformed; { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); const auto& transform = getTransformForDisplayLocked(displayId); transformed = transformWithoutTranslation(transform, {deltaX, deltaY}); } @@ -108,7 +140,7 @@ void PointerController::setPosition(float x, float y) { const int32_t displayId = mCursorController.getDisplayId(); vec2 transformed; { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); const auto& transform = getTransformForDisplayLocked(displayId); transformed = transform.transform(x, y); } @@ -119,7 +151,7 @@ void PointerController::getPosition(float* outX, float* outY) const { const int32_t displayId = mCursorController.getDisplayId(); mCursorController.getPosition(outX, outY); { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); const auto& transform = getTransformForDisplayLocked(displayId); const auto xy = transform.inverse().transform(*outX, *outY); *outX = xy.x; @@ -132,17 +164,17 @@ int32_t PointerController::getDisplayId() const { } void PointerController::fade(Transition transition) { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); mCursorController.fade(transition); } void PointerController::unfade(Transition transition) { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); mCursorController.unfade(transition); } void PointerController::setPresentation(Presentation presentation) { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); if (mLocked.presentation == presentation) { return; @@ -162,7 +194,7 @@ void PointerController::setPresentation(Presentation presentation) { void PointerController::setSpots(const PointerCoords* spotCoords, const uint32_t* spotIdToIndex, BitSet32 spotIdBits, int32_t displayId) { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); std::array outSpotCoords{}; const ui::Transform& transform = getTransformForDisplayLocked(displayId); @@ -185,11 +217,11 @@ void PointerController::setSpots(const PointerCoords* spotCoords, const uint32_t } void PointerController::clearSpots() { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); clearSpotsLocked(); } -void PointerController::clearSpotsLocked() REQUIRES(mLock) { +void PointerController::clearSpotsLocked() { for (auto& [displayID, spotController] : mLocked.spotControllers) { spotController.clearSpots(); } @@ -200,7 +232,7 @@ void PointerController::setInactivityTimeout(InactivityTimeout inactivityTimeout } void PointerController::reloadPointerResources() { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); for (auto& [displayID, spotController] : mLocked.spotControllers) { spotController.reloadSpotResources(); @@ -216,7 +248,7 @@ void PointerController::reloadPointerResources() { } void PointerController::setDisplayViewport(const DisplayViewport& viewport) { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); bool getAdditionalMouseResources = false; if (mLocked.presentation == PointerController::Presentation::POINTER) { @@ -226,12 +258,12 @@ void PointerController::setDisplayViewport(const DisplayViewport& viewport) { } void PointerController::updatePointerIcon(int32_t iconId) { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); mCursorController.updatePointerIcon(iconId); } void PointerController::setCustomPointerIcon(const SpriteIcon& icon) { - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); mCursorController.setCustomPointerIcon(icon); } @@ -245,7 +277,7 @@ void PointerController::onDisplayViewportsUpdated(std::vector& displayIdSet.insert(viewport.displayId); } - std::scoped_lock lock(mLock); + std::scoped_lock lock(getLock()); for (auto it = mLocked.spotControllers.begin(); it != mLocked.spotControllers.end();) { int32_t displayID = it->first; if (!displayIdSet.count(displayID)) { @@ -261,8 +293,8 @@ void PointerController::onDisplayViewportsUpdated(std::vector& } } -void PointerController::onDisplayInfosChanged(const std::vector& displayInfo) { - std::scoped_lock lock(mLock); +void PointerController::onDisplayInfosChangedLocked( + const std::vector& displayInfo) { mLocked.mDisplayInfos = displayInfo; } -- cgit v1.2.3-59-g8ed1b