From da4d1c363992fcc38993e8539e2b18f23673da8b Mon Sep 17 00:00:00 2001
From: Soonil Nagarkar <sooniln@google.com>
Date: Thu, 29 Apr 2021 09:53:55 -0700
Subject: Fix cache by userId bugs

Three bugs were present here:

1) isLocationEnabled() will return incorrect results for USER_CURRENT
and USER_CURRENT_OR_SELF
2) Disabling the location enabled cache does not work properly in the
system process, we only disable the cache for a single instance of
LocationManager.
3) isLocationEnabled() was not respecting the user as defined by the
context used to create the LocationManager.

Bug: 185401689
Test: atest CtsLocationFineTestCases
Change-Id: I84bff02604a4b2d84494a0e099e172c4ffb400a2
---
 .../java/android/location/LocationManager.java     | 69 +++++++++++++++-------
 1 file changed, 47 insertions(+), 22 deletions(-)

(limited to 'location/java')

diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java
index c1d672524ac5..5952479da702 100644
--- a/location/java/android/location/LocationManager.java
+++ b/location/java/android/location/LocationManager.java
@@ -360,6 +360,9 @@ public class LocationManager {
         }
     }
 
+    private static volatile LocationEnabledCache sLocationEnabledCache =
+            new LocationEnabledCache(4);
+
     @GuardedBy("sLocationListeners")
     private static final WeakHashMap<LocationListener, WeakReference<LocationListenerTransport>>
             sLocationListeners = new WeakHashMap<>();
@@ -386,20 +389,6 @@ public class LocationManager {
     final Context mContext;
     final ILocationManager mService;
 
-    private volatile PropertyInvalidatedCache<Integer, Boolean> mLocationEnabledCache =
-            new PropertyInvalidatedCache<Integer, Boolean>(
-                    4,
-                    CACHE_KEY_LOCATION_ENABLED_PROPERTY) {
-                @Override
-                protected Boolean recompute(Integer userHandle) {
-                    try {
-                        return mService.isLocationEnabledForUser(userHandle);
-                    } catch (RemoteException e) {
-                        throw e.rethrowFromSystemServer();
-                    }
-                }
-            };
-
     /**
      * @hide
      */
@@ -533,7 +522,7 @@ public class LocationManager {
      * @return true if location is enabled and false if location is disabled.
      */
     public boolean isLocationEnabled() {
-        return isLocationEnabledForUser(Process.myUserHandle());
+        return isLocationEnabledForUser(mContext.getUser());
     }
 
     /**
@@ -546,12 +535,17 @@ public class LocationManager {
      */
     @SystemApi
     public boolean isLocationEnabledForUser(@NonNull UserHandle userHandle) {
-        PropertyInvalidatedCache<Integer, Boolean> cache = mLocationEnabledCache;
-        if (cache != null) {
-            return cache.query(userHandle.getIdentifier());
+        // skip the cache for any "special" user ids - special ids like CURRENT_USER may change
+        // their meaning over time and should never be in the cache. we could resolve the special
+        // user ids here, but that would require an x-process call anyways, and the whole point of
+        // the cache is to avoid x-process calls.
+        if (userHandle.getIdentifier() >= 0) {
+            PropertyInvalidatedCache<Integer, Boolean> cache = sLocationEnabledCache;
+            if (cache != null) {
+                return cache.query(userHandle.getIdentifier());
+            }
         }
 
-        // fallback if cache is disabled
         try {
             return mService.isLocationEnabledForUser(userHandle.getIdentifier());
         } catch (RemoteException e) {
@@ -3004,7 +2998,7 @@ public class LocationManager {
             ListenerExecutor, CancellationSignal.OnCancelListener {
 
         private final Executor mExecutor;
-        private volatile @Nullable Consumer<Location> mConsumer;
+        volatile @Nullable Consumer<Location> mConsumer;
 
         GetCurrentLocationTransport(Executor executor, Consumer<Location> consumer,
                 @Nullable CancellationSignal cancellationSignal) {
@@ -3465,6 +3459,37 @@ public class LocationManager {
         }
     }
 
+    private static class LocationEnabledCache extends PropertyInvalidatedCache<Integer, Boolean> {
+
+        // this is not loaded immediately because this class is created as soon as LocationManager
+        // is referenced for the first time, and within the system server, the ILocationManager
+        // service may not have been loaded yet at that time.
+        private @Nullable ILocationManager mManager;
+
+        LocationEnabledCache(int numEntries) {
+            super(numEntries, CACHE_KEY_LOCATION_ENABLED_PROPERTY);
+        }
+
+        @Override
+        protected Boolean recompute(Integer userId) {
+            Preconditions.checkArgument(userId >= 0);
+
+            if (mManager == null) {
+                try {
+                    mManager = getService();
+                } catch (RemoteException e) {
+                    e.rethrowFromSystemServer();
+                }
+            }
+
+            try {
+                return mManager.isLocationEnabledForUser(userId);
+            } catch (RemoteException e) {
+                throw e.rethrowFromSystemServer();
+            }
+        }
+    }
+
     /**
      * @hide
      */
@@ -3475,7 +3500,7 @@ public class LocationManager {
     /**
      * @hide
      */
-    public void disableLocalLocationEnabledCaches() {
-        mLocationEnabledCache = null;
+    public static void disableLocalLocationEnabledCaches() {
+        sLocationEnabledCache = null;
     }
 }
-- 
cgit v1.2.3-59-g8ed1b