Properly initialize MotionClassifier

InputClassifier will now play a more active role in managing the
lifecycle of MotionClassifier.

In the previous version of the code, we had a circular reference:
MotionClassifier owned a wp<InputClassifier>, that sometimes got
promoted to sp<>.
But the InputClassifier was the official owner of MotionClassifier, and
owned a unique_ptr<MotionClassifier>.

The owner of InputClassifier in real code is InputManager, and in test
code, it's the test class. When the owner of InputClassifier destroyed
InputClassifier, this could sometimes happen at the time when the
MotionClassifier also held a reference to the InputClassifier. That
meant that the proper owner of InputClassifier was not invoking the
destructor.
Instead, the MotionClassifier was invoking the InputClassifier
destructor.

To fix the situation, we now do the following:

1. InputClassifier will never die before MotionClassifier.

2. MotionClassifier constructor is now allowed to block. It would block
for some time because calling getService takes a while. To account for
this, InputClassifier launches a new thread to create MotionClassifier.

3. When MotionClassifier is ready to process events, InputClassifier
updates mMotionClassifier, which makes it non-null.

4. We now create a separate death recipient, which is co-owned by
InputClassifier and MotionClassifier. This is done so that the refcount
of the deathrecipient does not affect the refcount of InputClassifier,
and thus enforces the ownership of InputClassifier by the InputManager.
Now, no one can call ~InputClassifier except for its real owner.

5. MotionClassifier will subscribe the death recipient to the death of the
HAL. InputClassifier will delete MotionClassifier if HAL dies.
MotionClassifier no longer holds on to the death recipient.

6. We move the loop of the MotionClassifier thread to focus only on
processing events. That thread will no longer do any initialization.

7. Remove the thread check inside MotionClassifier. It isn't really
useful, now that there's only 1 function for the entire thread.

Ownership summary: Both InputClassifier and MotionClassifier own
DeathRecipient. DeathRecipient has a reference to InputClassifier. Thus,
we must guarantee that DeathRecipient dies before InputClassifier.
InputClassifier owns MotionClassifier. This is OK, since even if
InputClassifier dies, it will first delete MotionClassifier. That will
cause MotionClassifier to release 1 refCount from DeathRecipient. That
means the only reference remaining to DeathRecipient will be inside
InputClassifier, so InputClassifier will always be alive until
DeathRecipient is dead. Similar argument applies if MotionClassifier and
DeathRecipient die in different order (as observed from
InputClassifier).

Tests:
1) Manually running inputflinger_tests on cuttlefish:
build/launch cuttlefish using go/acloud
m inputflinger_tests
adb push out/target/product/vsoc_x86/data/nativetest/inputflinger_tests/inputflinger_tests /data/nativetest/inputflinger_tests/inputflinger_tests
adb shell
/data/nativetest/inputflinger_tests # ./inputflinger_tests --gtest_filter=*InputClassifierTest* --gtest_repeat=1000 --gtest_break_on_failure
2) Boot flame and open logcat. Observe in logcat:
StartInputManagerService took to complete: 2ms
Previously, in synchronous approach ( b/130184032) it was
about 100 ms (so we did not regress).
3) Kill the HAL while system_server is running, and dumpsys input before
and after:
adb shell dumpsys input (observe that MotionClassifier is non-null on flame)
adb shell -t killall android.hardware.input.classifier@1.0-service
adb shell dumpsys input (observe that MotionCLassifier is null)

Bug: 148311342
Test: see "Tests" section above
Change-Id: Ic76b82bd5f2cd374e3b001400eb495ca36de7353
Merged-In: Ic76b82bd5f2cd374e3b001400eb495ca36de7353
(cherry picked from commit 1652397ab77904092509a0315ad026f0b4f10a18)

3 files changed, 135 insertions, 120 deletions

diff --git a/services/inputflinger/InputClassifier.cpp b/services/inputflinger/InputClassifier.cpp
index ef1a2247e9..6e51fb2717 100644
--- a/services/inputflinger/InputClassifier.cpp
+++ b/services/inputflinger/InputClassifier.cpp
@@ -46,8 +46,6 @@ using namespace android::hardware::input;
 
 namespace android {
 
-static constexpr bool DEBUG = false;
-
 // Category (=namespace) name for the input settings that are applied at boot time
 static const char* INPUT_NATIVE_BOOT = "input_native_boot";
 // Feature flag name for the deep press feature
@@ -141,53 +139,46 @@ std::optional<int32_t> ClassifierEvent::getDeviceId() const {
 
 // --- MotionClassifier ---
 
-MotionClassifier::MotionClassifier(sp<android::hardware::hidl_death_recipient> deathRecipient) :
-        mDeathRecipient(deathRecipient), mEvents(MAX_EVENTS) {
-    mHalThread = std::thread(&MotionClassifier::callInputClassifierHal, this);
+MotionClassifier::MotionClassifier(
+        sp<android::hardware::input::classifier::V1_0::IInputClassifier> service)
+      : mEvents(MAX_EVENTS), mService(service) {
+    // Under normal operation, we do not need to reset the HAL here. But in the case where system
+    // crashed, but HAL didn't, we may be connecting to an existing HAL process that might already
+    // have received events in the past. That means, that HAL could be in an inconsistent state
+    // once it receives events from the newly created MotionClassifier.
+    mEvents.push(ClassifierEvent::createHalResetEvent());
+
+    mHalThread = std::thread(&MotionClassifier::processEvents, this);
 #if defined(__linux__)
     // Set the thread name for debugging
     pthread_setname_np(mHalThread.native_handle(), "InputClassifier");
 #endif
 }
 
-/**
- * This function may block for some time to initialize the HAL, so it should only be called
- * from the "InputClassifier HAL" thread.
- */
-bool MotionClassifier::init() {
-    ensureHalThread(__func__);
+std::unique_ptr<MotionClassifierInterface> MotionClassifier::create(
+        sp<android::hardware::hidl_death_recipient> deathRecipient) {
+    if (!deepPressEnabled()) {
+        // If feature is not enabled, MotionClassifier should stay null to avoid unnecessary work.
+        // When MotionClassifier is null, InputClassifier will forward all events
+        // to the next InputListener, unmodified.
+        return nullptr;
+    }
     sp<android::hardware::input::classifier::V1_0::IInputClassifier> service =
             classifier::V1_0::IInputClassifier::getService();
     if (!service) {
         // Not really an error, maybe the device does not have this HAL,
         // but somehow the feature flag is flipped
         ALOGI("Could not obtain InputClassifier HAL");
-        return false;
+        return nullptr;
     }
 
-    sp<android::hardware::hidl_death_recipient> recipient = mDeathRecipient.promote();
-    if (recipient != nullptr) {
-        const bool linked = service->linkToDeath(recipient, 0 /* cookie */).withDefault(false);
-        if (!linked) {
-            ALOGE("Could not link MotionClassifier to the HAL death");
-            return false;
-        }
-    }
-
-    // Under normal operation, we do not need to reset the HAL here. But in the case where system
-    // crashed, but HAL didn't, we may be connecting to an existing HAL process that might already
-    // have received events in the past. That means, that HAL could be in an inconsistent state
-    // once it receives events from the newly created MotionClassifier.
-    mEvents.push(ClassifierEvent::createHalResetEvent());
-
-    {
-        std::scoped_lock lock(mLock);
-        if (mService) {
-            ALOGE("MotionClassifier::%s should only be called once", __func__);
-        }
-        mService = service;
+    const bool linked = service->linkToDeath(deathRecipient, 0 /* cookie */).withDefault(false);
+    if (!linked) {
+        ALOGE("Could not link death recipient to the HAL death");
+        return nullptr;
     }
-    return true;
+    // Using 'new' to access a non-public constructor
+    return std::unique_ptr<MotionClassifier>(new MotionClassifier(service));
 }
 
 MotionClassifier::~MotionClassifier() {
@@ -195,14 +186,6 @@ MotionClassifier::~MotionClassifier() {
     mHalThread.join();
 }
 
-void MotionClassifier::ensureHalThread(const char* function) {
-    if (DEBUG) {
-        if (std::this_thread::get_id() != mHalThread.get_id()) {
-            LOG_FATAL("Function %s should only be called from InputClassifier thread", function);
-        }
-    }
-}
-
 /**
  * Obtain the classification from the HAL for a given MotionEvent.
  * Should only be called from the InputClassifier thread (mHalThread).
@@ -213,23 +196,7 @@ void MotionClassifier::ensureHalThread(const char* function) {
  * To remove any possibility of negatively affecting the touch latency, the HAL
  * is called from a dedicated thread.
  */
-void MotionClassifier::callInputClassifierHal() {
-    ensureHalThread(__func__);
-    const bool initialized = init();
-    if (!initialized) {
-        // MotionClassifier no longer useful.
-        // Deliver death notification from a separate thread
-        // because ~MotionClassifier may be invoked, which calls mHalThread.join()
-        std::thread([deathRecipient = mDeathRecipient](){
-                sp<android::hardware::hidl_death_recipient> recipient = deathRecipient.promote();
-                if (recipient != nullptr) {
-                    recipient->serviceDied(0 /*cookie*/, nullptr);
-                }
-        }).detach();
-        return;
-    }
-    // From this point on, mService is guaranteed to be non-null.
-
+void MotionClassifier::processEvents() {
     while (true) {
         ClassifierEvent event = mEvents.pop();
         bool halResponseOk = true;
@@ -383,24 +350,30 @@ void MotionClassifier::dump(std::string& dump) {
     }
 }
 
+// --- HalDeathRecipient
 
-// --- InputClassifier ---
+InputClassifier::HalDeathRecipient::HalDeathRecipient(InputClassifier& parent) : mParent(parent) {}
 
-InputClassifier::InputClassifier(const sp<InputListenerInterface>& listener) :
-        mListener(listener) {
-    // The rest of the initialization is done in onFirstRef, because we need to obtain
-    // an sp to 'this' in order to register for HAL death notifications
+void InputClassifier::HalDeathRecipient::serviceDied(
+        uint64_t cookie, const wp<android::hidl::base::V1_0::IBase>& who) {
+    sp<android::hidl::base::V1_0::IBase> service = who.promote();
+    if (service) {
+        service->unlinkToDeath(this);
+    }
+    mParent.setMotionClassifier(nullptr);
 }
 
-void InputClassifier::onFirstRef() {
-    if (!deepPressEnabled()) {
-        // If feature is not enabled, MotionClassifier should stay null to avoid unnecessary work.
-        // When MotionClassifier is null, InputClassifier will forward all events
-        // to the next InputListener, unmodified.
-        return;
-    }
-    std::scoped_lock lock(mLock);
-    mMotionClassifier = std::make_unique<MotionClassifier>(this);
+// --- InputClassifier ---
+
+InputClassifier::InputClassifier(const sp<InputListenerInterface>& listener)
+      : mListener(listener), mHalDeathRecipient(new HalDeathRecipient(*this)) {
+    mInitializeMotionClassifierThread = std::thread(
+            [this] { setMotionClassifier(MotionClassifier::create(mHalDeathRecipient)); });
+#if defined(__linux__)
+    // Set the thread name for debugging
+    pthread_setname_np(mInitializeMotionClassifierThread.native_handle(),
+                       "Create MotionClassifier");
+#endif
 }
 
 void InputClassifier::notifyConfigurationChanged(const NotifyConfigurationChangedArgs* args) {
@@ -441,15 +414,10 @@ void InputClassifier::notifyDeviceReset(const NotifyDeviceResetArgs* args) {
     mListener->notifyDeviceReset(args);
 }
 
-void InputClassifier::serviceDied(uint64_t /*cookie*/,
-        const wp<android::hidl::base::V1_0::IBase>& who) {
+void InputClassifier::setMotionClassifier(
+        std::unique_ptr<MotionClassifierInterface> motionClassifier) {
     std::scoped_lock lock(mLock);
-    ALOGE("InputClassifier HAL has died. Setting mMotionClassifier to null");
-    mMotionClassifier = nullptr;
-    sp<android::hidl::base::V1_0::IBase> service = who.promote();
-    if (service) {
-        service->unlinkToDeath(this);
-    }
+    mMotionClassifier = std::move(motionClassifier);
 }
 
 void InputClassifier::dump(std::string& dump) {
@@ -465,4 +433,8 @@ void InputClassifier::dump(std::string& dump) {
     dump += "\n";
 }
 
+InputClassifier::~InputClassifier() {
+    mInitializeMotionClassifierThread.join();
+}
+
 } // namespace android
diff --git a/services/inputflinger/InputClassifier.h b/services/inputflinger/InputClassifier.h
index 47e20dbf75..b1769b9d96 100644
--- a/services/inputflinger/InputClassifier.h
+++ b/services/inputflinger/InputClassifier.h
@@ -19,8 +19,8 @@
 
 #include <android-base/thread_annotations.h>
 #include <utils/RefBase.h>
-#include <unordered_map>
 #include <thread>
+#include <unordered_map>
 
 #include "BlockingQueue.h"
 #include "InputListener.h"
@@ -113,23 +113,23 @@ protected:
  */
 class MotionClassifier final : public MotionClassifierInterface {
 public:
-    /**
-     * The deathRecipient will be subscribed to the HAL death. If the death recipient
-     * owns MotionClassifier and receives HAL death, it should delete its copy of it.
-     * The callback serviceDied will also be sent if the MotionClassifier itself fails
-     * to initialize. If the MotionClassifier fails to initialize, it is not useful, and
-     * should be deleted.
-     * If no death recipient is supplied, then the registration step will be skipped, so there will
-     * be no listeners registered for the HAL death. This is useful for testing
-     * MotionClassifier in isolation.
+    /*
+     * Create an instance of MotionClassifier.
+     * The death recipient, if provided, will be subscribed to the HAL death.
+     * The death recipient could be used to destroy MotionClassifier.
+     *
+     * This function should be called asynchronously, because getService takes a long time.
      */
-    explicit MotionClassifier(sp<android::hardware::hidl_death_recipient> deathRecipient = nullptr);
+    static std::unique_ptr<MotionClassifierInterface> create(
+            sp<android::hardware::hidl_death_recipient> deathRecipient);
+
     ~MotionClassifier();
 
     /**
      * Classifies events asynchronously; that is, it doesn't block events on a classification,
-     * but instead sends them over to the classifier HAL and after a classification is
-     * determined, it then marks the next event it sees in the stream with it.
+     * but instead sends them over to the classifier HAL. After a classification of a specific
+     * event is determined, MotionClassifier then marks the next event in the stream with this
+     * classification.
      *
      * Therefore, it is acceptable to have the classifications be delayed by 1-2 events
      * in a particular gesture.
@@ -141,15 +141,9 @@ public:
     virtual void dump(std::string& dump) override;
 
 private:
-    /**
-     * Initialize MotionClassifier.
-     * Return true if initializaion is successful.
-     */
-    bool init();
-    /**
-     * Entity that will be notified of the HAL death (most likely InputClassifier).
-     */
-    wp<android::hardware::hidl_death_recipient> mDeathRecipient;
+    friend class MotionClassifierTest; // to create MotionClassifier with a test HAL implementation
+    explicit MotionClassifier(
+            sp<android::hardware::input::classifier::V1_0::IInputClassifier> service);
 
     // The events that need to be sent to the HAL.
     BlockingQueue<ClassifierEvent> mEvents;
@@ -164,14 +158,9 @@ private:
      */
     std::thread mHalThread;
     /**
-     * Print an error message if the caller is not on the InputClassifier thread.
-     * Caller must supply the name of the calling function as __func__
+     * Process events and call the InputClassifier HAL
      */
-    void ensureHalThread(const char* function);
-    /**
-     * Call the InputClassifier HAL
-     */
-    void callInputClassifierHal();
+    void processEvents();
     /**
      * Access to the InputClassifier HAL. May be null if init() hasn't completed yet.
      * When init() successfully completes, mService is guaranteed to remain non-null and to not
@@ -223,19 +212,15 @@ private:
     const char* getServiceStatus() REQUIRES(mLock);
 };
 
-
 /**
  * Implementation of the InputClassifierInterface.
  * Represents a separate stage of input processing. All of the input events go through this stage.
  * Acts as a passthrough for all input events except for motion events.
  * The events of motion type are sent to MotionClassifier.
  */
-class InputClassifier : public InputClassifierInterface,
-        public android::hardware::hidl_death_recipient {
+class InputClassifier : public InputClassifierInterface {
 public:
     explicit InputClassifier(const sp<InputListenerInterface>& listener);
-    // Some of the constructor logic is finished in onFirstRef
-    virtual void onFirstRef() override;
 
     virtual void notifyConfigurationChanged(const NotifyConfigurationChangedArgs* args) override;
     virtual void notifyKey(const NotifyKeyArgs* args) override;
@@ -243,17 +228,44 @@ public:
     virtual void notifySwitch(const NotifySwitchArgs* args) override;
     virtual void notifyDeviceReset(const NotifyDeviceResetArgs* args) override;
 
-    virtual void serviceDied(uint64_t cookie,
-            const wp<android::hidl::base::V1_0::IBase>& who) override;
-
     virtual void dump(std::string& dump) override;
 
+    ~InputClassifier();
+
 private:
     // Protect access to mMotionClassifier, since it may become null via a hidl callback
     std::mutex mLock;
-    std::unique_ptr<MotionClassifierInterface> mMotionClassifier GUARDED_BY(mLock);
     // The next stage to pass input events to
     sp<InputListenerInterface> mListener;
+
+    std::unique_ptr<MotionClassifierInterface> mMotionClassifier GUARDED_BY(mLock);
+    std::thread mInitializeMotionClassifierThread;
+    /**
+     * Set the value of mMotionClassifier.
+     * This is called from 2 different threads:
+     * 1) mInitializeMotionClassifierThread, when we have constructed a MotionClassifier
+     * 2) A binder thread of the HalDeathRecipient, which is created when HAL dies. This would cause
+     *    mMotionClassifier to become nullptr.
+     */
+    void setMotionClassifier(std::unique_ptr<MotionClassifierInterface> motionClassifier);
+
+    /**
+     * The deathRecipient will call setMotionClassifier(null) when the HAL dies.
+     */
+    class HalDeathRecipient : public android::hardware::hidl_death_recipient {
+    public:
+        explicit HalDeathRecipient(InputClassifier& parent);
+        virtual void serviceDied(uint64_t cookie,
+                                 const wp<android::hidl::base::V1_0::IBase>& who) override;
+
+    private:
+        InputClassifier& mParent;
+    };
+    // We retain a reference to death recipient, because the death recipient will be calling
+    // ~MotionClassifier if the HAL dies.
+    // If we don't retain a reference, and MotionClassifier is the only owner of the death
+    // recipient, the serviceDied call will cause death recipient to call its own destructor.
+    sp<HalDeathRecipient> mHalDeathRecipient;
 };
 
 } // namespace android
diff --git a/services/inputflinger/tests/InputClassifier_test.cpp b/services/inputflinger/tests/InputClassifier_test.cpp
index 7cc17a2215..9121d7ebba 100644
--- a/services/inputflinger/tests/InputClassifier_test.cpp
+++ b/services/inputflinger/tests/InputClassifier_test.cpp
@@ -22,6 +22,9 @@
 #include <android/hardware/input/classifier/1.0/IInputClassifier.h>
 
 using namespace android::hardware::input;
+using android::hardware::Return;
+using android::hardware::Void;
+using android::hardware::input::common::V1_0::Classification;
 
 namespace android {
 
@@ -129,6 +132,27 @@ TEST_F(InputClassifierTest, SendToNextStage_NotifyDeviceResetArgs) {
     ASSERT_EQ(args, outArgs);
 }
 
+/**
+ * A minimal implementation of IInputClassifier.
+ */
+struct TestHal : public android::hardware::input::classifier::V1_0::IInputClassifier {
+    Return<Classification> classify(
+            const android::hardware::input::common::V1_0::MotionEvent& event) override {
+        return Classification::NONE;
+    };
+    Return<void> reset() override { return Void(); };
+    Return<void> resetDevice(int32_t deviceId) override { return Void(); };
+};
+
+/**
+ * An entity that will be subscribed to the HAL death.
+ */
+class TestDeathRecipient : public android::hardware::hidl_death_recipient {
+public:
+    virtual void serviceDied(uint64_t cookie,
+                             const wp<android::hidl::base::V1_0::IBase>& who) override{};
+};
+
 // --- MotionClassifierTest ---
 
 class MotionClassifierTest : public testing::Test {
@@ -136,7 +160,14 @@ protected:
     std::unique_ptr<MotionClassifierInterface> mMotionClassifier;
 
     virtual void SetUp() override {
-        mMotionClassifier = std::make_unique<MotionClassifier>();
+        mMotionClassifier = MotionClassifier::create(new TestDeathRecipient());
+        if (mMotionClassifier == nullptr) {
+            // If the device running this test does not have IInputClassifier service,
+            // use the test HAL instead.
+            // Using 'new' to access non-public constructor
+            mMotionClassifier =
+                    std::unique_ptr<MotionClassifier>(new MotionClassifier(new TestHal()));
+        }
     }
 };