Reland "Let InputReader handle its own thread"

This CL was first landed in Ic732436d4f00a831e317be1f16ac106a11652cff
but was reverted due to flaky tests.

The flaky tests were caused by races between the instrumented test
classes and the InputReader class under test, which now runs in a new
thread.

In addition to re-landing the change, this CL ensures that InputReader
does not start its own thread in its unit tests, but instead keep using
the loopOnce method that is exposed for testing.

Bug: 130819454
Test: atest inputflinger_tests
Test: Touch input works on crosshatch
Change-Id: I5bbfc68b6be6745efc8b7106a9292ee3cb431c9c

7 files changed, 118 insertions, 80 deletions

diff --git a/services/inputflinger/InputManager.cpp b/services/inputflinger/InputManager.cpp
index e7640dd6af..1043390f84 100644
--- a/services/inputflinger/InputManager.cpp
+++ b/services/inputflinger/InputManager.cpp
@@ -46,7 +46,6 @@ InputManager::~InputManager() {
 }
 
 void InputManager::initialize() {
-    mReaderThread = new InputReaderThread(mReader);
     mDispatcherThread = new InputDispatcherThread(mDispatcher);
 }
 
@@ -57,9 +56,9 @@ status_t InputManager::start() {
         return result;
     }
 
-    result = mReaderThread->run("InputReader", PRIORITY_URGENT_DISPLAY);
+    result = mReader->start();
     if (result) {
-        ALOGE("Could not start InputReader thread due to error %d.", result);
+        ALOGE("Could not start InputReader due to error %d.", result);
 
         mDispatcherThread->requestExit();
         return result;
@@ -69,9 +68,9 @@ status_t InputManager::start() {
 }
 
 status_t InputManager::stop() {
-    status_t result = mReaderThread->requestExitAndWait();
+    status_t result = mReader->stop();
     if (result) {
-        ALOGW("Could not stop InputReader thread due to error %d.", result);
+        ALOGW("Could not stop InputReader due to error %d.", result);
     }
 
     result = mDispatcherThread->requestExitAndWait();
diff --git a/services/inputflinger/InputManager.h b/services/inputflinger/InputManager.h
index 40f66d82f4..2a7ed0ff44 100644
--- a/services/inputflinger/InputManager.h
+++ b/services/inputflinger/InputManager.h
@@ -43,15 +43,15 @@ class InputDispatcherThread;
 /*
  * The input manager is the core of the system event processing.
  *
- * The input manager uses two threads.
+ * The input manager has two components.
  *
- * 1. The InputReaderThread (called "InputReader") reads and preprocesses raw input events,
- *    applies policy, and posts messages to a queue managed by the DispatcherThread.
+ * 1. The InputReader class starts a thread that reads and preprocesses raw input events, applies
+ *    policy, and posts messages to a queue managed by the InputDispatcherThread.
  * 2. The InputDispatcherThread (called "InputDispatcher") thread waits for new events on the
  *    queue and asynchronously dispatches them to applications.
  *
- * By design, the InputReaderThread class and InputDispatcherThread class do not share any
- * internal state.  Moreover, all communication is done one way from the InputReaderThread
+ * By design, the InputReader class and InputDispatcherThread class do not share any
+ * internal state.  Moreover, all communication is done one way from the InputReader
  * into the InputDispatcherThread and never the reverse.  Both classes may interact with the
  * InputDispatchPolicy, however.
  *
@@ -102,7 +102,6 @@ public:
 
 private:
     sp<InputReaderInterface> mReader;
-    sp<InputReaderThread> mReaderThread;
 
     sp<InputClassifierInterface> mClassifier;
 
diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp
index 0422d8342b..2d6f2c1cc9 100644
--- a/services/inputflinger/InputReaderBase.cpp
+++ b/services/inputflinger/InputReaderBase.cpp
@@ -33,20 +33,6 @@ using android::base::StringPrintf;
 
 namespace android {
 
-// --- InputReaderThread ---
-
-InputReaderThread::InputReaderThread(const sp<InputReaderInterface>& reader) :
-        Thread(/*canCallJava*/ true), mReader(reader) {
-}
-
-InputReaderThread::~InputReaderThread() {
-}
-
-bool InputReaderThread::threadLoop() {
-    mReader->loopOnce();
-    return true;
-}
-
 // --- InputReaderConfiguration ---
 
 std::string InputReaderConfiguration::changesToString(uint32_t changes) {
diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h
index 5d576b94f3..56c0a7356d 100644
--- a/services/inputflinger/include/InputReaderBase.h
+++ b/services/inputflinger/include/InputReaderBase.h
@@ -19,12 +19,12 @@
 
 #include "PointerControllerInterface.h"
 
+#include <input/DisplayViewport.h>
 #include <input/Input.h>
 #include <input/InputDevice.h>
-#include <input/DisplayViewport.h>
 #include <input/VelocityControl.h>
 #include <input/VelocityTracker.h>
-#include <utils/Thread.h>
+#include <utils/Errors.h>
 #include <utils/RefBase.h>
 
 #include <stddef.h>
@@ -44,7 +44,16 @@
 
 namespace android {
 
-/* Processes raw input events and sends cooked event data to an input listener. */
+// --- InputReaderInterface ---
+
+/* The interface for the InputReader shared library.
+ *
+ * Manages one or more threads that process raw input events and sends cooked event data to an
+ * input listener.
+ *
+ * The implementation must guarantee thread safety for this interface. However, since the input
+ * listener is NOT thread safe, all calls to the listener must happen from the same thread.
+ */
 class InputReaderInterface : public virtual RefBase {
 protected:
     InputReaderInterface() { }
@@ -56,18 +65,17 @@ public:
      * This method may be called on any thread (usually by the input manager). */
     virtual void dump(std::string& dump) = 0;
 
-    /* Called by the heatbeat to ensures that the reader has not deadlocked. */
+    /* Called by the heartbeat to ensures that the reader has not deadlocked. */
     virtual void monitor() = 0;
 
     /* Returns true if the input device is enabled. */
     virtual bool isInputDeviceEnabled(int32_t deviceId) = 0;
 
-    /* Runs a single iteration of the processing loop.
-     * Nominally reads and processes one incoming message from the EventHub.
-     *
-     * This method should be called on the input reader thread.
-     */
-    virtual void loopOnce() = 0;
+    /* Makes the reader start processing events from the kernel. */
+    virtual status_t start() = 0;
+
+    /* Makes the reader stop processing any more events. */
+    virtual status_t stop() = 0;
 
     /* Gets information about all input devices.
      *
@@ -104,17 +112,7 @@ public:
     virtual bool canDispatchToDisplay(int32_t deviceId, int32_t displayId) = 0;
 };
 
-/* Reads raw events from the event hub and processes them, endlessly. */
-class InputReaderThread : public Thread {
-public:
-    explicit InputReaderThread(const sp<InputReaderInterface>& reader);
-    virtual ~InputReaderThread();
-
-private:
-    sp<InputReaderInterface> mReader;
-
-    virtual bool threadLoop();
-};
+// --- InputReaderConfiguration ---
 
 /*
  * Input reader configuration.
@@ -285,6 +283,8 @@ private:
     std::vector<DisplayViewport> mDisplays;
 };
 
+// --- TouchAffineTransformation ---
+
 struct TouchAffineTransformation {
     float x_scale;
     float x_ymix;
@@ -307,6 +307,8 @@ struct TouchAffineTransformation {
     void applyTo(float& x, float& y) const;
 };
 
+// --- InputReaderPolicyInterface ---
+
 /*
  * Input reader policy interface.
  *
@@ -316,8 +318,8 @@ struct TouchAffineTransformation {
  * The actual implementation is partially supported by callbacks into the DVM
  * via JNI.  This interface is also mocked in the unit tests.
  *
- * These methods must NOT re-enter the input reader since they may be called while
- * holding the input reader lock.
+ * These methods will NOT re-enter the input reader interface, so they may be called from
+ * any method in the input reader interface.
  */
 class InputReaderPolicyInterface : public virtual RefBase {
 protected:
diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp
index 1c5adc3a85..05f0db1329 100644
--- a/services/inputflinger/reader/InputReader.cpp
+++ b/services/inputflinger/reader/InputReader.cpp
@@ -38,16 +38,38 @@
 #include <unistd.h>
 
 #include <log/log.h>
+#include <utils/Errors.h>
 
 #include <android-base/stringprintf.h>
 #include <input/Keyboard.h>
 #include <input/VirtualKeyMap.h>
-
+#include <utils/Thread.h>
 
 using android::base::StringPrintf;
 
 namespace android {
 
+// --- InputReader::InputReaderThread ---
+
+/* Thread that reads raw events from the event hub and processes them, endlessly. */
+class InputReader::InputReaderThread : public Thread {
+public:
+    explicit InputReaderThread(InputReader* reader)
+          : Thread(/* canCallJava */ true), mReader(reader) {}
+
+    ~InputReaderThread() {}
+
+private:
+    InputReader* mReader;
+
+    bool threadLoop() override {
+        mReader->loopOnce();
+        return true;
+    }
+};
+
+// --- InputReader ---
+
 InputReader::InputReader(std::shared_ptr<EventHubInterface> eventHub,
                          const sp<InputReaderPolicyInterface>& policy,
                          const sp<InputListenerInterface>& listener)
@@ -61,6 +83,7 @@ InputReader::InputReader(std::shared_ptr<EventHubInterface> eventHub,
         mNextTimeout(LLONG_MAX),
         mConfigurationChangesToRefresh(0) {
     mQueuedListener = new QueuedInputListener(listener);
+    mThread = new InputReaderThread(this);
 
     { // acquire lock
         AutoMutex _l(mLock);
@@ -76,6 +99,28 @@ InputReader::~InputReader() {
     }
 }
 
+status_t InputReader::start() {
+    if (mThread->isRunning()) {
+        return ALREADY_EXISTS;
+    }
+    return mThread->run("InputReader", PRIORITY_URGENT_DISPLAY);
+}
+
+status_t InputReader::stop() {
+    if (!mThread->isRunning()) {
+        return OK;
+    }
+    if (gettid() == mThread->getTid()) {
+        ALOGE("InputReader can only be stopped from outside of the InputReaderThread!");
+        return INVALID_OPERATION;
+    }
+    // Directly calling requestExitAndWait() causes the thread to not exit
+    // if mEventHub is waiting for a long timeout.
+    mThread->requestExit();
+    mEventHub->wake();
+    return mThread->requestExitAndWait();
+}
+
 void InputReader::loopOnce() {
     int32_t oldGeneration;
     int32_t timeoutMillis;
diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h
index 557eb3b7c4..502490601e 100644
--- a/services/inputflinger/reader/include/InputReader.h
+++ b/services/inputflinger/reader/include/InputReader.h
@@ -38,12 +38,12 @@ struct StylusState;
  * that it sends to the input listener.  Some functions of the input reader, such as early
  * event filtering in low power states, are controlled by a separate policy object.
  *
- * The InputReader owns a collection of InputMappers.  Most of the work it does happens
- * on the input reader thread but the InputReader can receive queries from other system
+ * The InputReader owns a collection of InputMappers. InputReader starts its own thread, where
+ * most of the work happens, but the InputReader can receive queries from other system
  * components running on arbitrary threads.  To keep things manageable, the InputReader
  * uses a single Mutex to guard its state.  The Mutex may be held while calling into the
  * EventHub or the InputReaderPolicy but it is never held while calling into the
- * InputListener.
+ * InputListener. All calls to InputListener must happen from InputReader's thread.
  */
 class InputReader : public InputReaderInterface {
 public:
@@ -55,7 +55,8 @@ public:
     virtual void dump(std::string& dump) override;
     virtual void monitor() override;
 
-    virtual void loopOnce() override;
+    virtual status_t start() override;
+    virtual status_t stop() override;
 
     virtual void getInputDevices(std::vector<InputDeviceInfo>& outInputDevices) override;
 
@@ -86,6 +87,10 @@ protected:
                                             const InputDeviceIdentifier& identifier,
                                             uint32_t classes);
 
+    // With each iteration of the loop, InputReader reads and processes one incoming message from
+    // the EventHub.
+    void loopOnce();
+
     class ContextImpl : public InputReaderContext {
         InputReader* mReader;
 
@@ -111,6 +116,9 @@ protected:
     friend class ContextImpl;
 
 private:
+    class InputReaderThread;
+    sp<InputReaderThread> mThread;
+
     Mutex mLock;
 
     Condition mReaderIsAliveCondition;
diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp
index c1c912214a..d6624c9f5a 100644
--- a/services/inputflinger/tests/InputReader_test.cpp
+++ b/services/inputflinger/tests/InputReader_test.cpp
@@ -1113,6 +1113,9 @@ public:
                                classes);
     }
 
+    // Make the protected loopOnce method accessible to tests.
+    using InputReader::loopOnce;
+
 protected:
     virtual InputDevice* createDeviceLocked(int32_t deviceId, int32_t controllerNumber,
                                             const InputDeviceIdentifier& identifier,
@@ -1133,12 +1136,8 @@ class InputReaderPolicyTest : public testing::Test {
 protected:
     sp<FakeInputReaderPolicy> mFakePolicy;
 
-    virtual void SetUp() {
-        mFakePolicy = new FakeInputReaderPolicy();
-    }
-    virtual void TearDown() {
-        mFakePolicy.clear();
-    }
+    virtual void SetUp() override { mFakePolicy = new FakeInputReaderPolicy(); }
+    virtual void TearDown() override { mFakePolicy.clear(); }
 };
 
 /**
@@ -1321,19 +1320,18 @@ protected:
     sp<TestInputListener> mFakeListener;
     sp<FakeInputReaderPolicy> mFakePolicy;
     std::shared_ptr<FakeEventHub> mFakeEventHub;
-    sp<InstrumentedInputReader> mReader;
+    std::unique_ptr<InstrumentedInputReader> mReader;
 
-    virtual void SetUp() {
+    virtual void SetUp() override {
         mFakeEventHub = std::make_unique<FakeEventHub>();
         mFakePolicy = new FakeInputReaderPolicy();
         mFakeListener = new TestInputListener();
 
-        mReader = new InstrumentedInputReader(mFakeEventHub, mFakePolicy, mFakeListener);
+        mReader = std::make_unique<InstrumentedInputReader>(mFakeEventHub, mFakePolicy,
+                                                            mFakeListener);
     }
 
-    virtual void TearDown() {
-        mReader.clear();
-
+    virtual void TearDown() override {
         mFakeListener.clear();
         mFakePolicy.clear();
     }
@@ -1354,16 +1352,12 @@ protected:
 
     void disableDevice(int32_t deviceId, InputDevice* device) {
         mFakePolicy->addDisabledDevice(deviceId);
-        configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device);
+        mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE);
     }
 
     void enableDevice(int32_t deviceId, InputDevice* device) {
         mFakePolicy->removeDisabledDevice(deviceId);
-        configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device);
-    }
-
-    void configureDevice(uint32_t changes, InputDevice* device) {
-        device->configure(ARBITRARY_TIME, mFakePolicy->getReaderConfiguration(), changes);
+        mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE);
     }
 
     FakeInputMapper* addDeviceWithFakeInputMapper(int32_t deviceId, int32_t controllerNumber,
@@ -1417,7 +1411,6 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) {
 
     NotifyDeviceResetArgs resetArgs;
     ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs));
-    ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime);
     ASSERT_EQ(deviceId, resetArgs.deviceId);
 
     ASSERT_EQ(device->isEnabled(), true);
@@ -1425,7 +1418,6 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) {
     mReader->loopOnce();
 
     ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs));
-    ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime);
     ASSERT_EQ(deviceId, resetArgs.deviceId);
     ASSERT_EQ(device->isEnabled(), false);
 
@@ -1438,7 +1430,6 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) {
     enableDevice(deviceId, device);
     mReader->loopOnce();
     ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs));
-    ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime);
     ASSERT_EQ(deviceId, resetArgs.deviceId);
     ASSERT_EQ(device->isEnabled(), true);
 }
@@ -1629,7 +1620,6 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) {
     FakeInputMapper* mapper = new FakeInputMapper(device, AINPUT_SOURCE_TOUCHSCREEN);
     device->addMapper(mapper);
     mReader->setNextDevice(device);
-    addDevice(deviceId, "fake", deviceClass, nullptr);
 
     const uint8_t hdmi1 = 1;
 
@@ -1637,13 +1627,21 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) {
     mFakePolicy->addInputPortAssociation(DEVICE_LOCATION, hdmi1);
 
     // Add default and second display.
+    mFakePolicy->clearViewports();
     mFakePolicy->addDisplayViewport(DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT,
             DISPLAY_ORIENTATION_0, "local:0", NO_PORT, ViewportType::VIEWPORT_INTERNAL);
     mFakePolicy->addDisplayViewport(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT,
             DISPLAY_ORIENTATION_0, "local:1", hdmi1, ViewportType::VIEWPORT_EXTERNAL);
     mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_DISPLAY_INFO);
     mReader->loopOnce();
+
+    // Add the device, and make sure all of the callbacks are triggered.
+    // The device is added after the input port associations are processed since
+    // we do not yet support dynamic device-to-display associations.
+    ASSERT_NO_FATAL_FAILURE(addDevice(deviceId, "fake", deviceClass, nullptr));
     ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasCalled());
+    ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled());
+    ASSERT_NO_FATAL_FAILURE(mapper->assertConfigureWasCalled());
 
     // Device should only dispatch to the specified display.
     ASSERT_EQ(deviceId, device->getId());
@@ -1652,6 +1650,7 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) {
 
     // Can't dispatch event from a disabled device.
     disableDevice(deviceId, device);
+    mReader->loopOnce();
     ASSERT_FALSE(mReader->canDispatchToDisplay(deviceId, SECONDARY_DISPLAY_ID));
 }
 
@@ -1674,7 +1673,7 @@ protected:
 
     InputDevice* mDevice;
 
-    virtual void SetUp() {
+    virtual void SetUp() override {
         mFakeEventHub = std::make_unique<FakeEventHub>();
         mFakePolicy = new FakeInputReaderPolicy();
         mFakeListener = new TestInputListener();
@@ -1688,7 +1687,7 @@ protected:
                 DEVICE_CONTROLLER_NUMBER, identifier, DEVICE_CLASSES);
     }
 
-    virtual void TearDown() {
+    virtual void TearDown() override {
         delete mDevice;
 
         delete mFakeContext;
@@ -1912,7 +1911,7 @@ protected:
     FakeInputReaderContext* mFakeContext;
     InputDevice* mDevice;
 
-    virtual void SetUp() {
+    virtual void SetUp() override {
         mFakeEventHub = std::make_unique<FakeEventHub>();
         mFakePolicy = new FakeInputReaderPolicy();
         mFakeListener = new TestInputListener();
@@ -1926,7 +1925,7 @@ protected:
         mFakeEventHub->addDevice(mDevice->getId(), DEVICE_NAME, 0);
     }
 
-    virtual void TearDown() {
+    virtual void TearDown() override {
         delete mDevice;
         delete mFakeContext;
         mFakeListener.clear();
@@ -2589,7 +2588,7 @@ protected:
 
     sp<FakePointerController> mFakePointerController;
 
-    virtual void SetUp() {
+    virtual void SetUp() override {
         InputMapperTest::SetUp();
 
         mFakePointerController = new FakePointerController();