Do not invoke callbacks when InputConsumer is destroyed
Before this CL, the InputConsumer would try to finish all of the pending
events that haven't been received by the app by forcefully consuming
everything in the destructor.
Unfortunately, this leads to the following crash: the callbacks (app) is
storing the consumer inside unique_ptr. When the destructor of consumer
runs, the unique_ptr will first assign the raw pointer to nullptr and
then run the destructor. Next, inside the destructor, the callbacks are
invoked, and the app may try to finish an input event that it receives.
However, to finish, app needs to call back into the consumer, which is
being destroyed. Since the only way the app can talk to the consumer is
via unique_ptr, that pointer has already been set to nullptr, and this
causes an NPE.
To fix this, we will no longer be invoking the callbacks when the
consumer destructor is invoked (this is done in this CL).
The logic is as follows:
- All events that have already been delivered to the app should still be
finished by the app (this piece is debatable. we can fix this later if
this becomes a problem).
- Events that are batched, and not yet delivered to the app, will be
automatically finished by the consumer, without sending them to the
app. Since the app is destroying the consumer, it's unlikely that it
cares about handling these events, anyways.
If we want to finish _all_ pending messages (the messages that app
hasn't yet called "finish" on), then we should probably iterate through
mConsumeTimes, which is the only struct that keeps track of _all_ events
that have been sent to the app, and not yet finished. However, to do
this, we would need to rename this data structure, since it will no
longer be solely used for keeping track of consume times, but also for
"unprocessed events" that were read from the channel.
Bug: 332613662
Flag: EXEMPT refactor
Test: TEST=libinput_tests; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST
Change-Id: I9b0128291f6197f099cb9e3c305f9717c0198c90
diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp
index ce8bb43..9665de7 100644
--- a/libs/input/InputConsumerNoResampling.cpp
+++ b/libs/input/InputConsumerNoResampling.cpp
@@ -193,13 +193,29 @@
InputConsumerNoResampling::~InputConsumerNoResampling() {
ensureCalledOnLooperThread(__func__);
- consumeBatchedInputEvents(/*requestedFrameTime=*/std::nullopt);
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) {
+ while (!batches.empty()) {
+ finishInputEvent(batches.front().header.seq, /*handled=*/false);
+ batches.pop();
+ }
+ }
+ // 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"
+ // instead. We can't use "mBatchedSequenceNumbers" for this purpose, because it only contains
+ // batchable (i.e., ACTION_MOVE) events that were sent to the callbacks.
+ const size_t unfinishedEvents = mConsumeTimes.size();
+ LOG_IF(INFO, unfinishedEvents != 0)
+ << getName() << " has " << unfinishedEvents << " unfinished event(s)";
}
int InputConsumerNoResampling::handleReceiveCallback(int events) {