Remove callback at the end of consumer destructor
It turns out that 'finishInputEvent' was adding the fd back to the
looper. This caused an NPE because we were calling 'finishInputEvent' at
the end of the consumer destructor, thus leaving the callback in the
looper. The looper would hit an NPE whenever the fd signaled after that.
To fix this, we move the call to setFdEvents(0) to the very end of the
destructor. This way, we can be sure that the last thing that executes
is the removal of the fd from the Looper.
There is also a possibility that fd is signaled during the destructor
execution. Theoretically, this should be okay because the fd callbacks
are processed on the same thread as the one where destructor runs.
Therefore, the signals would only be processed after the destructor has
completed, which means the callback would be removed before it gets the
chance to execute.
Bug: 332613662
Test: TEST=libinput_tests; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST --gtest_filter="*InputConsumerTest*"
Test: TEST=libutils_test; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST
Flag: EXEMPT bugfix
Change-Id: If5ac7a8eaf96e842d5d8e44008b9c1bff74e674e
diff --git a/libs/input/tests/InputConsumer_test.cpp b/libs/input/tests/InputConsumer_test.cpp
index 6a3bbe5..b32c2cb 100644
--- a/libs/input/tests/InputConsumer_test.cpp
+++ b/libs/input/tests/InputConsumer_test.cpp
@@ -70,11 +70,20 @@
[]() { return std::make_unique<LegacyResampler>(); });
}
- void invokeLooperCallback() const {
+ bool invokeLooperCallback() const {
sp<LooperCallback> callback;
- ASSERT_TRUE(mLooper->getFdStateDebug(mClientTestChannel->getFd(), /*ident=*/nullptr,
- /*events=*/nullptr, &callback, /*data=*/nullptr));
+ const bool found =
+ mLooper->getFdStateDebug(mClientTestChannel->getFd(), /*ident=*/nullptr,
+ /*events=*/nullptr, &callback, /*data=*/nullptr);
+ if (!found) {
+ return false;
+ }
+ if (callback == nullptr) {
+ LOG(FATAL) << "Looper has the fd of interest, but the callback is null!";
+ return false;
+ }
callback->handleEvent(mClientTestChannel->getFd(), ALOOPER_EVENT_INPUT, /*data=*/nullptr);
+ return true;
}
void assertOnBatchedInputEventPendingWasCalled() {
@@ -271,6 +280,27 @@
}
/**
+ * Check what happens when looper invokes callback after consumer has been destroyed.
+ * This reproduces a crash where the LooperEventCallback was added back to the Looper during
+ * destructor, thus allowing the looper callback to be invoked onto a null consumer object.
+ */
+TEST_F(InputConsumerTest, LooperCallbackInvokedAfterConsumerDestroyed) {
+ mClientTestChannel->enqueueMessage(
+ InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/0}.action(ACTION_DOWN).build());
+ mClientTestChannel->enqueueMessage(
+ InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/1}.action(ACTION_MOVE).build());
+ ASSERT_TRUE(invokeLooperCallback());
+ assertOnBatchedInputEventPendingWasCalled();
+ assertReceivedMotionEvent(WithMotionAction(ACTION_DOWN));
+ mClientTestChannel->assertFinishMessage(/*seq=*/0, /*handled=*/true);
+
+ // Now, destroy the consumer and invoke the looper callback again after it's been destroyed.
+ mConsumer.reset();
+ mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/false);
+ ASSERT_FALSE(invokeLooperCallback());
+}
+
+/**
* Send an event to the InputConsumer, but do not invoke "consumeBatchedInputEvents", thus leaving
* the input event unconsumed by the callbacks. Ensure that no crash occurs when the consumer is
* destroyed.