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

5 files changed, 96 insertions, 74 deletions

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;