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
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.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.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.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 @@
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 @@
mListenerHelper = new GpsStatusListenerHelper(mHandler) {
@Override
protected boolean isAvailableInPlatform() {
- return GpsLocationProvider.isSupported();
+ return isSupported();
}
@Override
@@ -1027,6 +1024,9 @@
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 @@
// do this before releasing wakelock
native_cleanup();
+
+ mGpsMeasurementsProvider.onGpsEnabledChanged();
+ mGpsNavigationMessageProvider.onGpsEnabledChanged();
}
@Override
@@ -1479,9 +1482,7 @@
}
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 @@
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 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 @@
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 @@
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 @@
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 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 @@
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 @@
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 @@
* 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 @@
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 @@
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.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 @@
}
}
- 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 @@
}
}
- 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 @@
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;