From d535ead295abf3b74afb6450c83bd197f69c5a26 Mon Sep 17 00:00:00 2001 From: Zhentao Sun Date: Thu, 29 Aug 2013 14:43:35 -0700 Subject: [PATCH] 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 --- .../com/android/server/location/GeofenceProxy.java | 75 +++++++++++++--------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/services/java/com/android/server/location/GeofenceProxy.java b/services/java/com/android/server/location/GeofenceProxy.java index a86c92379d43..bbc1f472387f 100644 --- 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; } } -- 2.11.0