diff options
author | destradaa <destradaa@google.com> | 2015-01-15 18:36:01 -0800 |
---|---|---|
committer | destradaa <destradaa@google.com> | 2015-01-23 10:26:17 -0800 |
commit | 13a60b0d41c740448ea39ca19842c7b193c61efd (patch) | |
tree | 2ea5b94cf67b90031101e5a4d8963aa9334719c3 | |
parent | 34efbcedac4157b1e92fcd8fd746ba2754b44858 (diff) | |
download | frameworks_base-13a60b0d41c740448ea39ca19842c7b193c61efd.zip frameworks_base-13a60b0d41c740448ea39ca19842c7b193c61efd.tar.gz frameworks_base-13a60b0d41c740448ea39ca19842c7b193c61efd.tar.bz2 |
Fix race condition generating READY and NOT_SUPPORTED statuses.
The race condition only affects when the client registers for several (all) location listeners.
And the side efects are benign: only the measurement and navigation message status are incurrectly
being sent to the application, but there are no crashes or any real data from GPS being
misscommunicated.
Also:
- cache the last reported status to filter sending notifications when no changes have occurred
- do some cleanup and refactoring in the code changed
Change-Id: I0692e6b70847dc1ee092d7a05a2c6ba3cd9fa147
6 files changed, 99 insertions, 77 deletions
diff --git a/location/java/android/location/GpsNavigationMessageEvent.java b/location/java/android/location/GpsNavigationMessageEvent.java index bd6921c..7452721 100644 --- a/location/java/android/location/GpsNavigationMessageEvent.java +++ b/location/java/android/location/GpsNavigationMessageEvent.java @@ -36,18 +36,18 @@ public class GpsNavigationMessageEvent implements Parcelable { * The system does not support tracking of GPS Navigation Messages. This status will not change * in the future. */ - public static int STATUS_NOT_SUPPORTED = 0; + public static final int STATUS_NOT_SUPPORTED = 0; /** * GPS Navigation Messages are successfully being tracked, it will receive updates once they are * available. */ - public static int STATUS_READY = 1; + public static final int STATUS_READY = 1; /** * GPS provider or Location is disabled, updated will not be received until they are enabled. */ - public static int STATUS_GPS_LOCATION_DISABLED = 2; + public static final int STATUS_GPS_LOCATION_DISABLED = 2; private final GpsNavigationMessage mNavigationMessage; diff --git a/services/core/java/com/android/server/location/GpsLocationProvider.java b/services/core/java/com/android/server/location/GpsLocationProvider.java index 2a1f7d6..e41b3da 100644 --- a/services/core/java/com/android/server/location/GpsLocationProvider.java +++ b/services/core/java/com/android/server/location/GpsLocationProvider.java @@ -25,7 +25,6 @@ import com.android.internal.location.ProviderRequest; import com.android.internal.R; import com.android.internal.telephony.Phone; import com.android.internal.telephony.PhoneConstants; -import com.android.internal.telephony.TelephonyIntents; import android.app.AlarmManager; import android.app.AppOpsManager; @@ -72,7 +71,6 @@ import android.provider.Settings; import android.provider.Telephony.Carriers; import android.provider.Telephony.Sms.Intents; import android.telephony.SmsMessage; -import android.telephony.SubscriptionInfo; import android.telephony.SubscriptionManager; import android.telephony.SubscriptionManager.OnSubscriptionsChangedListener; import android.telephony.TelephonyManager; @@ -91,7 +89,6 @@ import java.io.StringReader; import java.net.InetAddress; import java.net.UnknownHostException; import java.util.Date; -import java.util.List; import java.util.Map.Entry; import java.util.Properties; @@ -395,7 +392,7 @@ public class GpsLocationProvider implements LocationProviderInterface { private final IGpsStatusProvider mGpsStatusProvider = new IGpsStatusProvider.Stub() { @Override - public void addGpsStatusListener(IGpsStatusListener listener) throws RemoteException { + public void addGpsStatusListener(IGpsStatusListener listener) { mListenerHelper.addListener(listener); } @@ -681,7 +678,7 @@ public class GpsLocationProvider implements LocationProviderInterface { mListenerHelper = new GpsStatusListenerHelper(mHandler) { @Override protected boolean isAvailableInPlatform() { - return GpsLocationProvider.isSupported(); + return isSupported(); } @Override @@ -1027,6 +1024,9 @@ public class GpsLocationProvider implements LocationProviderInterface { if (mC2KServerHost != null) { native_set_agps_server(AGPS_TYPE_C2K, mC2KServerHost, mC2KServerPort); } + + mGpsMeasurementsProvider.onGpsEnabledChanged(); + mGpsNavigationMessageProvider.onGpsEnabledChanged(); } else { synchronized (mLock) { mEnabled = false; @@ -1060,6 +1060,9 @@ public class GpsLocationProvider implements LocationProviderInterface { // do this before releasing wakelock native_cleanup(); + + mGpsMeasurementsProvider.onGpsEnabledChanged(); + mGpsNavigationMessageProvider.onGpsEnabledChanged(); } @Override @@ -1479,9 +1482,7 @@ public class GpsLocationProvider implements LocationProviderInterface { } if (wasNavigating != mNavigating) { - mListenerHelper.onGpsEnabledChanged(mNavigating); - mGpsMeasurementsProvider.onGpsEnabledChanged(mNavigating); - mGpsNavigationMessageProvider.onGpsEnabledChanged(mNavigating); + mListenerHelper.onStatusChanged(mNavigating); // send an intent to notify that the GPS has been enabled or disabled Intent intent = new Intent(LocationManager.GPS_ENABLED_CHANGE_ACTION); diff --git a/services/core/java/com/android/server/location/GpsMeasurementsProvider.java b/services/core/java/com/android/server/location/GpsMeasurementsProvider.java index 0514e0c..b327ca2 100644 --- a/services/core/java/com/android/server/location/GpsMeasurementsProvider.java +++ b/services/core/java/com/android/server/location/GpsMeasurementsProvider.java @@ -33,7 +33,7 @@ public abstract class GpsMeasurementsProvider extends RemoteListenerHelper<IGpsMeasurementsListener> { private static final String TAG = "GpsMeasurementsProvider"; - public GpsMeasurementsProvider(Handler handler) { + protected GpsMeasurementsProvider(Handler handler) { super(handler, TAG); } @@ -49,15 +49,19 @@ public abstract class GpsMeasurementsProvider } public void onCapabilitiesUpdated(boolean isGpsMeasurementsSupported) { - int status = isGpsMeasurementsSupported ? - GpsMeasurementsEvent.STATUS_READY : - GpsMeasurementsEvent.STATUS_NOT_SUPPORTED; - setSupported(isGpsMeasurementsSupported, new StatusChangedOperation(status)); + setSupported(isGpsMeasurementsSupported); + updateResult(); + } + + public void onGpsEnabledChanged() { + if (tryUpdateRegistrationWithService()) { + updateResult(); + } } @Override protected ListenerOperation<IGpsMeasurementsListener> getHandlerOperation(int result) { - final int status; + int status; switch (result) { case RESULT_SUCCESS: status = GpsMeasurementsEvent.STATUS_READY; @@ -70,6 +74,8 @@ public abstract class GpsMeasurementsProvider case RESULT_GPS_LOCATION_DISABLED: status = GpsMeasurementsEvent.STATUS_GPS_LOCATION_DISABLED; break; + case RESULT_UNKNOWN: + return null; default: Log.v(TAG, "Unhandled addListener result: " + result); return null; @@ -77,15 +83,8 @@ public abstract class GpsMeasurementsProvider return new StatusChangedOperation(status); } - @Override - protected void handleGpsEnabledChanged(boolean enabled) { - int status = enabled ? - GpsMeasurementsEvent.STATUS_READY : - GpsMeasurementsEvent.STATUS_GPS_LOCATION_DISABLED; - foreach(new StatusChangedOperation(status)); - } - - private class StatusChangedOperation implements ListenerOperation<IGpsMeasurementsListener> { + private static class StatusChangedOperation + implements ListenerOperation<IGpsMeasurementsListener> { private final int mStatus; public StatusChangedOperation(int status) { diff --git a/services/core/java/com/android/server/location/GpsNavigationMessageProvider.java b/services/core/java/com/android/server/location/GpsNavigationMessageProvider.java index 13d22fc..e6bbe56 100644 --- a/services/core/java/com/android/server/location/GpsNavigationMessageProvider.java +++ b/services/core/java/com/android/server/location/GpsNavigationMessageProvider.java @@ -33,7 +33,7 @@ public abstract class GpsNavigationMessageProvider extends RemoteListenerHelper<IGpsNavigationMessageListener> { private static final String TAG = "GpsNavigationMessageProvider"; - public GpsNavigationMessageProvider(Handler handler) { + protected GpsNavigationMessageProvider(Handler handler) { super(handler, TAG); } @@ -50,15 +50,19 @@ public abstract class GpsNavigationMessageProvider } public void onCapabilitiesUpdated(boolean isGpsNavigationMessageSupported) { - int status = isGpsNavigationMessageSupported ? - GpsNavigationMessageEvent.STATUS_READY : - GpsNavigationMessageEvent.STATUS_NOT_SUPPORTED; - setSupported(isGpsNavigationMessageSupported, new StatusChangedOperation(status)); + setSupported(isGpsNavigationMessageSupported); + updateResult(); + } + + public void onGpsEnabledChanged() { + if (tryUpdateRegistrationWithService()) { + updateResult(); + } } @Override protected ListenerOperation<IGpsNavigationMessageListener> getHandlerOperation(int result) { - final int status; + int status; switch (result) { case RESULT_SUCCESS: status = GpsNavigationMessageEvent.STATUS_READY; @@ -71,6 +75,8 @@ public abstract class GpsNavigationMessageProvider case RESULT_GPS_LOCATION_DISABLED: status = GpsNavigationMessageEvent.STATUS_GPS_LOCATION_DISABLED; break; + case RESULT_UNKNOWN: + return null; default: Log.v(TAG, "Unhandled addListener result: " + result); return null; @@ -78,15 +84,7 @@ public abstract class GpsNavigationMessageProvider return new StatusChangedOperation(status); } - @Override - protected void handleGpsEnabledChanged(boolean enabled) { - int status = enabled ? - GpsNavigationMessageEvent.STATUS_READY : - GpsNavigationMessageEvent.STATUS_GPS_LOCATION_DISABLED; - foreach(new StatusChangedOperation(status)); - } - - private class StatusChangedOperation + private static class StatusChangedOperation implements ListenerOperation<IGpsNavigationMessageListener> { private final int mStatus; diff --git a/services/core/java/com/android/server/location/GpsStatusListenerHelper.java b/services/core/java/com/android/server/location/GpsStatusListenerHelper.java index 376b4a5..53ff6c2 100644 --- a/services/core/java/com/android/server/location/GpsStatusListenerHelper.java +++ b/services/core/java/com/android/server/location/GpsStatusListenerHelper.java @@ -24,14 +24,9 @@ import android.os.RemoteException; * Implementation of a handler for {@link IGpsStatusListener}. */ abstract class GpsStatusListenerHelper extends RemoteListenerHelper<IGpsStatusListener> { - public GpsStatusListenerHelper(Handler handler) { + protected GpsStatusListenerHelper(Handler handler) { super(handler, "GpsStatusListenerHelper"); - - Operation nullOperation = new Operation() { - @Override - public void execute(IGpsStatusListener iGpsStatusListener) throws RemoteException {} - }; - setSupported(GpsLocationProvider.isSupported(), nullOperation); + setSupported(GpsLocationProvider.isSupported()); } @Override @@ -47,10 +42,9 @@ abstract class GpsStatusListenerHelper extends RemoteListenerHelper<IGpsStatusLi return null; } - @Override - protected void handleGpsEnabledChanged(boolean enabled) { + public void onStatusChanged(boolean isNavigating) { Operation operation; - if (enabled) { + if (isNavigating) { operation = new Operation() { @Override public void execute(IGpsStatusListener listener) throws RemoteException { @@ -114,5 +108,5 @@ abstract class GpsStatusListenerHelper extends RemoteListenerHelper<IGpsStatusLi foreach(operation); } - private abstract class Operation implements ListenerOperation<IGpsStatusListener> { } + private interface Operation extends ListenerOperation<IGpsStatusListener> {} } diff --git a/services/core/java/com/android/server/location/RemoteListenerHelper.java b/services/core/java/com/android/server/location/RemoteListenerHelper.java index 402b601..ec2828b 100644 --- a/services/core/java/com/android/server/location/RemoteListenerHelper.java +++ b/services/core/java/com/android/server/location/RemoteListenerHelper.java @@ -26,26 +26,31 @@ import android.os.RemoteException; import android.util.Log; import java.util.HashMap; +import java.util.Map; /** * A helper class, that handles operations in remote listeners, and tracks for remote process death. */ abstract class RemoteListenerHelper<TListener extends IInterface> { + protected static final int RESULT_SUCCESS = 0; protected static final int RESULT_NOT_AVAILABLE = 1; protected static final int RESULT_NOT_SUPPORTED = 2; protected static final int RESULT_GPS_LOCATION_DISABLED = 3; protected static final int RESULT_INTERNAL_ERROR = 4; + protected static final int RESULT_UNKNOWN = 5; private final Handler mHandler; private final String mTag; - private final HashMap<IBinder, LinkedListener> mListenerMap = new HashMap<>(); + private final Map<IBinder, LinkedListener> mListenerMap = new HashMap<>(); private boolean mIsRegistered; private boolean mHasIsSupported; private boolean mIsSupported; + private int mLastReportedResult = RESULT_UNKNOWN; + protected RemoteListenerHelper(Handler handler, String name) { Preconditions.checkNotNull(name); mHandler = handler; @@ -110,33 +115,11 @@ abstract class RemoteListenerHelper<TListener extends IInterface> { } } - public void onGpsEnabledChanged(boolean enabled) { - // handle first the sub-class implementation, so any error in registration can take - // precedence - handleGpsEnabledChanged(enabled); - synchronized (mListenerMap) { - if (!enabled) { - tryUnregister(); - return; - } - if (mListenerMap.isEmpty()) { - return; - } - if (tryRegister()) { - // registration was successful, there is no need to update the state - return; - } - ListenerOperation<TListener> operation = getHandlerOperation(RESULT_INTERNAL_ERROR); - foreachUnsafe(operation); - } - } - protected abstract boolean isAvailableInPlatform(); protected abstract boolean isGpsEnabled(); protected abstract boolean registerWithService(); protected abstract void unregisterFromService(); protected abstract ListenerOperation<TListener> getHandlerOperation(int result); - protected abstract void handleGpsEnabledChanged(boolean enabled); protected interface ListenerOperation<TListener extends IInterface> { void execute(TListener listener) throws RemoteException; @@ -148,11 +131,40 @@ abstract class RemoteListenerHelper<TListener extends IInterface> { } } - protected void setSupported(boolean value, ListenerOperation<TListener> notifier) { + protected void setSupported(boolean value) { synchronized (mListenerMap) { mHasIsSupported = true; mIsSupported = value; - foreachUnsafe(notifier); + } + } + + protected boolean tryUpdateRegistrationWithService() { + synchronized (mListenerMap) { + if (!isGpsEnabled()) { + tryUnregister(); + return true; + } + if (mListenerMap.isEmpty()) { + return true; + } + if (tryRegister()) { + // registration was successful, there is no need to update the state + return true; + } + ListenerOperation<TListener> operation = getHandlerOperation(RESULT_INTERNAL_ERROR); + foreachUnsafe(operation); + return false; + } + } + + protected void updateResult() { + synchronized (mListenerMap) { + int newResult = calculateCurrentResultUnsafe(); + if (mLastReportedResult == newResult) { + return; + } + foreachUnsafe(getHandlerOperation(newResult)); + mLastReportedResult = newResult; } } @@ -183,6 +195,24 @@ abstract class RemoteListenerHelper<TListener extends IInterface> { mIsRegistered = false; } + private int calculateCurrentResultUnsafe() { + // update statuses we already know about, starting from the ones that will never change + if (!isAvailableInPlatform()) { + return RESULT_NOT_AVAILABLE; + } + if (!mHasIsSupported || mListenerMap.isEmpty()) { + // we'll update once we have a supported status available + return RESULT_UNKNOWN; + } + if (!mIsSupported) { + return RESULT_NOT_SUPPORTED; + } + if (!isGpsEnabled()) { + return RESULT_GPS_LOCATION_DISABLED; + } + return RESULT_SUCCESS; + } + private class LinkedListener implements IBinder.DeathRecipient { private final TListener mListener; |