Unregister for notifications when we're done waiting.

Otherwise we're stuck with a strong refcount we can't remove:
1) We hold a sp<> to the Waiter
2) The waiter sends itself to hwservicemanager for notifications,
   hwservicemanager then holds an sp<> to the Waiter
3) We drop our sp<> and no longer have a strong ref,
   object remains alive until hwservicemanager drops it

To prevent this, explicitly unregister for notifications when
we're done waiting, so hwservicemanager will drop its strong
ref and the object can be safely destructed.

Bug: 73121807
Test: verified Waiter objects get destructed
Change-Id: I59403a3e962364e88378839b984f602fefeea3bb

1 files changed, 25 insertions, 7 deletions

diff --git a/transport/ServiceManagement.cpp b/transport/ServiceManagement.cpp
index 4bdeb6e..e3c19e5 100644
--- a/transport/ServiceManagement.cpp
+++ b/transport/ServiceManagement.cpp
@@ -511,12 +511,9 @@ struct Waiter : IServiceNotification {
     }
 
     ~Waiter() {
-        if (mRegisteredForNotifications) {
-            if (!mSm->unregisterForNotifications(mInterfaceName, mInstanceName, this).
-                    withDefault(false)) {
-                LOG(ERROR) << "Could not unregister service notification for "
-                    << mInterfaceName << "/" << mInstanceName << ".";
-            }
+        if (!mDoneCalled) {
+            LOG(FATAL)
+                << "Waiter still registered for notifications, call done() before dropping ref!";
         }
     }
 
@@ -567,7 +564,23 @@ struct Waiter : IServiceNotification {
         std::unique_lock<std::mutex> lock(mMutex);
         mRegistered = false;
     }
-private:
+
+    // done() must be called before dropping the last strong ref to the Waiter, to make
+    // sure we can properly unregister with hwservicemanager.
+    void done() {
+        if (mRegisteredForNotifications) {
+            if (!mSm->unregisterForNotifications(mInterfaceName, mInstanceName, this)
+                     .withDefault(false)) {
+                LOG(ERROR) << "Could not unregister service notification for " << mInterfaceName
+                           << "/" << mInstanceName << ".";
+            } else {
+                mRegisteredForNotifications = false;
+            }
+        }
+        mDoneCalled = true;
+    }
+
+   private:
     const std::string mInterfaceName;
     const std::string mInstanceName;
     const sp<IServiceManager1_1>& mSm;
@@ -575,12 +588,14 @@ private:
     std::condition_variable mCondition;
     bool mRegistered = false;
     bool mRegisteredForNotifications = false;
+    bool mDoneCalled = false;
 };
 
 void waitForHwService(
         const std::string &interface, const std::string &instanceName) {
     sp<Waiter> waiter = new Waiter(interface, instanceName, defaultServiceManager1_1());
     waiter->wait();
+    waiter->done();
 }
 
 // Prints relevant error/warning messages for error return values from
@@ -670,6 +685,7 @@ sp<::android::hidl::base::V1_0::IBase> getRawServiceInternal(const std::string&
                 details::canCastInterface(base.get(), descriptor.c_str(), true /* emitError */);
 
             if (canCastRet.isOk() && canCastRet) {
+                waiter->done();
                 return base; // still needs to be wrapped by Bp class.
             }
 
@@ -683,6 +699,8 @@ sp<::android::hidl::base::V1_0::IBase> getRawServiceInternal(const std::string&
         waiter->wait();
     }
 
+    waiter->done();
+
     if (getStub || vintfPassthru || vintfLegacy) {
         const sp<IServiceManager> pm = getPassthroughServiceManager();
         if (pm != nullptr) {