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

Author: vishniakou

include/input/InputConsumerNoResampling.h

@@ -141,7 +141,7 @@ class InputConsumerNoResampling final {
         }
 
     private:
-        std::function<int(int events)> mCallback;
+        const std::function<int(int events)> mCallback;
     };
     sp<LooperEventCallback> mCallback;
     /**

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) {

libs/input/tests/InputConsumer_test.cpp

@@ -70,11 +70,20 @@ class InputConsumerTest : public testing::Test, public InputConsumerCallbacks {
                                            []() { 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() {
@@ -270,6 +279,27 @@ TEST_F(InputConsumerTest, UnhandledEventsNotFinishedInDestructor) {
     mClientTestChannel->assertNoSentMessages();
 }
 
+/**
+ * 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