Fix potential deadlock in unregisterStaleHandlers()

The scenario is that a call to unregisterStaleHandlers() is in progress,
and is holding a temporary sp<ALooper> reference to an active ALooper inside
of the loop. At this point the only other remaining external reference to
the ALooper goes away, so the temporary sp<ALooper> in the loop is now
the only reference keeping that object alive. When the loop iterates and
the sp<> goes out of scope, the ALooper destructor is called, which in turn
calls unregisterStaleHandlers again, resulting in a recursive lock.

Bug: 17300093
Change-Id: I116f2ffab4ae7c43b6bcf54a367ae6f9d77c9626

1 files changed, 19 insertions, 7 deletions

diff --git a/media/libstagefright/foundation/ALooperRoster.cpp b/media/libstagefright/foundation/ALooperRoster.cpp
index 0c181ff..0f44b52 100644
--- a/media/libstagefright/foundation/ALooperRoster.cpp
+++ b/media/libstagefright/foundation/ALooperRoster.cpp
@@ -72,15 +72,27 @@ void ALooperRoster::unregisterHandler(ALooper::handler_id handlerID) {
 }
 
 void ALooperRoster::unregisterStaleHandlers() {
-    Mutex::Autolock autoLock(mLock);
 
-    for (size_t i = mHandlers.size(); i-- > 0;) {
-        const HandlerInfo &info = mHandlers.valueAt(i);
+    Vector<sp<ALooper> > activeLoopers;
+    {
+        Mutex::Autolock autoLock(mLock);
 
-        sp<ALooper> looper = info.mLooper.promote();
-        if (looper == NULL) {
-            ALOGV("Unregistering stale handler %d", mHandlers.keyAt(i));
-            mHandlers.removeItemsAt(i);
+        for (size_t i = mHandlers.size(); i-- > 0;) {
+            const HandlerInfo &info = mHandlers.valueAt(i);
+
+            sp<ALooper> looper = info.mLooper.promote();
+            if (looper == NULL) {
+                ALOGV("Unregistering stale handler %d", mHandlers.keyAt(i));
+                mHandlers.removeItemsAt(i);
+            } else {
+                // At this point 'looper' might be the only sp<> keeping
+                // the object alive. To prevent it from going out of scope
+                // and having ~ALooper call this method again recursively
+                // and then deadlocking because of the Autolock above, add
+                // it to a Vector which will go out of scope after the lock
+                // has been released.
+                activeLoopers.add(looper);
+            }
         }
     }
 }