Fixed b/10512887.

This is an issue caused by multi-threading. If geofence provider service
is connected and disconnected immediately, the ServiceWatcher can return a
null service handle to the private thread used by GeofenceProxy, and
this can cause NPE and system crash.
This CL also fixed a hidden race conditions bug where mGeofenceHardware
is not synchronized between two threads.

Change-Id: I824642cd638fbb1e6799a5a1220b047ebc2556a1

diff --git a/services/java/com/android/server/location/GeofenceProxy.java b/services/java/com/android/server/location/GeofenceProxy.java
index a86c923..bbc1f47 100644 (file)
--- a/services/java/com/android/server/location/GeofenceProxy.java
+++ b/services/java/com/android/server/location/GeofenceProxy.java
@@ -41,11 +41,15 @@ public final class GeofenceProxy {
     private static final String TAG = "GeofenceProxy";
     private static final String SERVICE_ACTION =
             "com.android.location.service.GeofenceProvider";
-    private ServiceWatcher mServiceWatcher;
-    private Context mContext;
+    private final ServiceWatcher mServiceWatcher;
+    private final Context mContext;
+    private final IGpsGeofenceHardware mGpsGeofenceHardware;
+    private final IFusedGeofenceHardware mFusedGeofenceHardware;
+
+    private final Object mLock = new Object();
+
+    // Access to mGeofenceHardware needs to be synchronized by mLock.
     private IGeofenceHardware mGeofenceHardware;
-    private IGpsGeofenceHardware mGpsGeofenceHardware;
-    private IFusedGeofenceHardware mFusedGeofenceHardware;
 
     private static final int GEOFENCE_PROVIDER_CONNECTED = 1;
     private static final int GEOFENCE_HARDWARE_CONNECTED = 2;
@@ -90,10 +94,6 @@ public final class GeofenceProxy {
         return mServiceWatcher.start();
     }
 
-    private IGeofenceProvider getGeofenceProviderService() {
-        return IGeofenceProvider.Stub.asInterface(mServiceWatcher.getBinder());
-    }
-
     private void bindHardwareGeofence() {
         mContext.bindServiceAsUser(new Intent(mContext, GeofenceHardwareService.class),
                 mServiceConnection, Context.BIND_AUTO_CREATE, UserHandle.OWNER);
@@ -102,26 +102,34 @@ public final class GeofenceProxy {
     private ServiceConnection mServiceConnection = new ServiceConnection() {
         @Override
         public void onServiceConnected(ComponentName name, IBinder service) {
-            mGeofenceHardware = IGeofenceHardware.Stub.asInterface(service);
-            mHandler.sendEmptyMessage(GEOFENCE_HARDWARE_CONNECTED);
+            synchronized (mLock) {
+                mGeofenceHardware = IGeofenceHardware.Stub.asInterface(service);
+                mHandler.sendEmptyMessage(GEOFENCE_HARDWARE_CONNECTED);
+            }
         }
 
         @Override
         public void onServiceDisconnected(ComponentName name) {
-            mGeofenceHardware = null;
-            mHandler.sendEmptyMessage(GEOFENCE_HARDWARE_DISCONNECTED);
+            synchronized (mLock) {
+                mGeofenceHardware = null;
+                mHandler.sendEmptyMessage(GEOFENCE_HARDWARE_DISCONNECTED);
+            }
         }
     };
 
-    private void setGeofenceHardwareInProvider() {
+    private void setGeofenceHardwareInProviderLocked() {
         try {
-            getGeofenceProviderService().setGeofenceHardware(mGeofenceHardware);
+            IGeofenceProvider provider = IGeofenceProvider.Stub.asInterface(
+                      mServiceWatcher.getBinder());
+            if (provider != null) {
+                provider.setGeofenceHardware(mGeofenceHardware);
+            }
         } catch (RemoteException e) {
-            Log.e(TAG, "Remote Exception: setGeofenceHardwareInProvider: " + e);
+            Log.e(TAG, "Remote Exception: setGeofenceHardwareInProviderLocked: " + e);
         }
     }
 
-    private void setGpsGeofence() {
+    private void setGpsGeofenceLocked() {
         try {
             mGeofenceHardware.setGpsGeofenceHardware(mGpsGeofenceHardware);
         } catch (RemoteException e) {
@@ -129,7 +137,7 @@ public final class GeofenceProxy {
         }
     }
 
-    private void setFusedGeofence() {
+    private void setFusedGeofenceLocked() {
         try {
             mGeofenceHardware.setFusedGeofenceHardware(mFusedGeofenceHardware);
         } catch(RemoteException e) {
@@ -140,30 +148,37 @@ public final class GeofenceProxy {
     // This needs to be reworked, when more services get added,
     // Might need a state machine or add a framework utility class,
     private Handler mHandler = new Handler() {
-        private boolean mGeofenceHardwareConnected = false;
-        private boolean mGeofenceProviderConnected = false;
-
 
         @Override
         public void handleMessage(Message msg) {
             switch (msg.what) {
                 case GEOFENCE_PROVIDER_CONNECTED:
-                    mGeofenceProviderConnected = true;
-                    if (mGeofenceHardwareConnected) {
-                        setGeofenceHardwareInProvider();
+                    synchronized (mLock) {
+                        if (mGeofenceHardware != null) {
+                            setGeofenceHardwareInProviderLocked();
+                        }
+                        // else: the geofence provider will be notified when the connection to
+                        // GeofenceHardwareService is established.
                     }
                     break;
                 case GEOFENCE_HARDWARE_CONNECTED:
-                    setGpsGeofence();
-                    setFusedGeofence();
-                    mGeofenceHardwareConnected = true;
-                    if (mGeofenceProviderConnected) {
-                        setGeofenceHardwareInProvider();
+                    synchronized (mLock) {
+                        // Theoretically this won't happen because once the GeofenceHardwareService
+                        // is connected to, we won't lose connection to it because it's a system
+                        // service. But this check does make the code more robust.
+                        if (mGeofenceHardware != null) {
+                            setGpsGeofenceLocked();
+                            setFusedGeofenceLocked();
+                            setGeofenceHardwareInProviderLocked();
+                        }
                     }
                     break;
                 case GEOFENCE_HARDWARE_DISCONNECTED:
-                    mGeofenceHardwareConnected = false;
-                    setGeofenceHardwareInProvider();
+                    synchronized (mLock) {
+                        if (mGeofenceHardware == null) {
+                            setGeofenceHardwareInProviderLocked();
+                        }
+                    }
                     break;
             }
         }