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

Author: vishniakou

libs/input/InputConsumerNoResampling.cpp

@@ -193,13 +193,29 @@ InputConsumerNoResampling::InputConsumerNoResampling(
 
 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) {

libs/input/tests/InputConsumer_test.cpp

@@ -112,18 +112,23 @@ class InputConsumerTest : public testing::Test, public InputConsumerCallbacks {
     std::queue<std::unique_ptr<DragEvent>> mDragEvents;
     std::queue<std::unique_ptr<TouchModeEvent>> mTouchModeEvents;
 
+    // Whether or not to automatically call "finish" whenever a motion event is received.
+    bool mShouldFinishMotions{true};
+
 private:
     uint32_t mLastSeq{0};
     size_t mOnBatchedInputEventPendingInvocationCount{0};
 
     // InputConsumerCallbacks interface
     void onKeyEvent(std::unique_ptr<KeyEvent> event, uint32_t seq) override {
         mKeyEvents.push(std::move(event));
-        mConsumer->finishInputEvent(seq, true);
+        mConsumer->finishInputEvent(seq, /*handled=*/true);
     }
     void onMotionEvent(std::unique_ptr<MotionEvent> event, uint32_t seq) override {
         mMotionEvents.push(std::move(event));
-        mConsumer->finishInputEvent(seq, true);
+        if (mShouldFinishMotions) {
+            mConsumer->finishInputEvent(seq, /*handled=*/true);
+        }
     }
     void onBatchedInputEventPending(int32_t pendingBatchSource) override {
         if (!mConsumer->probablyHasInput()) {
@@ -133,19 +138,19 @@ class InputConsumerTest : public testing::Test, public InputConsumerCallbacks {
     };
     void onFocusEvent(std::unique_ptr<FocusEvent> event, uint32_t seq) override {
         mFocusEvents.push(std::move(event));
-        mConsumer->finishInputEvent(seq, true);
+        mConsumer->finishInputEvent(seq, /*handled=*/true);
     };
     void onCaptureEvent(std::unique_ptr<CaptureEvent> event, uint32_t seq) override {
         mCaptureEvents.push(std::move(event));
-        mConsumer->finishInputEvent(seq, true);
+        mConsumer->finishInputEvent(seq, /*handled=*/true);
     };
     void onDragEvent(std::unique_ptr<DragEvent> event, uint32_t seq) override {
         mDragEvents.push(std::move(event));
-        mConsumer->finishInputEvent(seq, true);
+        mConsumer->finishInputEvent(seq, /*handled=*/true);
     }
     void onTouchModeEvent(std::unique_ptr<TouchModeEvent> event, uint32_t seq) override {
         mTouchModeEvents.push(std::move(event));
-        mConsumer->finishInputEvent(seq, true);
+        mConsumer->finishInputEvent(seq, /*handled=*/true);
     };
 };
 
@@ -231,16 +236,58 @@ TEST_F(InputConsumerTest, LastBatchedSampleIsLessThanResampleTime) {
     EXPECT_LT(moveMotionEvent->getHistoricalEventTime(numSamples - 2),
               moveMotionEvent->getEventTime());
 
-    // Consume all remaining events before ending the test. Otherwise, the smart pointer that owns
-    // consumer is set to null before destroying consumer. This leads to a member function call on a
-    // null object.
-    // TODO(b/332613662): Remove this workaround.
-    mConsumer->consumeBatchedInputEvents(std::nullopt);
+    mClientTestChannel->assertFinishMessage(/*seq=*/0, /*handled=*/true);
+    mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/true);
+    mClientTestChannel->assertFinishMessage(/*seq=*/2, /*handled=*/true);
+    // The event with seq=3 remains unconsumed, and therefore finish will not be called for it until
+    // after the consumer is destroyed.
+    mConsumer.reset();
+    mClientTestChannel->assertFinishMessage(/*seq=*/3, /*handled=*/false);
+    mClientTestChannel->assertNoSentMessages();
+}
+
+/**
+ * During normal operation, the user of InputConsumer (callbacks) is expected to call "finish"
+ * for each input event received in InputConsumerCallbacks.
+ * If the InputConsumer is destroyed, the events that were already sent to the callbacks will not
+ * be finished automatically.
+ */
+TEST_F(InputConsumerTest, UnhandledEventsNotFinishedInDestructor) {
+    mClientTestChannel->enqueueMessage(
+            InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/0}.action(ACTION_DOWN).build());
+    mClientTestChannel->enqueueMessage(
+            InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/1}.action(ACTION_MOVE).build());
+    mShouldFinishMotions = false;
+    invokeLooperCallback();
+    assertOnBatchedInputEventPendingWasCalled();
+    assertReceivedMotionEvent(WithMotionAction(ACTION_DOWN));
+    mClientTestChannel->assertNoSentMessages();
+    // The "finishInputEvent" was not called by the InputConsumerCallbacks.
+    // Now, destroy the consumer and check that the "finish" was not called automatically for the
+    // DOWN event, but was called for the undelivered MOVE event.
+    mConsumer.reset();
+    mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/false);
+    mClientTestChannel->assertNoSentMessages();
+}
 
-    mClientTestChannel->assertFinishMessage(/*seq=*/0, true);
-    mClientTestChannel->assertFinishMessage(/*seq=*/1, true);
-    mClientTestChannel->assertFinishMessage(/*seq=*/2, true);
-    mClientTestChannel->assertFinishMessage(/*seq=*/3, true);
+/**
+ * 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.
+ * This test is similar to the one above, but here we are calling "finish"
+ * automatically for any event received in the callbacks.
+ */
+TEST_F(InputConsumerTest, UnconsumedEventDoesNotCauseACrash) {
+    mClientTestChannel->enqueueMessage(
+            InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/0}.action(ACTION_DOWN).build());
+    invokeLooperCallback();
+    assertReceivedMotionEvent(WithMotionAction(ACTION_DOWN));
+    mClientTestChannel->assertFinishMessage(/*seq=*/0, /*handled=*/true);
+    mClientTestChannel->enqueueMessage(
+            InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/1}.action(ACTION_MOVE).build());
+    invokeLooperCallback();
+    mConsumer.reset();
+    mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/false);
 }
 
 TEST_F(InputConsumerTest, BatchedEventsMultiDeviceConsumption) {