logcat: Fix deadlock

LogCatReceiverFactory has a deadlock scenario that occurs like this:

1. When a device is disconnected, removeReceiverFor() is called from
   a DDMS device monitor thread. This thread holds certain DDMS internal
   locks. removeReceiverFor() is synchronized on LogCatReceiverFactory.
   So we have:
            DDMS thread - holds device lock
            removeReceiverFor - waiting to obtain lock for "this" object.
2. At the same time, a separate thread could call newReceiver.
   As part of creating a new receiver, we add a device change listener.
   So we have:
            newReceiver - holds "this" object.
            create device change listener - waiting for device lock.

As a result, the system deadlocks. This patch fixes this condition by
calling the removeReceiverFor from a non DDMS thread since we don't
really care about DDMS locks here.

Change-Id: Ic4ef4589acdcd7980864da23cde760debc4cfbb8

1 files changed, 29 insertions, 3 deletions

diff --git a/ddms/libs/ddmuilib/src/com/android/ddmuilib/logcat/LogCatReceiverFactory.java b/ddms/libs/ddmuilib/src/com/android/ddmuilib/logcat/LogCatReceiverFactory.java
index 5617988..5b25e17 100644
--- a/ddms/libs/ddmuilib/src/com/android/ddmuilib/logcat/LogCatReceiverFactory.java
+++ b/ddms/libs/ddmuilib/src/com/android/ddmuilib/logcat/LogCatReceiverFactory.java
@@ -38,8 +38,20 @@ public class LogCatReceiverFactory {
     private LogCatReceiverFactory() {
         AndroidDebugBridge.addDeviceChangeListener(new IDeviceChangeListener() {
             @Override
-            public void deviceDisconnected(IDevice device) {
-                removeReceiverFor(device);
+            public void deviceDisconnected(final IDevice device) {
+                // The deviceDisconnected() is called from DDMS code that holds
+                // multiple locks regarding list of clients, etc.
+                // It so happens that #newReceiver() below adds a clientChangeListener
+                // which requires those locks as well. So if we call
+                // #removeReceiverFor from a DDMS/Monitor thread, we could end up
+                // in a deadlock. As a result, we spawn a separate thread that
+                // doesn't hold any of the DDMS locks to remove the receiver.
+                Thread t = new Thread(new Runnable() {
+                        @Override
+                        public void run() {
+                            removeReceiverFor(device);                        }
+                    }, "Remove logcat receiver for " + device.getSerialNumber());
+                t.start();
             }
 
             @Override
@@ -52,8 +64,22 @@ public class LogCatReceiverFactory {
         });
     }
 
+    /**
+     * Remove existing logcat receivers. This method should not be called from a DDMS thread
+     * context that might be holding locks. Doing so could result in a deadlock with the following
+     * two threads locked up: <ul>
+     * <li> {@link #removeReceiverFor(IDevice)} waiting to lock {@link LogCatReceiverFactory},
+     * while holding a DDMS monitor internal lock. </li>
+     * <li> {@link #newReceiver(IDevice, IPreferenceStore)} holding {@link LogCatReceiverFactory}
+     * while attempting to obtain a DDMS monitor lock. </li>
+     * </ul>
+     */
     private synchronized void removeReceiverFor(IDevice device) {
-        mReceiverCache.remove(device.getSerialNumber());
+        LogCatReceiver r = mReceiverCache.get(device.getSerialNumber());
+        if (r != null) {
+            r.stop();
+            mReceiverCache.remove(device.getSerialNumber());
+        }
     }
 
     public synchronized LogCatReceiver newReceiver(IDevice device, IPreferenceStore prefs) {