Merge "Fix possible deadlock"

2 files changed, 48 insertions, 36 deletions

diff --git a/services/core/java/com/android/server/ServiceWatcher.java b/services/core/java/com/android/server/ServiceWatcher.java
index 14cd3a5d5c0d..b43ae36c7ef5 100644
--- a/services/core/java/com/android/server/ServiceWatcher.java
+++ b/services/core/java/com/android/server/ServiceWatcher.java
@@ -49,6 +49,8 @@ import android.util.Log;
 import com.android.internal.content.PackageMonitor;
 import com.android.internal.util.Preconditions;
 
+import java.io.FileDescriptor;
+import java.io.PrintWriter;
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.Callable;
@@ -183,9 +185,7 @@ public class ServiceWatcher implements ServiceConnection {
 
     // write from handler thread only, read anywhere
     private volatile ServiceInfo mServiceInfo;
-
-    // read/write from handler thread only
-    private IBinder mBinder;
+    private volatile IBinder mBinder;
 
     public ServiceWatcher(Context context, Handler handler, String action,
             @Nullable BinderRunner onBind, @Nullable Runnable onUnbind,
@@ -345,20 +345,25 @@ public class ServiceWatcher implements ServiceConnection {
         }
 
         mBinder = binder;
+
+        // we always run the on bind callback even if we know that the binder is dead already so
+        // that there are always balance pairs of bind/unbind callbacks
         if (mOnBind != null) {
-            runOnBinder(mOnBind);
+            try {
+                mOnBind.run(binder);
+            } catch (RuntimeException | RemoteException e) {
+                // binders may propagate some specific non-RemoteExceptions from the other side
+                // through the binder as well - we cannot allow those to crash the system server
+                Log.e(TAG, getLogPrefix() + " exception running on " + mServiceInfo, e);
+            }
         }
-    }
 
-    @Override
-    public void onBindingDied(ComponentName component) {
-        Preconditions.checkState(Looper.myLooper() == mHandler.getLooper());
-
-        if (D) {
-            Log.i(TAG, getLogPrefix() + " " + component.toShortString() + " died");
+        try {
+            // setting the binder to null lets us skip queued transactions
+            binder.linkToDeath(() -> mBinder = null, 0);
+        } catch (RemoteException e) {
+            mBinder = null;
         }
-
-        onBestServiceChanged(true);
     }
 
     @Override
@@ -375,6 +380,17 @@ public class ServiceWatcher implements ServiceConnection {
         }
     }
 
+    @Override
+    public void onBindingDied(ComponentName component) {
+        Preconditions.checkState(Looper.myLooper() == mHandler.getLooper());
+
+        if (D) {
+            Log.i(TAG, getLogPrefix() + " " + component.toShortString() + " died");
+        }
+
+        onBestServiceChanged(true);
+    }
+
     private void onUserSwitched(@UserIdInt int userId) {
         mCurrentUserId = userId;
         onBestServiceChanged(false);
@@ -398,7 +414,7 @@ public class ServiceWatcher implements ServiceConnection {
      * RemoteException thrown during execution.
      */
     public final void runOnBinder(BinderRunner runner) {
-        runOnHandler(() -> {
+        mHandler.post(() -> {
             if (mBinder == null) {
                 return;
             }
@@ -447,14 +463,6 @@ public class ServiceWatcher implements ServiceConnection {
         }
     }
 
-    private void runOnHandler(Runnable r) {
-        if (Looper.myLooper() == mHandler.getLooper()) {
-            r.run();
-        } else {
-            mHandler.post(r);
-        }
-    }
-
     private <T> T runOnHandlerBlocking(Callable<T> c)
             throws InterruptedException, TimeoutException {
         if (Looper.myLooper() == mHandler.getLooper()) {
@@ -489,4 +497,12 @@ public class ServiceWatcher implements ServiceConnection {
     public String toString() {
         return mServiceInfo.toString();
     }
+
+    /**
+     * Dump for debugging.
+     */
+    public void dump(FileDescriptor fd, PrintWriter pw, String[] args) {
+        pw.println("service=" + mServiceInfo);
+        pw.println("connected=" + (mBinder != null));
+    }
 }
diff --git a/services/core/java/com/android/server/location/LocationProviderProxy.java b/services/core/java/com/android/server/location/LocationProviderProxy.java
index 19fb6694bbb5..96ffaa6c0bff 100644
--- a/services/core/java/com/android/server/location/LocationProviderProxy.java
+++ b/services/core/java/com/android/server/location/LocationProviderProxy.java
@@ -139,13 +139,13 @@ public class LocationProviderProxy extends AbstractLocationProvider {
 
     @GuardedBy("mLock")
     private boolean mBound;
-    @GuardedBy("mLock")
-    private ProviderRequest mRequest;
+
+    private volatile ProviderRequest mRequest;
 
     private LocationProviderProxy(Context context, String action, int enableOverlayResId,
             int nonOverlayPackageResId) {
-        // safe to use direct executor even though this class has internal locks - all of our
-        // callbacks go to oneway binder transactions which cannot possibly be re-entrant
+        // safe to use direct executor since our locks are not acquired in a code path invoked by
+        // our owning provider
         super(DIRECT_EXECUTOR, Collections.emptySet());
 
         mContext = context;
@@ -167,8 +167,10 @@ public class LocationProviderProxy extends AbstractLocationProvider {
             mBound = true;
 
             provider.setLocationProviderManager(mManager);
-            if (!mRequest.equals(ProviderRequest.EMPTY_REQUEST)) {
-                provider.setRequest(mRequest, mRequest.workSource);
+
+            ProviderRequest request = mRequest;
+            if (!request.equals(ProviderRequest.EMPTY_REQUEST)) {
+                provider.setRequest(request, request.workSource);
             }
 
             ComponentName service = mServiceWatcher.getBoundService().component;
@@ -187,10 +189,7 @@ public class LocationProviderProxy extends AbstractLocationProvider {
 
     @Override
     public void onSetRequest(ProviderRequest request) {
-        synchronized (mLock) {
-            mRequest = request;
-        }
-
+        mRequest = request;
         mServiceWatcher.runOnBinder(binder -> {
             ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
             service.setRequest(request, request.workSource);
@@ -215,9 +214,6 @@ public class LocationProviderProxy extends AbstractLocationProvider {
 
     @Override
     public void dump(FileDescriptor fd, PrintWriter pw, String[] args) {
-        pw.println("service=" + mServiceWatcher);
-        synchronized (mLock) {
-            pw.println("bound=" + mBound);
-        }
+        mServiceWatcher.dump(fd, pw, args);
     }
 }