summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
author Siarhei Vishniakou <svv@google.com> 2024-10-17 14:19:52 -0700
committer Siarhei Vishniakou <svv@google.com> 2024-10-22 09:10:41 -0700
commit362fa2273d66fae4af1ec38cac32c0ad22e1acf5 (patch)
treed3d92e8616564034f05e988f6ea629dd658d2741
parent1c375bb6207dc32d418868bca8b3173b66847c3d (diff)
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
-rw-r--r--include/input/InputConsumerNoResampling.h2
-rw-r--r--libs/input/InputConsumerNoResampling.cpp17
-rw-r--r--libs/input/tests/InputConsumer_test.cpp36
3 files changed, 44 insertions, 11 deletions
diff --git a/include/input/InputConsumerNoResampling.h b/include/input/InputConsumerNoResampling.h
index 228347d818..2d1714c70a 100644
--- a/include/input/InputConsumerNoResampling.h
+++ b/include/input/InputConsumerNoResampling.h
@@ -141,7 +141,7 @@ private:
}
private:
- std::function<int(int events)> mCallback;
+ const std::function<int(int events)> mCallback;
};
sp<LooperEventCallback> mCallback;
/**
diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp
index 9665de799f..4cb6bf7bb4 100644
--- a/libs/input/InputConsumerNoResampling.cpp
+++ b/libs/input/InputConsumerNoResampling.cpp
@@ -193,13 +193,6 @@ InputConsumerNoResampling::InputConsumerNoResampling(
InputConsumerNoResampling::~InputConsumerNoResampling() {
ensureCalledOnLooperThread(__func__);
- while (!mOutboundQueue.empty()) {
- processOutboundEvents();
- // This is our last chance to ack the events. If we don't ack them here, we will get an ANR,
- // so keep trying to send the events as long as they are present in the queue.
- }
-
- setFdEvents(0);
// If there are any remaining unread batches, send an ack for them and don't deliver
// them to callbacks.
for (auto& [_, batches] : mBatches) {
@@ -208,6 +201,12 @@ InputConsumerNoResampling::~InputConsumerNoResampling() {
batches.pop();
}
}
+
+ while (!mOutboundQueue.empty()) {
+ processOutboundEvents();
+ // This is our last chance to ack the events. If we don't ack them here, we will get an ANR,
+ // so keep trying to send the events as long as they are present in the queue.
+ }
// However, it is still up to the app to finish any events that have already been delivered
// to the callbacks. If we wanted to change that behaviour and auto-finish all unfinished events
// that were already sent to callbacks, we could potentially loop through "mConsumeTimes"
@@ -216,6 +215,10 @@ InputConsumerNoResampling::~InputConsumerNoResampling() {
const size_t unfinishedEvents = mConsumeTimes.size();
LOG_IF(INFO, unfinishedEvents != 0)
<< getName() << " has " << unfinishedEvents << " unfinished event(s)";
+ // Remove the fd from epoll, so that Looper does not call 'handleReceiveCallback' anymore.
+ // This must be done at the end of the destructor; otherwise, some of the other functions may
+ // call 'setFdEvents' as a side-effect, thus adding the fd back to the epoll set of the looper.
+ setFdEvents(0);
}
int InputConsumerNoResampling::handleReceiveCallback(int events) {
diff --git a/libs/input/tests/InputConsumer_test.cpp b/libs/input/tests/InputConsumer_test.cpp
index 6a3bbe5b36..b32c2cb655 100644
--- a/libs/input/tests/InputConsumer_test.cpp
+++ b/libs/input/tests/InputConsumer_test.cpp
@@ -70,11 +70,20 @@ protected:
[]() { 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 @@ TEST_F(InputConsumerTest, UnhandledEventsNotFinishedInDestructor) {
}
/**
+ * 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.