Merge "Fix bug in high power location attribution" into sc-v2-dev am: a8158298fd

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/16358588

Change-Id: I6e26563285c7cdfe6881ad1a9ec9d15bf27f082e

services/core/java/com/android/server/location/gnss/GnssMeasurementsProvider.java

@@ -52,8 +52,6 @@ public final class GnssMeasurementsProvider extends
 
     private class GnssMeasurementListenerRegistration extends GnssListenerRegistration {
 
-        private static final String GNSS_MEASUREMENTS_BUCKET = "gnss_measurement";
-
         protected GnssMeasurementListenerRegistration(
                 @Nullable GnssMeasurementRequest request,
                 CallerIdentity callerIdentity,
@@ -70,15 +68,13 @@ public final class GnssMeasurementsProvider extends
         @Nullable
         @Override
         protected void onActive() {
-            mLocationAttributionHelper.reportHighPowerLocationStart(
+            mLocationAttributionHelper.reportHighPowerLocationStart(getIdentity());
-                    getIdentity(), GNSS_MEASUREMENTS_BUCKET, getKey());
         }
 
         @Nullable
         @Override
         protected void onInactive() {
-            mLocationAttributionHelper.reportHighPowerLocationStop(
+            mLocationAttributionHelper.reportHighPowerLocationStop(getIdentity());
-                    getIdentity(), GNSS_MEASUREMENTS_BUCKET, getKey());
         }
     }
 

services/core/java/com/android/server/location/injector/LocationAttributionHelper.java

@@ -24,55 +24,23 @@ import static com.android.server.location.LocationManagerService.TAG;
 
 import android.location.util.identity.CallerIdentity;
 import android.util.ArrayMap;
-import android.util.ArraySet;
 import android.util.Log;
 
 import com.android.internal.annotations.GuardedBy;
 
 import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
 
 /**
  * Helps manage appop monitoring for multiple location clients.
  */
 public class LocationAttributionHelper {
 
-    private static class BucketKey {
-        private final String mBucket;
-        private final Object mKey;
-
-        private BucketKey(String bucket, Object key) {
-            mBucket = Objects.requireNonNull(bucket);
-            mKey = Objects.requireNonNull(key);
-        }
-
-        @Override
-        public boolean equals(Object o) {
-            if (this == o) {
-                return true;
-            }
-            if (o == null || getClass() != o.getClass()) {
-                return false;
-            }
-
-            BucketKey that = (BucketKey) o;
-            return mBucket.equals(that.mBucket)
-                    && mKey.equals(that.mKey);
-        }
-
-        @Override
-        public int hashCode() {
-            return Objects.hash(mBucket, mKey);
-        }
-    }
-
     private final AppOpsHelper mAppOpsHelper;
 
     @GuardedBy("this")
-    private final Map<CallerIdentity, Set<BucketKey>> mAttributions;
+    private final Map<CallerIdentity, Integer> mAttributions;
     @GuardedBy("this")
-    private final Map<CallerIdentity, Set<BucketKey>> mHighPowerAttributions;
+    private final Map<CallerIdentity, Integer> mHighPowerAttributions;
 
     public LocationAttributionHelper(AppOpsHelper appOpsHelper) {
         mAppOpsHelper = appOpsHelper;
@@ -84,15 +52,16 @@ public class LocationAttributionHelper {
     /**
      * Report normal location usage for the given caller in the given bucket, with a unique key.
      */
-    public synchronized void reportLocationStart(CallerIdentity identity, String bucket,
+    public synchronized void reportLocationStart(CallerIdentity identity) {
-            Object key) {
+        identity = CallerIdentity.forAggregation(identity);
-        Set<BucketKey> keySet = mAttributions.computeIfAbsent(identity,
+
-                i -> new ArraySet<>());
+        int count = mAttributions.getOrDefault(identity, 0);
-        boolean empty = keySet.isEmpty();
+        if (count == 0) {
-        if (keySet.add(new BucketKey(bucket, key)) && empty) {
+            if (mAppOpsHelper.startOpNoThrow(OP_MONITOR_LOCATION, identity)) {
-            if (!mAppOpsHelper.startOpNoThrow(OP_MONITOR_LOCATION, identity)) {
+                mAttributions.put(identity, 1);
-                mAttributions.remove(identity);
             }
+        } else {
+            mAttributions.put(identity, count + 1);
         }
     }
 
@@ -100,13 +69,15 @@ public class LocationAttributionHelper {
      * Report normal location usage has stopped for the given caller in the given bucket, with a
      * unique key.
      */
-    public synchronized void reportLocationStop(CallerIdentity identity, String bucket,
+    public synchronized void reportLocationStop(CallerIdentity identity) {
-            Object key) {
+        identity = CallerIdentity.forAggregation(identity);
-        Set<BucketKey> keySet = mAttributions.get(identity);
+
-        if (keySet != null && keySet.remove(new BucketKey(bucket, key))
+        int count = mAttributions.getOrDefault(identity, 0);
-                && keySet.isEmpty()) {
+        if (count == 1) {
             mAttributions.remove(identity);
             mAppOpsHelper.finishOp(OP_MONITOR_LOCATION, identity);
+        } else if (count > 1) {
+            mAttributions.put(identity, count - 1);
         }
     }
 
@@ -114,19 +85,19 @@ public class LocationAttributionHelper {
      * Report high power location usage for the given caller in the given bucket, with a unique
      * key.
      */
-    public synchronized void reportHighPowerLocationStart(CallerIdentity identity, String bucket,
+    public synchronized void reportHighPowerLocationStart(CallerIdentity identity) {
-            Object key) {
+        identity = CallerIdentity.forAggregation(identity);
-        Set<BucketKey> keySet = mHighPowerAttributions.computeIfAbsent(identity,
+
-                i -> new ArraySet<>());
+        int count = mHighPowerAttributions.getOrDefault(identity, 0);
-        boolean empty = keySet.isEmpty();
+        if (count == 0) {
-        if (keySet.add(new BucketKey(bucket, key)) && empty) {
+            if (D) {
-            if (mAppOpsHelper.startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION, identity)) {
+                Log.v(TAG, "starting high power location attribution for " + identity);
-                if (D) {
-                    Log.v(TAG, "starting high power location attribution for " + identity);
-                }
-            } else {
-                mHighPowerAttributions.remove(identity);
             }
+            if (mAppOpsHelper.startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION, identity)) {
+                mHighPowerAttributions.put(identity, 1);
+            }
+        } else {
+            mHighPowerAttributions.put(identity, count + 1);
         }
     }
 
@@ -134,16 +105,18 @@ public class LocationAttributionHelper {
      * Report high power location usage has stopped for the given caller in the given bucket,
      * with a unique key.
      */
-    public synchronized void reportHighPowerLocationStop(CallerIdentity identity, String bucket,
+    public synchronized void reportHighPowerLocationStop(CallerIdentity identity) {
-            Object key) {
+        identity = CallerIdentity.forAggregation(identity);
-        Set<BucketKey> keySet = mHighPowerAttributions.get(identity);
+
-        if (keySet != null && keySet.remove(new BucketKey(bucket, key))
+        int count = mHighPowerAttributions.getOrDefault(identity, 0);
-                && keySet.isEmpty()) {
+        if (count == 1) {
             if (D) {
                 Log.v(TAG, "stopping high power location attribution for " + identity);
             }
             mHighPowerAttributions.remove(identity);
             mAppOpsHelper.finishOp(OP_MONITOR_HIGH_POWER_LOCATION, identity);
+        } else if (count > 1) {
+            mHighPowerAttributions.put(identity, count - 1);
         }
     }
 }

services/core/java/com/android/server/location/provider/LocationProviderManager.java

@@ -398,7 +398,7 @@ public class LocationProviderManager extends
             EVENT_LOG.logProviderClientActive(mName, getIdentity());
 
             if (!getRequest().isHiddenFromAppOps()) {
-                mLocationAttributionHelper.reportLocationStart(getIdentity(), getName(), getKey());
+                mLocationAttributionHelper.reportLocationStart(getIdentity());
             }
             onHighPowerUsageChanged();
 
@@ -413,7 +413,7 @@ public class LocationProviderManager extends
 
             onHighPowerUsageChanged();
             if (!getRequest().isHiddenFromAppOps()) {
-                mLocationAttributionHelper.reportLocationStop(getIdentity(), getName(), getKey());
+                mLocationAttributionHelper.reportLocationStop(getIdentity());
             }
 
             onProviderListenerInactive();
@@ -488,10 +488,10 @@ public class LocationProviderManager extends
                 if (!getRequest().isHiddenFromAppOps()) {
                     if (mIsUsingHighPower) {
                         mLocationAttributionHelper.reportHighPowerLocationStart(
-                                getIdentity(), getName(), getKey());
+                                getIdentity());
                     } else {
                         mLocationAttributionHelper.reportHighPowerLocationStop(
-                                getIdentity(), getName(), getKey());
+                                getIdentity());
                     }
                 }
             }

services/tests/mockingservicestests/src/com/android/server/location/injector/LocationAttributionHelperTest.java

@@ -58,72 +58,86 @@ public class LocationAttributionHelperTest {
     @Test
     public void testLocationMonitoring() {
         CallerIdentity caller1 = CallerIdentity.forTest(1, 1, "test1", null);
-        Object key1 = new Object();
-        Object key2 = new Object();
         CallerIdentity caller2 = CallerIdentity.forTest(2, 2, "test2", null);
-        Object key3 = new Object();
-        Object key4 = new Object();
 
-        mHelper.reportLocationStart(caller1, "gps", key1);
+        mHelper.reportLocationStart(caller1);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION, caller1);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION, caller1);
+                CallerIdentity.forAggregation(caller1));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION,
+                CallerIdentity.forAggregation(caller1));
 
-        mHelper.reportLocationStart(caller1, "gps", key2);
+        mHelper.reportLocationStart(caller1);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION, caller1);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION, caller1);
+                CallerIdentity.forAggregation(caller1));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION,
+                CallerIdentity.forAggregation(caller1));
 
-        mHelper.reportLocationStart(caller2, "gps", key3);
+        mHelper.reportLocationStart(caller2);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION, caller2);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION, caller2);
+                CallerIdentity.forAggregation(caller2));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION,
+                CallerIdentity.forAggregation(caller2));
 
-        mHelper.reportLocationStart(caller2, "gps", key4);
+        mHelper.reportLocationStart(caller2);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION, caller2);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION, caller2);
+                CallerIdentity.forAggregation(caller2));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION,
+                CallerIdentity.forAggregation(caller2));
 
-        mHelper.reportLocationStop(caller1, "gps", key2);
+        mHelper.reportLocationStop(caller1);
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION, caller1);
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION,
-        mHelper.reportLocationStop(caller1, "gps", key1);
+                CallerIdentity.forAggregation(caller1));
-        verify(mAppOpsHelper).finishOp(OP_MONITOR_LOCATION, caller1);
+        mHelper.reportLocationStop(caller1);
+        verify(mAppOpsHelper).finishOp(OP_MONITOR_LOCATION, CallerIdentity.forAggregation(caller1));
 
-        mHelper.reportLocationStop(caller2, "gps", key3);
+        mHelper.reportLocationStop(caller2);
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION, caller2);
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_LOCATION,
-        mHelper.reportLocationStop(caller2, "gps", key4);
+                CallerIdentity.forAggregation(caller2));
-        verify(mAppOpsHelper).finishOp(OP_MONITOR_LOCATION, caller2);
+        mHelper.reportLocationStop(caller2);
+        verify(mAppOpsHelper).finishOp(OP_MONITOR_LOCATION, CallerIdentity.forAggregation(caller2));
     }
 
     @Test
     public void testHighPowerLocationMonitoring() {
         CallerIdentity caller1 = CallerIdentity.forTest(1, 1, "test1", null);
-        Object key1 = new Object();
-        Object key2 = new Object();
         CallerIdentity caller2 = CallerIdentity.forTest(2, 2, "test2", null);
-        Object key3 = new Object();
-        Object key4 = new Object();
 
-        mHelper.reportHighPowerLocationStart(caller1, "gps", key1);
+        mHelper.reportHighPowerLocationStart(caller1);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION, caller1);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller1);
+                CallerIdentity.forAggregation(caller1));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
+                CallerIdentity.forAggregation(caller1));
 
-        mHelper.reportHighPowerLocationStart(caller1, "gps", key2);
+        mHelper.reportHighPowerLocationStart(caller1);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION, caller1);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller1);
+                CallerIdentity.forAggregation(caller1));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
+                CallerIdentity.forAggregation(caller1));
 
-        mHelper.reportHighPowerLocationStart(caller2, "gps", key3);
+        mHelper.reportHighPowerLocationStart(caller2);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION, caller2);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller2);
+                CallerIdentity.forAggregation(caller2));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
+                CallerIdentity.forAggregation(caller2));
 
-        mHelper.reportHighPowerLocationStart(caller2, "gps", key4);
+        mHelper.reportHighPowerLocationStart(caller2);
-        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION, caller2);
+        verify(mAppOpsHelper).startOpNoThrow(OP_MONITOR_HIGH_POWER_LOCATION,
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller2);
+                CallerIdentity.forAggregation(caller2));
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
+                CallerIdentity.forAggregation(caller2));
 
-        mHelper.reportHighPowerLocationStop(caller1, "gps", key2);
+        mHelper.reportHighPowerLocationStop(caller1);
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller1);
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
-        mHelper.reportHighPowerLocationStop(caller1, "gps", key1);
+                CallerIdentity.forAggregation(caller1));
-        verify(mAppOpsHelper).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller1);
+        mHelper.reportHighPowerLocationStop(caller1);
+        verify(mAppOpsHelper).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
+                CallerIdentity.forAggregation(caller1));
 
-        mHelper.reportHighPowerLocationStop(caller2, "gps", key3);
+        mHelper.reportHighPowerLocationStop(caller2);
-        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller2);
+        verify(mAppOpsHelper, never()).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
-        mHelper.reportHighPowerLocationStop(caller2, "gps", key4);
+                CallerIdentity.forAggregation(caller2));
-        verify(mAppOpsHelper).finishOp(OP_MONITOR_HIGH_POWER_LOCATION, caller2);
+        mHelper.reportHighPowerLocationStop(caller2);
+        verify(mAppOpsHelper).finishOp(OP_MONITOR_HIGH_POWER_LOCATION,
+                CallerIdentity.forAggregation(caller2));
     }
 }

services/tests/mockingservicestests/src/com/android/server/location/provider/LocationProviderManagerTest.java

@@ -844,6 +844,48 @@ public class LocationProviderManagerTest {
                 IDENTITY.getPackageName())).isFalse();
     }
 
+    @Test
+    public void testLocationMonitoring_multipleIdentities() {
+        CallerIdentity identity1 = CallerIdentity.forTest(CURRENT_USER, 1,
+                "mypackage", "attribution", "listener1");
+        CallerIdentity identity2 = CallerIdentity.forTest(CURRENT_USER, 1,
+                "mypackage", "attribution", "listener2");
+
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_LOCATION,
+                IDENTITY.getPackageName())).isFalse();
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_HIGH_POWER_LOCATION,
+                IDENTITY.getPackageName())).isFalse();
+
+        ILocationListener listener1 = createMockLocationListener();
+        LocationRequest request1 = new LocationRequest.Builder(0).setWorkSource(
+                WORK_SOURCE).build();
+        mManager.registerLocationRequest(request1, identity1, PERMISSION_FINE, listener1);
+
+        ILocationListener listener2 = createMockLocationListener();
+        LocationRequest request2 = new LocationRequest.Builder(0).setWorkSource(
+                WORK_SOURCE).build();
+        mManager.registerLocationRequest(request2, identity2, PERMISSION_FINE, listener2);
+
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_LOCATION,
+                "mypackage")).isTrue();
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_HIGH_POWER_LOCATION,
+                "mypackage")).isTrue();
+
+        mManager.unregisterLocationRequest(listener2);
+
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_LOCATION,
+                "mypackage")).isTrue();
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_HIGH_POWER_LOCATION,
+                "mypackage")).isTrue();
+
+        mManager.unregisterLocationRequest(listener1);
+
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_LOCATION,
+                "mypackage")).isFalse();
+        assertThat(mInjector.getAppOpsHelper().isAppOpStarted(OP_MONITOR_HIGH_POWER_LOCATION,
+                "mypackage")).isFalse();
+    }
+
     @Test
     public void testProviderRequest() {
         assertThat(mProvider.getRequest().isActive()).isFalse();