Skip to content

Commit 67b101b

Browse files
vishniakouvishniakou
committed
Reland "Reject inconsistent globally injected events" --try 2
This reverts commit f7f93f5. Reason for revert: fixed tests Changes: the test for inconsistent sequence needed to be updated because the injection will now always fail. Original description: The dispatcher currently crashes when certain inconsistent input events are injected. This is affecting test stability negatively. In this CL, we reject globally-injected inconsistent events. That may cause some test flakiness, but should eliminate the crashes due to dispatcher reaching bad state later. Unfortunately, we can't currently reject all inconsistent injected events. In the case of targeted injection, it is common for the caller to leave pointers dangling. Since the injection happens into the caller-owned windows only, at the end of those tests the windows get cleaned up, so the dispatcher is still in a good state. The eventual goal is to completely get rid of injection. Meanwhile, however, this should help avoid at least some of the crashes. Bug: 369935405 Flag: EXEMPT bugfix Test: atest inputflinger_tests Change-Id: Ic778677a63e544feadf01f3cc47b08ec3b207297
1 parent 7db45d5 commit 67b101b

4 files changed

Lines changed: 65 additions & 32 deletions

File tree

‎include/input/InputEventBuilders.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,9 @@ class MotionEventBuilder {
250250
public:
251251
MotionEventBuilder(int32_t action, int32_t source) {
252252
mAction = action;
253+
if (mAction == AMOTION_EVENT_ACTION_CANCEL) {
254+
mFlags |= AMOTION_EVENT_FLAG_CANCELED;
255+
}
253256
mSource = source;
254257
mEventTime = systemTime(SYSTEM_TIME_MONOTONIC);
255258
mDownTime = mEventTime;

‎services/inputflinger/dispatcher/InputDispatcher.cpp‎

Lines changed: 37 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4794,6 +4794,39 @@ void InputDispatcher::notifyPointerCaptureChanged(const NotifyPointerCaptureChan
47944794
}
47954795
}
47964796

4797+
bool InputDispatcher::shouldRejectInjectedMotionLocked(const MotionEvent& motionEvent,
4798+
DeviceId deviceId,
4799+
ui::LogicalDisplayId displayId,
4800+
std::optional<gui::Uid> targetUid,
4801+
int32_t flags) {
4802+
// Don't verify targeted injection, since it will only affect the caller's
4803+
// window, and the windows are typically destroyed at the end of the test.
4804+
if (targetUid.has_value()) {
4805+
return false;
4806+
}
4807+
4808+
// Verify all other injected streams, whether the injection is coming from apps or from
4809+
// input filter. Print an error if the stream becomes inconsistent with this event.
4810+
// An inconsistent injected event sent could cause a crash in the later stages of
4811+
// dispatching pipeline.
4812+
auto [it, _] = mInputFilterVerifiersByDisplay.try_emplace(displayId,
4813+
std::string("Injection on ") +
4814+
displayId.toString());
4815+
InputVerifier& verifier = it->second;
4816+
4817+
Result<void> result =
4818+
verifier.processMovement(deviceId, motionEvent.getSource(), motionEvent.getAction(),
4819+
motionEvent.getPointerCount(),
4820+
motionEvent.getPointerProperties(),
4821+
motionEvent.getSamplePointerCoords(), flags);
4822+
if (!result.ok()) {
4823+
logDispatchStateLocked();
4824+
LOG(ERROR) << "Inconsistent event: " << motionEvent << ", reason: " << result.error();
4825+
return true;
4826+
}
4827+
return false;
4828+
}
4829+
47974830
InputEventInjectionResult InputDispatcher::injectInputEvent(const InputEvent* event,
47984831
std::optional<gui::Uid> targetUid,
47994832
InputEventInjectionSync syncMode,
@@ -4904,32 +4937,10 @@ InputEventInjectionResult InputDispatcher::injectInputEvent(const InputEvent* ev
49044937

49054938
mLock.lock();
49064939

4907-
{
4908-
// Verify all injected streams, whether the injection is coming from apps or from
4909-
// input filter. Print an error if the stream becomes inconsistent with this event.
4910-
// An inconsistent injected event sent could cause a crash in the later stages of
4911-
// dispatching pipeline.
4912-
auto [it, _] =
4913-
mInputFilterVerifiersByDisplay.try_emplace(displayId,
4914-
std::string("Injection on ") +
4915-
displayId.toString());
4916-
InputVerifier& verifier = it->second;
4917-
4918-
Result<void> result =
4919-
verifier.processMovement(resolvedDeviceId, motionEvent.getSource(),
4920-
motionEvent.getAction(),
4921-
motionEvent.getPointerCount(),
4922-
motionEvent.getPointerProperties(),
4923-
motionEvent.getSamplePointerCoords(), flags);
4924-
if (!result.ok()) {
4925-
logDispatchStateLocked();
4926-
LOG(ERROR) << "Inconsistent event: " << motionEvent
4927-
<< ", reason: " << result.error();
4928-
if (policyFlags & POLICY_FLAG_INJECTED_FROM_ACCESSIBILITY) {
4929-
mLock.unlock();
4930-
return InputEventInjectionResult::FAILED;
4931-
}
4932-
}
4940+
if (shouldRejectInjectedMotionLocked(motionEvent, resolvedDeviceId, displayId,
4941+
targetUid, flags)) {
4942+
mLock.unlock();
4943+
return InputEventInjectionResult::FAILED;
49334944
}
49344945

49354946
const nsecs_t* sampleEventTimes = motionEvent.getSampleEventTimes();

‎services/inputflinger/dispatcher/InputDispatcher.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,10 @@ class InputDispatcher : public android::InputDispatcherInterface {
299299

300300
// Event injection and synchronization.
301301
std::condition_variable mInjectionResultAvailable;
302+
bool shouldRejectInjectedMotionLocked(const MotionEvent& motion, DeviceId deviceId,
303+
ui::LogicalDisplayId displayId,
304+
std::optional<gui::Uid> targetUid, int32_t flags)
305+
REQUIRES(mLock);
302306
void setInjectionResult(const EventEntry& entry,
303307
android::os::InputEventInjectionResult injectionResult);
304308
void transformMotionEntryForInjectionLocked(MotionEntry&,

‎services/inputflinger/tests/InputDispatcher_test.cpp‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5105,9 +5105,7 @@ TEST_F(InputDispatcherTest, HoverExitIsSentToRemovedWindow) {
51055105
/**
51065106
* Test that invalid HOVER events sent by accessibility do not cause a fatal crash.
51075107
*/
5108-
TEST_F_WITH_FLAGS(InputDispatcherTest, InvalidA11yHoverStreamDoesNotCrash,
5109-
REQUIRES_FLAGS_DISABLED(ACONFIG_FLAG(com::android::input::flags,
5110-
a11y_crash_on_inconsistent_event_stream))) {
5108+
TEST_F(InputDispatcherTest, InvalidA11yHoverStreamDoesNotCrash) {
51115109
std::shared_ptr<FakeApplicationHandle> application = std::make_shared<FakeApplicationHandle>();
51125110
sp<FakeWindowHandle> window = sp<FakeWindowHandle>::make(application, mDispatcher, "Window",
51135111
ui::LogicalDisplayId::DEFAULT);
@@ -5123,10 +5121,11 @@ TEST_F_WITH_FLAGS(InputDispatcherTest, InvalidA11yHoverStreamDoesNotCrash,
51235121
.addFlag(AMOTION_EVENT_FLAG_IS_ACCESSIBILITY_EVENT);
51245122
ASSERT_EQ(InputEventInjectionResult::SUCCEEDED,
51255123
injectMotionEvent(*mDispatcher, hoverEnterBuilder.build()));
5126-
ASSERT_EQ(InputEventInjectionResult::SUCCEEDED,
5127-
injectMotionEvent(*mDispatcher, hoverEnterBuilder.build()));
5128-
window->consumeMotionEvent(WithMotionAction(AMOTION_EVENT_ACTION_HOVER_ENTER));
51295124
window->consumeMotionEvent(WithMotionAction(AMOTION_EVENT_ACTION_HOVER_ENTER));
5125+
// Another HOVER_ENTER would be inconsistent, and should therefore fail to
5126+
// get injected.
5127+
ASSERT_EQ(InputEventInjectionResult::FAILED,
5128+
injectMotionEvent(*mDispatcher, hoverEnterBuilder.build()));
51305129
}
51315130

51325131
/**
@@ -12889,6 +12888,22 @@ TEST_F(InputDispatcherDragTests, DragAndDropFinishedWhenCancelCurrentTouch) {
1288912888
// Remove drag window
1289012889
mDispatcher->onWindowInfosChanged({{*mWindow->getInfo(), *mSecondWindow->getInfo()}, {}, 0, 0});
1289112890

12891+
// Complete the first event stream, even though the injection will fail because there aren't any
12892+
// valid targets to dispatch this event to. This is still needed to make the input stream
12893+
// consistent
12894+
ASSERT_EQ(InputEventInjectionResult::FAILED,
12895+
injectMotionEvent(*mDispatcher,
12896+
MotionEventBuilder(ACTION_CANCEL, AINPUT_SOURCE_TOUCHSCREEN)
12897+
.displayId(ui::LogicalDisplayId::DEFAULT)
12898+
.pointer(PointerBuilder(/*id=*/0, ToolType::FINGER)
12899+
.x(150)
12900+
.y(50))
12901+
.pointer(PointerBuilder(/*id=*/1, ToolType::FINGER)
12902+
.x(50)
12903+
.y(50))
12904+
.build(),
12905+
INJECT_EVENT_TIMEOUT, InputEventInjectionSync::WAIT_FOR_RESULT));
12906+
1289212907
// Inject a simple gesture, ensure dispatcher not crashed
1289312908
ASSERT_EQ(InputEventInjectionResult::SUCCEEDED,
1289412909
injectMotionDown(*mDispatcher, AINPUT_SOURCE_TOUCHSCREEN,

0 commit comments

Comments
 (0)