Merge "Remove callback at the end of consumer destructor" into main

3 files changed, 44 insertions, 11 deletions

diff --git a/include/input/InputConsumerNoResampling.h b/include/input/InputConsumerNoResampling.h
index 2e346bb7cd..70d00d16f4 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 d3653cfc45..2c0f77a814 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 06e19bb644..226b892fdc 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.