commit 3fa15c2679039147959cc7717cc3dccc1e747e11
author Martijn Coenen <maco@google.com> Sat Feb 10 11:30:00 2018 +0100
committer Martijn Coenen <maco@google.com> Tue Feb 13 20:36:40 2018 +0100
tree 5f6e5b7d7daa1a1637b236891f93bec4710c46f1
parent 0288c9a89f9c06414a3ccee926bb075dd303e52a

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

transport/ServiceManagement.cpp

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 @@
     }
 
     ~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 @@
         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 @@
     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 @@
                 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 @@
         waiter->wait();
     }
 
+    waiter->done();
+
     if (getStub || vintfPassthru || vintfLegacy) {
         const sp<IServiceManager> pm = getPassthroughServiceManager();
         if (pm != nullptr) {