Restore SocketCallbacks class from HAL impl

The SocketCallbacks abstraction is needed because SocketClient utilizes
a sp<> reference to the SocketClient::ICallbacks object provided to it
as input. If the abstraction isn't present, then there's no way to
ensure a strong pointer reference is also held by
HalChreSocketConnection and a crash can result if the HAL instance is
destroyed due to incorrect destruction ordering.

Bug: 230922800
Test: CHQTS
Change-Id: I93127cac4f6ceddd27adeb11aafb9abc0822b66a

2 files changed, 76 insertions, 62 deletions

diff --git a/host/hal_generic/common/hal_chre_socket_connection.cc b/host/hal_generic/common/hal_chre_socket_connection.cc
index 8ad38b92..2070508f 100644
--- a/host/hal_generic/common/hal_chre_socket_connection.cc
+++ b/host/hal_generic/common/hal_chre_socket_connection.cc
@@ -50,13 +50,10 @@ HalChreSocketConnection::HalChreSocketConnection(
     IChreSocketCallback *callback) {
   constexpr char kChreSocketName[] = "chre";
 
-  if (!mClient.connectInBackground(kChreSocketName, this)) {
+  mSocketCallbacks = sp<SocketCallbacks>::make(*this, callback);
+  if (!mClient.connectInBackground(kChreSocketName, mSocketCallbacks)) {
     ALOGE("Couldn't start socket client");
   }
-#ifdef CHRE_HAL_SOCKET_METRICS_ENABLED
-  mLastClearedTimestamp = elapsedRealtime();
-#endif  // CHRE_HAL_SOCKET_METRICS_ENABLED
-  mCallback = callback;
 }
 
 bool HalChreSocketConnection::getContextHubs(
@@ -171,14 +168,22 @@ bool HalChreSocketConnection::onHostEndpointDisconnected(
   return mClient.sendMessage(builder.GetBufferPointer(), builder.GetSize());
 }
 
-void HalChreSocketConnection::onMessageReceived(const void *data,
-                                                size_t length) {
-  if (!chre::HostProtocolHost::decodeMessageFromChre(data, length, *this)) {
+HalChreSocketConnection::SocketCallbacks::SocketCallbacks(
+    HalChreSocketConnection &parent, IChreSocketCallback *callback)
+    : mParent(parent), mCallback(callback) {
+#ifdef CHRE_HAL_SOCKET_METRICS_ENABLED
+  mLastClearedTimestamp = elapsedRealtime();
+#endif  // CHRE_HAL_SOCKET_METRICS_ENABLED
+}
+
+void HalChreSocketConnection::SocketCallbacks::onMessageReceived(
+    const void *data, size_t length) {
+  if (!HostProtocolHost::decodeMessageFromChre(data, length, *this)) {
     ALOGE("Failed to decode message");
   }
 }
 
-void HalChreSocketConnection::onConnected() {
+void HalChreSocketConnection::SocketCallbacks::onConnected() {
   ALOGI("Reconnected to CHRE daemon");
   if (mHaveConnected) {
     ALOGI("Reconnected to CHRE daemon");
@@ -187,11 +192,11 @@ void HalChreSocketConnection::onConnected() {
   mHaveConnected = true;
 }
 
-void HalChreSocketConnection::onDisconnected() {
+void HalChreSocketConnection::SocketCallbacks::onDisconnected() {
   ALOGW("Lost connection to CHRE daemon");
 }
 
-void HalChreSocketConnection::handleNanoappMessage(
+void HalChreSocketConnection::SocketCallbacks::handleNanoappMessage(
     const ::chre::fbs::NanoappMessageT &message) {
   ALOGD("Got message from nanoapp: ID 0x%" PRIx64, message.app_id);
   mCallback->onNanoappMessage(message);
@@ -220,58 +225,59 @@ void HalChreSocketConnection::handleNanoappMessage(
           .values{std::move(values)},
       };
 
-      reportMetric(atom);
+      mParent.reportMetric(atom);
     }
   }
 #endif  // CHRE_HAL_SOCKET_METRICS_ENABLED
 }
 
-void HalChreSocketConnection::handleHubInfoResponse(
+void HalChreSocketConnection::SocketCallbacks::handleHubInfoResponse(
     const ::chre::fbs::HubInfoResponseT &response) {
   ALOGD("Got hub info response");
 
-  std::lock_guard<std::mutex> lock(mHubInfoMutex);
-  if (mHubInfoValid) {
+  std::lock_guard<std::mutex> lock(mParent.mHubInfoMutex);
+  if (mParent.mHubInfoValid) {
     ALOGI("Ignoring duplicate/unsolicited hub info response");
   } else {
-    mHubInfoResponse = response;
-    mHubInfoValid = true;
-    mHubInfoCond.notify_all();
+    mParent.mHubInfoResponse = response;
+    mParent.mHubInfoValid = true;
+    mParent.mHubInfoCond.notify_all();
   }
 }
 
-void HalChreSocketConnection::handleNanoappListResponse(
+void HalChreSocketConnection::SocketCallbacks::handleNanoappListResponse(
     const ::chre::fbs::NanoappListResponseT &response) {
   ALOGD("Got nanoapp list response with %zu apps", response.nanoapps.size());
   mCallback->onNanoappListResponse(response);
 }
 
-void HalChreSocketConnection::handleLoadNanoappResponse(
+void HalChreSocketConnection::SocketCallbacks::handleLoadNanoappResponse(
     const ::chre::fbs::LoadNanoappResponseT &response) {
   ALOGD("Got load nanoapp response for transaction %" PRIu32
         " fragment %" PRIu32 " with result %d",
         response.transaction_id, response.fragment_id, response.success);
-  std::unique_lock<std::mutex> lock(mPendingLoadTransactionMutex);
+  std::unique_lock<std::mutex> lock(mParent.mPendingLoadTransactionMutex);
 
   // TODO: Handle timeout in receiving load response
-  if (!mPendingLoadTransaction.has_value()) {
+  if (!mParent.mPendingLoadTransaction.has_value()) {
     ALOGE(
         "Dropping unexpected load response (no pending transaction "
         "exists)");
   } else {
-    FragmentedLoadTransaction &transaction = mPendingLoadTransaction.value();
+    FragmentedLoadTransaction &transaction =
+        mParent.mPendingLoadTransaction.value();
 
-    if (!isExpectedLoadResponseLocked(response)) {
+    if (!mParent.isExpectedLoadResponseLocked(response)) {
       ALOGE("Dropping unexpected load response, expected transaction %" PRIu32
             " fragment %" PRIu32 ", received transaction %" PRIu32
             " fragment %" PRIu32,
-            transaction.getTransactionId(), mCurrentFragmentId,
+            transaction.getTransactionId(), mParent.mCurrentFragmentId,
             response.transaction_id, response.fragment_id);
     } else {
       bool success = false;
       bool continueLoadRequest = false;
       if (response.success && !transaction.isComplete()) {
-        if (sendFragmentedLoadNanoAppRequest(transaction)) {
+        if (mParent.sendFragmentedLoadNanoAppRequest(transaction)) {
           continueLoadRequest = true;
           success = true;
         }
@@ -280,7 +286,7 @@ void HalChreSocketConnection::handleLoadNanoappResponse(
       }
 
       if (!continueLoadRequest) {
-        mPendingLoadTransaction.reset();
+        mParent.mPendingLoadTransaction.reset();
         lock.unlock();
         mCallback->onTransactionResult(response.transaction_id, success);
       }
@@ -288,7 +294,7 @@ void HalChreSocketConnection::handleLoadNanoappResponse(
   }
 }
 
-void HalChreSocketConnection::handleUnloadNanoappResponse(
+void HalChreSocketConnection::SocketCallbacks::handleUnloadNanoappResponse(
     const ::chre::fbs::UnloadNanoappResponseT &response) {
   ALOGV("Got unload nanoapp response for transaction %" PRIu32
         " with result %d",
@@ -296,13 +302,13 @@ void HalChreSocketConnection::handleUnloadNanoappResponse(
   mCallback->onTransactionResult(response.transaction_id, response.success);
 }
 
-void HalChreSocketConnection::handleDebugDumpData(
+void HalChreSocketConnection::SocketCallbacks::handleDebugDumpData(
     const ::chre::fbs::DebugDumpDataT &data) {
   ALOGV("Got debug dump data, size %zu", data.debug_str.size());
   mCallback->onDebugDumpData(data);
 }
 
-void HalChreSocketConnection::handleDebugDumpResponse(
+void HalChreSocketConnection::SocketCallbacks::handleDebugDumpResponse(
     const ::chre::fbs::DebugDumpResponseT &response) {
   ALOGV("Got debug dump response, success %d, data count %" PRIu32,
         response.success, response.data_count);
diff --git a/host/hal_generic/common/hal_chre_socket_connection.h b/host/hal_generic/common/hal_chre_socket_connection.h
index 1f8a4a04..983816c9 100644
--- a/host/hal_generic/common/hal_chre_socket_connection.h
+++ b/host/hal_generic/common/hal_chre_socket_connection.h
@@ -93,9 +93,7 @@ class IChreSocketCallback {
 /**
  * A helper class that can be used to connect to the CHRE socket.
  */
-class HalChreSocketConnection
-    : public ::android::chre::SocketClient::ICallbacks,
-      public ::android::chre::IChreMessageHandlers {
+class HalChreSocketConnection {
  public:
   HalChreSocketConnection(IChreSocketCallback *callback);
 
@@ -122,27 +120,47 @@ class HalChreSocketConnection
 
   bool onHostEndpointDisconnected(uint16_t hostEndpointId);
 
-  void onMessageReceived(const void *data, size_t length) override;
-  void onConnected() override;
-  void onDisconnected() override;
-  void handleNanoappMessage(
-      const ::chre::fbs::NanoappMessageT &message) override;
-  void handleHubInfoResponse(
-      const ::chre::fbs::HubInfoResponseT &response) override;
-  void handleNanoappListResponse(
-      const ::chre::fbs::NanoappListResponseT &response) override;
-  void handleLoadNanoappResponse(
-      const ::chre::fbs::LoadNanoappResponseT &response) override;
-  void handleUnloadNanoappResponse(
-      const ::chre::fbs::UnloadNanoappResponseT &response) override;
-  void handleDebugDumpData(const ::chre::fbs::DebugDumpDataT &data) override;
-  void handleDebugDumpResponse(
-      const ::chre::fbs::DebugDumpResponseT &response) override;
-
  private:
-  ::android::chre::SocketClient mClient;
+  class SocketCallbacks : public ::android::chre::SocketClient::ICallbacks,
+                          public ::android::chre::IChreMessageHandlers {
+   public:
+    explicit SocketCallbacks(HalChreSocketConnection &parent,
+                             IChreSocketCallback *callback);
+
+    void onMessageReceived(const void *data, size_t length) override;
+    void onConnected() override;
+    void onDisconnected() override;
+    void handleNanoappMessage(
+        const ::chre::fbs::NanoappMessageT &message) override;
+    void handleHubInfoResponse(
+        const ::chre::fbs::HubInfoResponseT &response) override;
+    void handleNanoappListResponse(
+        const ::chre::fbs::NanoappListResponseT &response) override;
+    void handleLoadNanoappResponse(
+        const ::chre::fbs::LoadNanoappResponseT &response) override;
+    void handleUnloadNanoappResponse(
+        const ::chre::fbs::UnloadNanoappResponseT &response) override;
+    void handleDebugDumpData(const ::chre::fbs::DebugDumpDataT &data) override;
+    void handleDebugDumpResponse(
+        const ::chre::fbs::DebugDumpResponseT &response) override;
+
+   private:
+    HalChreSocketConnection &mParent;
+    IChreSocketCallback *mCallback = nullptr;
+    bool mHaveConnected = false;
+
+#ifdef CHRE_HAL_SOCKET_METRICS_ENABLED
+    long mLastClearedTimestamp = 0;
+    static constexpr uint32_t kOneDayinMillis = 24 * 60 * 60 * 1000;
+    static constexpr uint16_t kMaxDailyReportedApWakeUp = 200;
+    uint16_t mNanoappWokeUpCount = 0;
+    std::mutex mNanoappWokeApCountMutex;
+#endif  // CHRE_HAL_SOCKET_METRICS_ENABLED
+  };
 
-  IChreSocketCallback *mCallback = nullptr;
+  sp<SocketCallbacks> mSocketCallbacks;
+
+  ::android::chre::SocketClient mClient;
 
   ::chre::fbs::HubInfoResponseT mHubInfoResponse;
   bool mHubInfoValid = false;
@@ -154,16 +172,6 @@ class HalChreSocketConnection
   std::optional<chre::FragmentedLoadTransaction> mPendingLoadTransaction;
   std::mutex mPendingLoadTransactionMutex;
 
-  bool mHaveConnected = false;
-
-#ifdef CHRE_HAL_SOCKET_METRICS_ENABLED
-  long mLastClearedTimestamp = 0;
-  static constexpr uint32_t kOneDayinMillis = 24 * 60 * 60 * 1000;
-  static constexpr uint16_t kMaxDailyReportedApWakeUp = 200;
-  uint16_t mNanoappWokeUpCount = 0;
-  std::mutex mNanoappWokeApCountMutex;
-#endif  // CHRE_HAL_SOCKET_METRICS_ENABLED
-
   /**
    * Checks to see if a load response matches the currently pending
    * fragmented load transaction. mPendingLoadTransactionMutex must