Refactor PointerChoreographer lock
Pointer choreographer and WindowInfoListener use separate locks which
is problematic. As WindowInfosUpdate callback is triggered from the
display thread, a deadlock might occur due to race between display and
reader thread. Both trying to acquire both the locks.
This becomes much more likely when we have multiple displays connected.
Test: Verify pointer indicators are hidden on screenshots of secure windows
Test: atest inputflinger_tests
Bug: 367660694
Flag: EXEMPT refactor
Change-Id: I7417c0071322a093643734432f8f0236bd89a317
diff --git a/services/inputflinger/PointerChoreographer.cpp b/services/inputflinger/PointerChoreographer.cpp
index 006d507..a67df58 100644
--- a/services/inputflinger/PointerChoreographer.cpp
+++ b/services/inputflinger/PointerChoreographer.cpp
@@ -139,23 +139,24 @@
mShowTouchesEnabled(false),
mStylusPointerIconEnabled(false),
mCurrentFocusedDisplay(ui::LogicalDisplayId::DEFAULT),
+ mIsWindowInfoListenerRegistered(false),
+ mWindowInfoListener(sp<PointerChoreographerDisplayInfoListener>::make(this)),
mRegisterListener(registerListener),
mUnregisterListener(unregisterListener) {}
PointerChoreographer::~PointerChoreographer() {
- std::scoped_lock _l(mLock);
- if (mWindowInfoListener == nullptr) {
- return;
+ if (mIsWindowInfoListenerRegistered) {
+ mUnregisterListener(mWindowInfoListener);
+ mIsWindowInfoListenerRegistered = false;
}
mWindowInfoListener->onPointerChoreographerDestroyed();
- mUnregisterListener(mWindowInfoListener);
}
void PointerChoreographer::notifyInputDevicesChanged(const NotifyInputDevicesChangedArgs& args) {
PointerDisplayChange pointerDisplayChange;
{ // acquire lock
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
mInputDeviceInfos = args.inputDeviceInfos;
pointerDisplayChange = updatePointerControllersLocked();
@@ -191,7 +192,7 @@
return;
}
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
ui::LogicalDisplayId targetDisplay = args.displayId;
if (targetDisplay == ui::LogicalDisplayId::INVALID) {
targetDisplay = mCurrentFocusedDisplay;
@@ -204,7 +205,7 @@
}
NotifyMotionArgs PointerChoreographer::processMotion(const NotifyMotionArgs& args) {
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
if (isFromMouse(args)) {
return processMouseEventLocked(args);
@@ -433,7 +434,7 @@
}
void PointerChoreographer::processDeviceReset(const NotifyDeviceResetArgs& args) {
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
mTouchPointersByDevice.erase(args.deviceId);
mStylusPointersByDevice.erase(args.deviceId);
mDrawingTabletPointersByDevice.erase(args.deviceId);
@@ -447,17 +448,22 @@
bool requireListener = !mTouchPointersByDevice.empty() || !mMousePointersByDisplay.empty() ||
!mDrawingTabletPointersByDevice.empty() || !mStylusPointersByDevice.empty();
- if (requireListener && mWindowInfoListener == nullptr) {
- mWindowInfoListener = sp<PointerChoreographerDisplayInfoListener>::make(this);
- mWindowInfoListener->setInitialDisplayInfos(mRegisterListener(mWindowInfoListener));
- onPrivacySensitiveDisplaysChangedLocked(mWindowInfoListener->getPrivacySensitiveDisplays());
- } else if (!requireListener && mWindowInfoListener != nullptr) {
+ // PointerChoreographer uses Listener's lock which is already held by caller
+ base::ScopedLockAssertion assumeLocked(mWindowInfoListener->mLock);
+
+ if (requireListener && !mIsWindowInfoListenerRegistered) {
+ mIsWindowInfoListenerRegistered = true;
+ mWindowInfoListener->setInitialDisplayInfosLocked(mRegisterListener(mWindowInfoListener));
+ onPrivacySensitiveDisplaysChangedLocked(
+ mWindowInfoListener->getPrivacySensitiveDisplaysLocked());
+ } else if (!requireListener && mIsWindowInfoListenerRegistered) {
+ mIsWindowInfoListenerRegistered = false;
mUnregisterListener(mWindowInfoListener);
- mWindowInfoListener = nullptr;
- } else if (requireListener && mWindowInfoListener != nullptr) {
+ } else if (requireListener) {
// controller may have been added to an existing privacy sensitive display, we need to
// update all controllers again
- onPrivacySensitiveDisplaysChangedLocked(mWindowInfoListener->getPrivacySensitiveDisplays());
+ onPrivacySensitiveDisplaysChangedLocked(
+ mWindowInfoListener->getPrivacySensitiveDisplaysLocked());
}
}
@@ -494,7 +500,7 @@
void PointerChoreographer::notifyPointerCaptureChanged(
const NotifyPointerCaptureChangedArgs& args) {
if (args.request.isEnable()) {
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
for (const auto& [_, mousePointerController] : mMousePointersByDisplay) {
mousePointerController->fade(PointerControllerInterface::Transition::IMMEDIATE);
}
@@ -502,14 +508,8 @@
mNextListener.notify(args);
}
-void PointerChoreographer::onPrivacySensitiveDisplaysChanged(
- const std::unordered_set<ui::LogicalDisplayId>& privacySensitiveDisplays) {
- std::scoped_lock _l(mLock);
- onPrivacySensitiveDisplaysChangedLocked(privacySensitiveDisplays);
-}
-
void PointerChoreographer::dump(std::string& dump) {
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
dump += "PointerChoreographer:\n";
dump += StringPrintf(INDENT "Show Touches Enabled: %s\n",
@@ -579,6 +579,10 @@
return mDisplaysWithPointersHidden.find(displayId) == mDisplaysWithPointersHidden.end();
}
+std::mutex& PointerChoreographer::getLock() const {
+ return mWindowInfoListener->mLock;
+}
+
PointerChoreographer::PointerDisplayChange PointerChoreographer::updatePointerControllersLocked() {
std::set<ui::LogicalDisplayId /*displayId*/> mouseDisplaysToKeep;
std::set<DeviceId> touchDevicesToKeep;
@@ -641,7 +645,7 @@
std::erase_if(mDrawingTabletPointersByDevice, [&drawingTabletDevicesToKeep](const auto& pair) {
return drawingTabletDevicesToKeep.find(pair.first) == drawingTabletDevicesToKeep.end();
});
- std::erase_if(mMouseDevices, [&](DeviceId id) REQUIRES(mLock) {
+ std::erase_if(mMouseDevices, [&](DeviceId id) REQUIRES(getLock()) {
return std::find_if(mInputDeviceInfos.begin(), mInputDeviceInfos.end(),
[id](const auto& info) { return info.getId() == id; }) ==
mInputDeviceInfos.end();
@@ -677,7 +681,7 @@
PointerDisplayChange pointerDisplayChange;
{ // acquire lock
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
mDefaultMouseDisplayId = displayId;
pointerDisplayChange = updatePointerControllersLocked();
@@ -690,7 +694,7 @@
PointerDisplayChange pointerDisplayChange;
{ // acquire lock
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
for (const auto& viewport : viewports) {
const ui::LogicalDisplayId displayId = viewport.displayId;
if (const auto it = mMousePointersByDisplay.find(displayId);
@@ -719,7 +723,7 @@
std::optional<DisplayViewport> PointerChoreographer::getViewportForPointerDevice(
ui::LogicalDisplayId associatedDisplayId) {
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
const ui::LogicalDisplayId resolvedDisplayId = getTargetMouseDisplayLocked(associatedDisplayId);
if (const auto viewport = findViewportByIdLocked(resolvedDisplayId); viewport) {
return *viewport;
@@ -728,7 +732,7 @@
}
FloatPoint PointerChoreographer::getMouseCursorPosition(ui::LogicalDisplayId displayId) {
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
const ui::LogicalDisplayId resolvedDisplayId = getTargetMouseDisplayLocked(displayId);
if (auto it = mMousePointersByDisplay.find(resolvedDisplayId);
it != mMousePointersByDisplay.end()) {
@@ -741,7 +745,7 @@
PointerDisplayChange pointerDisplayChange;
{ // acquire lock
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
if (mShowTouchesEnabled == enabled) {
return;
}
@@ -756,7 +760,7 @@
PointerDisplayChange pointerDisplayChange;
{ // acquire lock
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
if (mStylusPointerIconEnabled == enabled) {
return;
}
@@ -770,7 +774,7 @@
bool PointerChoreographer::setPointerIcon(
std::variant<std::unique_ptr<SpriteIcon>, PointerIconStyle> icon,
ui::LogicalDisplayId displayId, DeviceId deviceId) {
- std::scoped_lock _l(mLock);
+ std::scoped_lock _l(getLock());
if (deviceId < 0) {
LOG(WARNING) << "Invalid device id " << deviceId << ". Cannot set pointer icon.";
return false;
@@ -821,7 +825,7 @@
}
void PointerChoreographer::setPointerIconVisibility(ui::LogicalDisplayId displayId, bool visible) {
- std::scoped_lock lock(mLock);
+ std::scoped_lock lock(getLock());
if (visible) {
mDisplaysWithPointersHidden.erase(displayId);
// We do not unfade the icons here, because we don't know when the last event happened.
@@ -843,14 +847,14 @@
}
void PointerChoreographer::setFocusedDisplay(ui::LogicalDisplayId displayId) {
- std::scoped_lock lock(mLock);
+ std::scoped_lock lock(getLock());
mCurrentFocusedDisplay = displayId;
}
PointerChoreographer::ControllerConstructor PointerChoreographer::getMouseControllerConstructor(
ui::LogicalDisplayId displayId) {
std::function<std::shared_ptr<PointerControllerInterface>()> ctor =
- [this, displayId]() REQUIRES(mLock) {
+ [this, displayId]() REQUIRES(getLock()) {
auto pc = mPolicy.createPointerController(
PointerControllerInterface::ControllerType::MOUSE);
if (const auto viewport = findViewportByIdLocked(displayId); viewport) {
@@ -864,7 +868,7 @@
PointerChoreographer::ControllerConstructor PointerChoreographer::getStylusControllerConstructor(
ui::LogicalDisplayId displayId) {
std::function<std::shared_ptr<PointerControllerInterface>()> ctor =
- [this, displayId]() REQUIRES(mLock) {
+ [this, displayId]() REQUIRES(getLock()) {
auto pc = mPolicy.createPointerController(
PointerControllerInterface::ControllerType::STYLUS);
if (const auto viewport = findViewportByIdLocked(displayId); viewport) {
@@ -875,9 +879,11 @@
return ConstructorDelegate(std::move(ctor));
}
+// --- PointerChoreographer::PointerChoreographerDisplayInfoListener ---
+
void PointerChoreographer::PointerChoreographerDisplayInfoListener::onWindowInfosChanged(
const gui::WindowInfosUpdate& windowInfosUpdate) {
- std::scoped_lock _l(mListenerLock);
+ std::scoped_lock _l(mLock);
if (mPointerChoreographer == nullptr) {
return;
}
@@ -885,25 +891,25 @@
getPrivacySensitiveDisplaysFromWindowInfos(windowInfosUpdate.windowInfos);
if (newPrivacySensitiveDisplays != mPrivacySensitiveDisplays) {
mPrivacySensitiveDisplays = std::move(newPrivacySensitiveDisplays);
- mPointerChoreographer->onPrivacySensitiveDisplaysChanged(mPrivacySensitiveDisplays);
+ // PointerChoreographer uses Listener's lock.
+ base::ScopedLockAssertion assumeLocked(mPointerChoreographer->getLock());
+ mPointerChoreographer->onPrivacySensitiveDisplaysChangedLocked(mPrivacySensitiveDisplays);
}
}
-void PointerChoreographer::PointerChoreographerDisplayInfoListener::setInitialDisplayInfos(
+void PointerChoreographer::PointerChoreographerDisplayInfoListener::setInitialDisplayInfosLocked(
const std::vector<gui::WindowInfo>& windowInfos) {
- std::scoped_lock _l(mListenerLock);
mPrivacySensitiveDisplays = getPrivacySensitiveDisplaysFromWindowInfos(windowInfos);
}
std::unordered_set<ui::LogicalDisplayId /*displayId*/>
-PointerChoreographer::PointerChoreographerDisplayInfoListener::getPrivacySensitiveDisplays() {
- std::scoped_lock _l(mListenerLock);
+PointerChoreographer::PointerChoreographerDisplayInfoListener::getPrivacySensitiveDisplaysLocked() {
return mPrivacySensitiveDisplays;
}
void PointerChoreographer::PointerChoreographerDisplayInfoListener::
onPointerChoreographerDestroyed() {
- std::scoped_lock _l(mListenerLock);
+ std::scoped_lock _l(mLock);
mPointerChoreographer = nullptr;
}