diff --git a/location/java/android/location/util/identity/CallerIdentity.java b/location/java/android/location/util/identity/CallerIdentity.java index ade0ea40e1576..2f8d92b06f030 100644 --- a/location/java/android/location/util/identity/CallerIdentity.java +++ b/location/java/android/location/util/identity/CallerIdentity.java @@ -124,15 +124,22 @@ public final class CallerIdentity { packageName, attributionTag, listenerId); } + // in some tests these constants are loaded too early leading to an "incorrect" view of the + // current pid and uid. load lazily to prevent this problem in tests. + private static class Loader { + private static final int MY_UID = Process.myUid(); + private static final int MY_PID = Process.myPid(); + } + private final int mUid; private final int mPid; private final String mPackageName; - private final @Nullable String mAttributionTag; + @Nullable private final String mAttributionTag; - private final @Nullable String mListenerId; + @Nullable private final String mListenerId; private CallerIdentity(int uid, int pid, String packageName, @Nullable String attributionTag, @Nullable String listenerId) { @@ -181,6 +188,24 @@ public final class CallerIdentity { return mUid == Process.SYSTEM_UID; } + /** Returns true if this identity represents the same user this code is running in. */ + public boolean isMyUser() { + return UserHandle.getUserId(mUid) == UserHandle.getUserId(Loader.MY_UID); + } + + /** Returns true if this identity represents the same uid this code is running in. */ + public boolean isMyUid() { + return mUid == Loader.MY_UID; + } + + /** + * Returns true if this identity represents the same process this code is running in. Returns + * false if the identity process is unknown. + */ + public boolean isMyProcess() { + return mPid == Loader.MY_PID; + } + /** * Adds this identity to the worksource supplied, or if not worksource is supplied, creates a * new worksource representing this identity. diff --git a/services/core/java/com/android/server/location/LocationPermissions.java b/services/core/java/com/android/server/location/LocationPermissions.java index ca2ff60203ca3..f7da0d8639b8d 100644 --- a/services/core/java/com/android/server/location/LocationPermissions.java +++ b/services/core/java/com/android/server/location/LocationPermissions.java @@ -26,8 +26,10 @@ import android.app.AppOpsManager; import android.content.Context; import android.os.Binder; +import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; /** Utility class for dealing with location permissions. */ public final class LocationPermissions { @@ -49,6 +51,7 @@ public final class LocationPermissions { */ public static final int PERMISSION_FINE = 2; + @Target(ElementType.TYPE_USE) @IntDef({PERMISSION_NONE, PERMISSION_COARSE, PERMISSION_FINE}) @Retention(RetentionPolicy.SOURCE) public @interface PermissionLevel {} diff --git a/services/core/java/com/android/server/location/geofence/GeofenceKey.java b/services/core/java/com/android/server/location/geofence/GeofenceKey.java deleted file mode 100644 index bbfa68f1e292e..0000000000000 --- a/services/core/java/com/android/server/location/geofence/GeofenceKey.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.server.location.geofence; - -import android.app.PendingIntent; -import android.location.Geofence; - -import com.android.server.location.listeners.PendingIntentListenerRegistration; - -import java.util.Objects; - -// geofencing unfortunately allows multiple geofences under the same pending intent, even though -// this makes no real sense. therefore we manufacture an artificial key to use (pendingintent + -// geofence) instead of (pendingintent). -final class GeofenceKey implements PendingIntentListenerRegistration.PendingIntentKey { - - private final PendingIntent mPendingIntent; - private final Geofence mGeofence; - - GeofenceKey(PendingIntent pendingIntent, Geofence geofence) { - mPendingIntent = Objects.requireNonNull(pendingIntent); - mGeofence = Objects.requireNonNull(geofence); - } - - @Override - public PendingIntent getPendingIntent() { - return mPendingIntent; - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (!(o instanceof GeofenceKey)) { - return false; - } - GeofenceKey that = (GeofenceKey) o; - return mPendingIntent.equals(that.mPendingIntent) && mGeofence.equals(that.mGeofence); - } - - @Override - public int hashCode() { - return mPendingIntent.hashCode(); - } -} diff --git a/services/core/java/com/android/server/location/geofence/GeofenceManager.java b/services/core/java/com/android/server/location/geofence/GeofenceManager.java index b6342a472023f..0f5e3d46fd222 100644 --- a/services/core/java/com/android/server/location/geofence/GeofenceManager.java +++ b/services/core/java/com/android/server/location/geofence/GeofenceManager.java @@ -59,8 +59,8 @@ import java.util.Objects; * Manages all geofences. */ public class GeofenceManager extends - ListenerMultiplexer implements + ListenerMultiplexer implements LocationListener { private static final String TAG = "GeofenceManager"; @@ -73,13 +73,49 @@ public class GeofenceManager extends private static final long MAX_LOCATION_AGE_MS = 5 * 60 * 1000L; // five minutes private static final long MAX_LOCATION_INTERVAL_MS = 2 * 60 * 60 * 1000; // two hours - protected final class GeofenceRegistration extends - PendingIntentListenerRegistration { + // geofencing unfortunately allows multiple geofences under the same pending intent, even though + // this makes no real sense. therefore we manufacture an artificial key to use (pendingintent + + // geofence) instead of (pendingintent). + static class GeofenceKey { + + private final PendingIntent mPendingIntent; + private final Geofence mGeofence; + + GeofenceKey(PendingIntent pendingIntent, Geofence geofence) { + mPendingIntent = Objects.requireNonNull(pendingIntent); + mGeofence = Objects.requireNonNull(geofence); + } + + public PendingIntent getPendingIntent() { + return mPendingIntent; + } + + @Override + public boolean equals(Object o) { + if (o instanceof GeofenceKey) { + GeofenceKey that = (GeofenceKey) o; + return mPendingIntent.equals(that.mPendingIntent) && mGeofence.equals( + that.mGeofence); + } + + return false; + } + + @Override + public int hashCode() { + return mPendingIntent.hashCode(); + } + } + + protected class GeofenceRegistration extends + PendingIntentListenerRegistration { private static final int STATE_UNKNOWN = 0; private static final int STATE_INSIDE = 1; private static final int STATE_OUTSIDE = 2; + private final Geofence mGeofence; + private final CallerIdentity mIdentity; private final Location mCenter; private final PowerManager.WakeLock mWakeLock; @@ -89,13 +125,15 @@ public class GeofenceManager extends // spam us, and because checking the values may be more expensive private boolean mPermitted; - private @Nullable Location mCachedLocation; + @Nullable private Location mCachedLocation; private float mCachedLocationDistanceM; - protected GeofenceRegistration(Geofence geofence, CallerIdentity identity, + GeofenceRegistration(Geofence geofence, CallerIdentity identity, PendingIntent pendingIntent) { - super(geofence, identity, pendingIntent); + super(pendingIntent); + mGeofence = geofence; + mIdentity = identity; mCenter = new Location(""); mCenter.setLatitude(geofence.getLatitude()); mCenter.setLongitude(geofence.getLongitude()); @@ -107,16 +145,36 @@ public class GeofenceManager extends mWakeLock.setWorkSource(identity.addToWorkSource(null)); } + public Geofence getGeofence() { + return mGeofence; + } + + public CallerIdentity getIdentity() { + return mIdentity; + } + + @Override + public String getTag() { + return TAG; + } + + @Override + protected PendingIntent getPendingIntentFromKey(GeofenceKey geofenceKey) { + return geofenceKey.getPendingIntent(); + } + @Override protected GeofenceManager getOwner() { return GeofenceManager.this; } @Override - protected void onPendingIntentListenerRegister() { + protected void onRegister() { + super.onRegister(); + mGeofenceState = STATE_UNKNOWN; mPermitted = mLocationPermissionsHelper.hasLocationPermissions(PERMISSION_FINE, - getIdentity()); + mIdentity); } @Override @@ -132,7 +190,7 @@ public class GeofenceManager extends } boolean onLocationPermissionsChanged(@Nullable String packageName) { - if (packageName == null || getIdentity().getPackageName().equals(packageName)) { + if (packageName == null || mIdentity.getPackageName().equals(packageName)) { return onLocationPermissionsChanged(); } @@ -140,7 +198,7 @@ public class GeofenceManager extends } boolean onLocationPermissionsChanged(int uid) { - if (getIdentity().getUid() == uid) { + if (mIdentity.getUid() == uid) { return onLocationPermissionsChanged(); } @@ -149,7 +207,7 @@ public class GeofenceManager extends private boolean onLocationPermissionsChanged() { boolean permitted = mLocationPermissionsHelper.hasLocationPermissions(PERMISSION_FINE, - getIdentity()); + mIdentity); if (permitted != mPermitted) { mPermitted = permitted; return true; @@ -164,12 +222,12 @@ public class GeofenceManager extends mCachedLocationDistanceM = mCenter.distanceTo(mCachedLocation); } - return Math.abs(getRequest().getRadius() - mCachedLocationDistanceM); + return Math.abs(mGeofence.getRadius() - mCachedLocationDistanceM); } ListenerOperation onLocationChanged(Location location) { // remove expired fences - if (getRequest().isExpired()) { + if (mGeofence.isExpired()) { remove(); return null; } @@ -178,7 +236,7 @@ public class GeofenceManager extends mCachedLocationDistanceM = mCenter.distanceTo(mCachedLocation); int oldState = mGeofenceState; - float radius = Math.max(getRequest().getRadius(), location.getAccuracy()); + float radius = Math.max(mGeofence.getRadius(), location.getAccuracy()); if (mCachedLocationDistanceM <= radius) { mGeofenceState = STATE_INSIDE; if (oldState != STATE_INSIDE) { @@ -206,14 +264,14 @@ public class GeofenceManager extends null, null, PendingIntentUtils.createDontSendToRestrictedAppsBundle(null)); } catch (PendingIntent.CanceledException e) { mWakeLock.release(); - removeRegistration(new GeofenceKey(pendingIntent, getRequest()), this); + removeRegistration(new GeofenceKey(pendingIntent, mGeofence), this); } } @Override public String toString() { StringBuilder builder = new StringBuilder(); - builder.append(getIdentity()); + builder.append(mIdentity); ArraySet flags = new ArraySet<>(1); if (!mPermitted) { @@ -223,7 +281,7 @@ public class GeofenceManager extends builder.append(" ").append(flags); } - builder.append(" ").append(getRequest()); + builder.append(" ").append(mGeofence); return builder.toString(); } } @@ -258,10 +316,10 @@ public class GeofenceManager extends protected final LocationUsageLogger mLocationUsageLogger; @GuardedBy("mLock") - private @Nullable LocationManager mLocationManager; + @Nullable private LocationManager mLocationManager; @GuardedBy("mLock") - private @Nullable Location mLastLocation; + @Nullable private Location mLastLocation; public GeofenceManager(Context context, Injector injector) { mContext = context.createAttributionContext(ATTRIBUTION_TAG); @@ -271,11 +329,6 @@ public class GeofenceManager extends mLocationUsageLogger = injector.getLocationUsageLogger(); } - @Override - public String getTag() { - return TAG; - } - private LocationManager getLocationManager() { synchronized (mLock) { if (mLocationManager == null) { @@ -375,7 +428,7 @@ public class GeofenceManager extends /* LocationRequest= */ null, /* hasListener= */ false, true, - registration.getRequest(), true); + registration.getGeofence(), true); } @Override @@ -389,7 +442,7 @@ public class GeofenceManager extends /* LocationRequest= */ null, /* hasListener= */ false, true, - registration.getRequest(), true); + registration.getGeofence(), true); } @Override @@ -417,7 +470,7 @@ public class GeofenceManager extends WorkSource workSource = null; double minFenceDistanceM = Double.MAX_VALUE; for (GeofenceRegistration registration : registrations) { - if (registration.getRequest().isExpired(realtimeMs)) { + if (registration.getGeofence().isExpired(realtimeMs)) { continue; } diff --git a/services/core/java/com/android/server/location/gnss/GnssAntennaInfoProvider.java b/services/core/java/com/android/server/location/gnss/GnssAntennaInfoProvider.java index e3750074168c2..62ab22a46ba8b 100644 --- a/services/core/java/com/android/server/location/gnss/GnssAntennaInfoProvider.java +++ b/services/core/java/com/android/server/location/gnss/GnssAntennaInfoProvider.java @@ -16,6 +16,7 @@ package com.android.server.location.gnss; +import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR; import static com.android.server.location.gnss.GnssManagerService.TAG; import android.annotation.Nullable; @@ -25,6 +26,7 @@ import android.location.util.identity.CallerIdentity; import android.os.Binder; import android.os.IBinder; +import com.android.server.FgThread; import com.android.server.location.gnss.hal.GnssNative; import com.android.server.location.listeners.BinderListenerRegistration; import com.android.server.location.listeners.ListenerMultiplexer; @@ -45,17 +47,35 @@ public class GnssAntennaInfoProvider extends * Registration object for GNSS listeners. */ protected class AntennaInfoListenerRegistration extends - BinderListenerRegistration { + BinderListenerRegistration { - protected AntennaInfoListenerRegistration(CallerIdentity callerIdentity, + private final CallerIdentity mIdentity; + + protected AntennaInfoListenerRegistration(CallerIdentity identity, IGnssAntennaInfoListener listener) { - super(null, callerIdentity, listener); + super(identity.isMyProcess() ? FgThread.getExecutor() : DIRECT_EXECUTOR, listener); + mIdentity = identity; + } + + @Override + protected String getTag() { + return TAG; } @Override protected GnssAntennaInfoProvider getOwner() { return GnssAntennaInfoProvider.this; } + + @Override + protected IBinder getBinderFromKey(IBinder key) { + return key; + } + + @Override + public String toString() { + return mIdentity.toString(); + } } private final GnssNative mGnssNative; @@ -72,11 +92,6 @@ public class GnssAntennaInfoProvider extends return mAntennaInfos; } - @Override - public String getTag() { - return TAG; - } - public boolean isSupported() { return mGnssNative.isAntennaInfoSupported(); } diff --git a/services/core/java/com/android/server/location/gnss/GnssListenerMultiplexer.java b/services/core/java/com/android/server/location/gnss/GnssListenerMultiplexer.java index a54047665aba3..82bcca2b84706 100644 --- a/services/core/java/com/android/server/location/gnss/GnssListenerMultiplexer.java +++ b/services/core/java/com/android/server/location/gnss/GnssListenerMultiplexer.java @@ -18,6 +18,7 @@ package com.android.server.location.gnss; import static android.location.LocationManager.GPS_PROVIDER; +import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR; import static com.android.server.location.LocationPermissions.PERMISSION_FINE; import static com.android.server.location.gnss.GnssManagerService.TAG; @@ -33,6 +34,7 @@ import android.os.Process; import android.util.ArraySet; import com.android.internal.util.Preconditions; +import com.android.server.FgThread; import com.android.server.LocalServices; import com.android.server.location.injector.AppForegroundHelper; import com.android.server.location.injector.Injector; @@ -67,16 +69,34 @@ public abstract class GnssListenerMultiplexer { + BinderListenerRegistration { + + private final TRequest mRequest; + private final CallerIdentity mIdentity; // we store these values because we don't trust the listeners not to give us dupes, not to // spam us, and because checking the values may be more expensive private boolean mForeground; private boolean mPermitted; - protected GnssListenerRegistration(@Nullable TRequest request, - CallerIdentity callerIdentity, TListener listener) { - super(request, callerIdentity, listener); + protected GnssListenerRegistration(TRequest request, CallerIdentity identity, + TListener listener) { + super(identity.isMyProcess() ? FgThread.getExecutor() : DIRECT_EXECUTOR, listener); + mRequest = request; + mIdentity = identity; + } + + public final TRequest getRequest() { + return mRequest; + } + + public final CallerIdentity getIdentity() { + return mIdentity; + } + + @Override + public String getTag() { + return TAG; } @Override @@ -84,6 +104,11 @@ public abstract class GnssListenerMultiplexer flags = new ArraySet<>(2); if (!mForeground) { @@ -171,8 +181,8 @@ public abstract class GnssListenerMultiplexer listener.onStatusChanged( GnssMeasurementsEvent.Callback.STATUS_READY)); } diff --git a/services/core/java/com/android/server/location/gnss/GnssNavigationMessageProvider.java b/services/core/java/com/android/server/location/gnss/GnssNavigationMessageProvider.java index e9fce0514a18b..63134bb77ccb2 100644 --- a/services/core/java/com/android/server/location/gnss/GnssNavigationMessageProvider.java +++ b/services/core/java/com/android/server/location/gnss/GnssNavigationMessageProvider.java @@ -32,11 +32,7 @@ import com.android.server.location.injector.Injector; import java.util.Collection; /** - * An base implementation for GPS navigation messages provider. - * It abstracts out the responsibility of handling listeners, while still allowing technology - * specific implementations to be built. - * - * @hide + * GNSS navigation message HAL module and listener multiplexer. */ public class GnssNavigationMessageProvider extends GnssListenerMultiplexer implements @@ -51,7 +47,9 @@ public class GnssNavigationMessageProvider extends } @Override - protected void onGnssListenerRegister() { + protected void onRegister() { + super.onRegister(); + executeOperation(listener -> listener.onStatusChanged( GnssNavigationMessage.Callback.STATUS_READY)); } diff --git a/services/core/java/com/android/server/location/gnss/GnssNmeaProvider.java b/services/core/java/com/android/server/location/gnss/GnssNmeaProvider.java index bfef97856838e..d4e38b6a05dbe 100644 --- a/services/core/java/com/android/server/location/gnss/GnssNmeaProvider.java +++ b/services/core/java/com/android/server/location/gnss/GnssNmeaProvider.java @@ -34,7 +34,7 @@ import java.util.Collection; import java.util.function.Function; /** - * Implementation of a handler for {@link IGnssNmeaListener}. + * GNSS NMEA HAL module and listener multiplexer. */ class GnssNmeaProvider extends GnssListenerMultiplexer implements GnssNative.BaseCallbacks, GnssNative.NmeaCallbacks { @@ -97,7 +97,7 @@ class GnssNmeaProvider extends GnssListenerMultiplexer>() { // only read in the nmea string if we need to - private @Nullable String mNmea; + @Nullable private String mNmea; @Override public ListenerExecutor.ListenerOperation apply( diff --git a/services/core/java/com/android/server/location/gnss/GnssStatusProvider.java b/services/core/java/com/android/server/location/gnss/GnssStatusProvider.java index 0ce36d6a82763..41fa7a1832885 100644 --- a/services/core/java/com/android/server/location/gnss/GnssStatusProvider.java +++ b/services/core/java/com/android/server/location/gnss/GnssStatusProvider.java @@ -35,7 +35,7 @@ import com.android.server.location.injector.LocationUsageLogger; import java.util.Collection; /** - * Implementation of a handler for {@link IGnssStatusListener}. + * GNSS status HAL module and listener multiplexer. */ public class GnssStatusProvider extends GnssListenerMultiplexer implements diff --git a/services/core/java/com/android/server/location/listeners/BinderListenerRegistration.java b/services/core/java/com/android/server/location/listeners/BinderListenerRegistration.java index 709e23615b145..5555aeb3e5e8c 100644 --- a/services/core/java/com/android/server/location/listeners/BinderListenerRegistration.java +++ b/services/core/java/com/android/server/location/listeners/BinderListenerRegistration.java @@ -16,71 +16,59 @@ package com.android.server.location.listeners; -import android.annotation.Nullable; -import android.location.util.identity.CallerIdentity; -import android.os.Binder; import android.os.IBinder; +import android.os.IBinder.DeathRecipient; import android.os.RemoteException; import android.util.Log; +import java.util.NoSuchElementException; +import java.util.concurrent.Executor; + /** * A registration that works with IBinder keys, and registers a DeathListener to automatically - * remove the registration if the binder dies. The key for this registration must either be an - * {@link IBinder} or a {@link BinderKey}. + * remove the registration if the binder dies. * - * @param request type + * @param key type * @param listener type */ -public abstract class BinderListenerRegistration extends - RemoteListenerRegistration implements Binder.DeathRecipient { +public abstract class BinderListenerRegistration extends + RemovableListenerRegistration implements DeathRecipient { - /** - * Interface to allow binder retrieval when keys are not themselves IBinders. - */ - public interface BinderKey { - /** - * Returns the binder associated with this key. - */ - IBinder getBinder(); + protected BinderListenerRegistration(Executor executor, TListener listener) { + super(executor, listener); } - protected BinderListenerRegistration(@Nullable TRequest request, CallerIdentity callerIdentity, - TListener listener) { - super(request, callerIdentity, listener); - } + protected abstract IBinder getBinderFromKey(TKey key); @Override - protected final void onRemovableListenerRegister() { - IBinder binder = getBinderFromKey(getKey()); + protected void onRegister() { + super.onRegister(); + try { - binder.linkToDeath(this, 0); + getBinderFromKey(getKey()).linkToDeath(this, 0); } catch (RemoteException e) { remove(); } - - onBinderListenerRegister(); } @Override - protected final void onRemovableListenerUnregister() { - onBinderListenerUnregister(); - getBinderFromKey(getKey()).unlinkToDeath(this, 0); + protected void onUnregister() { + try { + getBinderFromKey(getKey()).unlinkToDeath(this, 0); + } catch (NoSuchElementException e) { + // the only way this exception can occur should be if another exception has been thrown + // prior to registration completing, and that exception is currently unwinding the call + // stack and causing this cleanup. since that exception should crash us anyways, drop + // this exception so we're not hiding the original exception. + Log.w(getTag(), "failed to unregister binder death listener", e); + } + + super.onUnregister(); } - /** - * May be overridden in place of {@link #onRemovableListenerRegister()}. - */ - protected void onBinderListenerRegister() {} - - /** - * May be overridden in place of {@link #onRemovableListenerUnregister()}. - */ - protected void onBinderListenerUnregister() {} - - @Override public void onOperationFailure(ListenerOperation operation, Exception e) { if (e instanceof RemoteException) { - Log.w(getOwner().getTag(), "registration " + this + " removed", e); + Log.w(getTag(), "registration " + this + " removed", e); remove(); } else { super.onOperationFailure(operation, e); @@ -90,9 +78,10 @@ public abstract class BinderListenerRegistration extends @Override public void binderDied() { try { - if (Log.isLoggable(getOwner().getTag(), Log.DEBUG)) { - Log.d(getOwner().getTag(), "binder registration " + getIdentity() + " died"); + if (Log.isLoggable(getTag(), Log.DEBUG)) { + Log.d(getTag(), "binder registration " + this + " died"); } + remove(); } catch (RuntimeException e) { // the caller may swallow runtime exceptions, so we rethrow as assertion errors to @@ -100,14 +89,4 @@ public abstract class BinderListenerRegistration extends throw new AssertionError(e); } } - - private static IBinder getBinderFromKey(Object key) { - if (key instanceof IBinder) { - return (IBinder) key; - } else if (key instanceof BinderKey) { - return ((BinderKey) key).getBinder(); - } else { - throw new IllegalArgumentException("key must be IBinder or BinderKey"); - } - } } diff --git a/services/core/java/com/android/server/location/listeners/ListenerMultiplexer.java b/services/core/java/com/android/server/location/listeners/ListenerMultiplexer.java index 33b08d41c07de..67ae26591d5b5 100644 --- a/services/core/java/com/android/server/location/listeners/ListenerMultiplexer.java +++ b/services/core/java/com/android/server/location/listeners/ListenerMultiplexer.java @@ -18,7 +18,6 @@ package com.android.server.location.listeners; import android.annotation.NonNull; import android.annotation.Nullable; -import android.os.Build; import android.util.ArrayMap; import android.util.ArraySet; @@ -37,40 +36,48 @@ import java.util.function.Function; import java.util.function.Predicate; /** - * A base class to multiplex client listener registrations within system server. Every listener is + * A base class to multiplex some event source to multiple listener registrations. Every listener is * represented by a registration object which stores all required state for a listener. Keys are * used to uniquely identify every registration. Listener operations may be executed on * registrations in order to invoke the represented listener. * - * Registrations are divided into two categories, active registrations and inactive registrations, - * as defined by {@link #isActive(ListenerRegistration)}. If a registration's active state changes, - * {@link #updateRegistrations(Predicate)} must be invoked and return true for any registration - * whose active state may have changed. Listeners will only be invoked for active registrations. + *

Registrations are divided into two categories, active registrations and inactive + * registrations, as defined by {@link #isActive(ListenerRegistration)}. The set of active + * registrations is combined into a single merged registration, which is submitted to the backing + * event source when necessary in order to register with the event source. The merged registration + * is updated whenever the set of active registration changes. Listeners will only be invoked for + * active registrations. * - * The set of active registrations is combined into a single merged registration, which is submitted - * to the backing service when necessary in order to register the service. The merged registration - * is updated whenever the set of active registration changes. + *

In order to inform the multiplexer of state changes, if a registration's active state changes, + * or if the merged registration changes, {@link #updateRegistrations(Predicate)} or {@link + * #updateRegistration(Object, Predicate)} must be invoked and return true for any registration + * whose state may have changed in such a way that the active state or merged registration state has + * changed. It is acceptable to return true from a predicate even if nothing has changed, though + * this may result in extra pointless work. * - * Callbacks invoked for various changes will always be ordered according to this lifecycle list: + *

Callbacks invoked for various changes will always be ordered according to this lifecycle list: * *

    - *
  • {@link #onRegister()}
    • - *
  • {@link ListenerRegistration#onRegister(Object)}
    • - *
  • {@link #onRegistrationAdded(Object, ListenerRegistration)}
    • - *
  • {@link #onRegistrationReplaced(Object, ListenerRegistration, ListenerRegistration)} (only - * invoked if this registration is replacing a prior registration)
    • - *
  • {@link #onActive()}
    • - *
  • {@link ListenerRegistration#onActive()}
    • - *
  • {@link ListenerRegistration#onInactive()}
    • - *
  • {@link #onInactive()}
    • - *
  • {@link #onRegistrationRemoved(Object, ListenerRegistration)}
    • - *
  • {@link ListenerRegistration#onUnregister()}
    • - *
  • {@link #onUnregister()}
    • + *
  • {@link #onRegister()} + *
  • {@link ListenerRegistration#onRegister(Object)} + *
  • {@link #onRegistrationAdded(Object, ListenerRegistration)} + *
  • {@link #onActive()} + *
  • {@link ListenerRegistration#onActive()} + *
  • {@link ListenerRegistration#onInactive()} + *
  • {@link #onInactive()} + *
  • {@link #onRegistrationRemoved(Object, ListenerRegistration)} + *
  • {@link ListenerRegistration#onUnregister()} + *
  • {@link #onUnregister()} *
* - * Adding registrations is not allowed to be called re-entrantly (ie, while in the middle of some - * other operation or callback. Removal is allowed re-entrantly, however only via - * {@link #removeRegistration(Object, ListenerRegistration)}, not via any other removal method. This + *

If one registration replaces another, then {@link #onRegistrationReplaced(Object, + * ListenerRegistration, Object, ListenerRegistration)} is invoked instead of {@link + * #onRegistrationRemoved(Object, ListenerRegistration)} and {@link #onRegistrationAdded(Object, + * ListenerRegistration)}. + * + *

Adding registrations is not allowed to be called re-entrantly (ie, while in the middle of some + * other operation or callback). Removal is allowed re-entrantly, however only via {@link + * #removeRegistration(Object, ListenerRegistration)}, not via any other removal method. This * ensures re-entrant removal does not accidentally remove the incorrect registration. * * @param key type @@ -81,29 +88,30 @@ import java.util.function.Predicate; public abstract class ListenerMultiplexer, TMergedRegistration> { - @GuardedBy("mRegistrations") + /** + * The lock object used by the multiplexer. Acquiring this lock allows for multiple operations + * on the multiplexer to be completed atomically. Otherwise, it is not required to hold this + * lock. This lock is held while invoking all lifecycle callbacks on both the multiplexer and + * any registrations. + */ + protected final Object mMultiplexerLock = new Object(); + + @GuardedBy("mMultiplexerLock") private final ArrayMap mRegistrations = new ArrayMap<>(); - @GuardedBy("mRegistrations") private final UpdateServiceBuffer mUpdateServiceBuffer = new UpdateServiceBuffer(); - @GuardedBy("mRegistrations") private final ReentrancyGuard mReentrancyGuard = new ReentrancyGuard(); - @GuardedBy("mRegistrations") + @GuardedBy("mMultiplexerLock") private int mActiveRegistrationsCount = 0; - @GuardedBy("mRegistrations") + @GuardedBy("mMultiplexerLock") private boolean mServiceRegistered = false; - @GuardedBy("mRegistrations") + @GuardedBy("mMultiplexerLock") @Nullable private TMergedRegistration mMerged; - /** - * Should be implemented to return a unique identifying tag that may be used for logging, etc... - */ - public abstract @NonNull String getTag(); - /** * Should be implemented to register with the backing service with the given merged * registration, and should return true if a matching call to {@link #unregisterWithService()} @@ -120,6 +128,7 @@ public abstract class ListenerMultiplexer registrations); @@ -130,6 +139,7 @@ public abstract class ListenerMultiplexer registrations) { return registerWithService(newMerged, registrations); @@ -138,6 +148,7 @@ public abstract class ListenerMultiplexer registrations); /** @@ -166,6 +179,7 @@ public abstract class ListenerMultiplexerThe default behavior is simply to call first {@link #onRegistrationRemoved(Object, + * ListenerRegistration)} and then {@link #onRegistrationAdded(Object, ListenerRegistration)}. */ - protected void onRegistrationReplaced(@NonNull TKey key, @NonNull TRegistration oldRegistration, + @GuardedBy("mMultiplexerLock") + protected void onRegistrationReplaced( + @NonNull TKey oldKey, + @NonNull TRegistration oldRegistration, + @NonNull TKey newKey, @NonNull TRegistration newRegistration) { - onRegistrationAdded(key, newRegistration); + onRegistrationRemoved(oldKey, oldRegistration); + onRegistrationAdded(newKey, newRegistration); } /** * Invoked when a registration is removed. Invoked while holding the multiplexer's internal * lock. */ + @GuardedBy("mMultiplexerLock") protected void onRegistrationRemoved(@NonNull TKey key, @NonNull TRegistration registration) {} /** @@ -204,6 +228,7 @@ public abstract class ListenerMultiplexer= 0) { - oldRegistration = removeRegistration(index, oldKey != key); + int oldIndex = mRegistrations.indexOfKey(oldKey); + if (oldIndex >= 0) { + // remove ourselves instead of using remove(), to balance registration callbacks + oldRegistration = mRegistrations.valueAt(oldIndex); + unregister(oldRegistration); + oldRegistration.onUnregister(); + if (oldKey != key) { + mRegistrations.removeAt(oldIndex); + } } - if (oldKey == key && index >= 0) { - mRegistrations.setValueAt(index, registration); + if (oldKey == key && oldIndex >= 0) { + mRegistrations.setValueAt(oldIndex, registration); } else { mRegistrations.put(key, registration); } @@ -274,37 +305,19 @@ public abstract class ListenerMultiplexer predicate) { - synchronized (mRegistrations) { + synchronized (mMultiplexerLock) { // this method does not support removing listeners reentrantly Preconditions.checkState(!mReentrancyGuard.isReentrant()); @@ -328,14 +341,32 @@ public abstract class ListenerMultiplexer registration) { - synchronized (mRegistrations) { + synchronized (mMultiplexerLock) { int index = mRegistrations.indexOfKey(key); if (index < 0) { return; @@ -350,17 +381,13 @@ public abstract class ListenerMultiplexer actives = new ArrayList<>(mRegistrations.size()); final int size = mRegistrations.size(); + ArrayList actives = new ArrayList<>(size); for (int i = 0; i < size; i++) { TRegistration registration = mRegistrations.valueAt(i); if (registration.isActive()) { @@ -413,17 +436,17 @@ public abstract class ListenerMultiplexer predicate) { + synchronized (mMultiplexerLock) { + // we only acquire a reentrancy guard in case of removal while iterating. this method + // does not directly affect active state or merged state, so there is no advantage to + // acquiring an update source buffer. + try (ReentrancyGuard ignored = mReentrancyGuard.acquire()) { + final int size = mRegistrations.size(); + for (int i = 0; i < size; i++) { + TRegistration registration = mRegistrations.valueAt(i); + if (predicate.test(registration)) { + return true; + } + } + } + + return false; + } } /** @@ -463,7 +510,7 @@ public abstract class ListenerMultiplexer predicate) { - synchronized (mRegistrations) { + synchronized (mMultiplexerLock) { // since updating a registration can invoke a variety of callbacks, we need to ensure // those callbacks themselves do not re-enter, as this could lead to out-of-order // callbacks. note that try-with-resources ordering is meaningful here as well. we want @@ -492,7 +539,7 @@ public abstract class ListenerMultiplexer predicate) { - synchronized (mRegistrations) { + synchronized (mMultiplexerLock) { // since updating a registration can invoke a variety of callbacks, we need to ensure // those callbacks themselves do not re-enter, as this could lead to out-of-order // callbacks. note that try-with-resources ordering is meaningful here as well. we want @@ -515,12 +562,8 @@ public abstract class ListenerMultiplexer> function) { - synchronized (mRegistrations) { + synchronized (mMultiplexerLock) { try (ReentrancyGuard ignored = mReentrancyGuard.acquire()) { final int size = mRegistrations.size(); for (int i = 0; i < size; i++) { @@ -571,7 +614,7 @@ public abstract class ListenerMultiplexer */ protected final void deliverToListeners(@NonNull ListenerOperation operation) { - synchronized (mRegistrations) { + synchronized (mMultiplexerLock) { try (ReentrancyGuard ignored = mReentrancyGuard.acquire()) { final int size = mRegistrations.size(); for (int i = 0; i < size; i++) { @@ -584,6 +627,7 @@ public abstract class ListenerMultiplexer>> mScheduledRemovals; + + @GuardedBy("mMultiplexerLock") + @Nullable private ArraySet>> mScheduledRemovals; ReentrancyGuard() { mGuardCount = 0; mScheduledRemovals = null; } - @GuardedBy("mRegistrations") boolean isReentrant() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mRegistrations)); + synchronized (mMultiplexerLock) { + return mGuardCount != 0; } - return mGuardCount != 0; } - @GuardedBy("mRegistrations") - void markForRemoval(Object key, ListenerRegistration registration) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mRegistrations)); - } - Preconditions.checkState(isReentrant()); + void markForRemoval(TKey key, ListenerRegistration registration) { + synchronized (mMultiplexerLock) { + Preconditions.checkState(isReentrant()); - if (mScheduledRemovals == null) { - mScheduledRemovals = new ArraySet<>(mRegistrations.size()); + if (mScheduledRemovals == null) { + mScheduledRemovals = new ArraySet<>(mRegistrations.size()); + } + mScheduledRemovals.add(new AbstractMap.SimpleImmutableEntry<>(key, registration)); } - mScheduledRemovals.add(new AbstractMap.SimpleImmutableEntry<>(key, registration)); } ReentrancyGuard acquire() { - ++mGuardCount; - return this; + synchronized (mMultiplexerLock) { + ++mGuardCount; + return this; + } } @Override public void close() { - ArraySet>> scheduledRemovals = null; + synchronized (mMultiplexerLock) { + Preconditions.checkState(mGuardCount > 0); - Preconditions.checkState(mGuardCount > 0); - if (--mGuardCount == 0) { - scheduledRemovals = mScheduledRemovals; - mScheduledRemovals = null; - } + ArraySet>> scheduledRemovals = null; - if (scheduledRemovals == null) { - return; - } + if (--mGuardCount == 0) { + scheduledRemovals = mScheduledRemovals; + mScheduledRemovals = null; + } - try (UpdateServiceBuffer ignored = mUpdateServiceBuffer.acquire()) { - final int size = scheduledRemovals.size(); - for (int i = 0; i < size; i++) { - Entry> entry = scheduledRemovals.valueAt(i); - removeRegistration(entry.getKey(), entry.getValue()); + if (scheduledRemovals == null) { + return; + } + + try (UpdateServiceBuffer ignored = mUpdateServiceBuffer.acquire()) { + final int size = scheduledRemovals.size(); + for (int i = 0; i < size; i++) { + Entry> entry = scheduledRemovals.valueAt(i); + removeRegistration(entry.getKey(), entry.getValue()); + } } } } @@ -721,6 +768,7 @@ public abstract class ListenerMultiplexer.UpdateServiceBuffer mUpdateServiceBuffer; - UpdateServiceLock(UpdateServiceBuffer updateServiceBuffer) { - mUpdateServiceBuffer = updateServiceBuffer; + UpdateServiceLock(ListenerMultiplexer.UpdateServiceBuffer updateServiceBuffer) { + mUpdateServiceBuffer = updateServiceBuffer.acquire(); } @Override public void close() { if (mUpdateServiceBuffer != null) { - UpdateServiceBuffer buffer = mUpdateServiceBuffer; + ListenerMultiplexer.UpdateServiceBuffer buffer = mUpdateServiceBuffer; mUpdateServiceBuffer = null; buffer.close(); } diff --git a/services/core/java/com/android/server/location/listeners/ListenerRegistration.java b/services/core/java/com/android/server/location/listeners/ListenerRegistration.java index 711dde89ef139..fcb2a9b70336e 100644 --- a/services/core/java/com/android/server/location/listeners/ListenerRegistration.java +++ b/services/core/java/com/android/server/location/listeners/ListenerRegistration.java @@ -35,7 +35,7 @@ public class ListenerRegistration implements ListenerExecutor { private boolean mActive; - private volatile @Nullable TListener mListener; + @Nullable private volatile TListener mListener; protected ListenerRegistration(Executor executor, TListener listener) { mExecutor = Objects.requireNonNull(executor); @@ -43,6 +43,13 @@ public class ListenerRegistration implements ListenerExecutor { mListener = Objects.requireNonNull(listener); } + /** + * Returns a tag to use for logging. Should be overridden by subclasses. + */ + protected String getTag() { + return "ListenerRegistration"; + } + protected final Executor getExecutor() { return mExecutor; } @@ -50,26 +57,36 @@ public class ListenerRegistration implements ListenerExecutor { /** * May be overridden by subclasses. Invoked when registration occurs. Invoked while holding the * owning multiplexer's internal lock. + * + *

If overridden you must ensure the superclass method is invoked (usually as the first thing + * in the overridden method). */ protected void onRegister(Object key) {} /** * May be overridden by subclasses. Invoked when unregistration occurs. Invoked while holding * the owning multiplexer's internal lock. + * + *

If overridden you must ensure the superclass method is invoked (usually as the last thing + * in the overridden method). */ protected void onUnregister() {} /** - * May be overridden by subclasses. Invoked when this registration becomes active. If this - * returns a non-null operation, that operation will be invoked for the listener. Invoked - * while holding the owning multiplexer's internal lock. + * May be overridden by subclasses. Invoked when this registration becomes active. Invoked while + * holding the owning multiplexer's internal lock. + * + *

If overridden you must ensure the superclass method is invoked (usually as the first thing + * in the overridden method). */ protected void onActive() {} /** - * May be overridden by subclasses. Invoked when registration becomes inactive. If this returns - * a non-null operation, that operation will be invoked for the listener. Invoked while holding - * the owning multiplexer's internal lock. + * May be overridden by subclasses. Invoked when registration becomes inactive. Invoked while + * holding the owning multiplexer's internal lock. + * + *

If overridden you must ensure the superclass method is invoked (usually as the last thing + * in the overridden method). */ protected void onInactive() {} diff --git a/services/core/java/com/android/server/location/listeners/PendingIntentListenerRegistration.java b/services/core/java/com/android/server/location/listeners/PendingIntentListenerRegistration.java index 240ac0144293a..c976601e61707 100644 --- a/services/core/java/com/android/server/location/listeners/PendingIntentListenerRegistration.java +++ b/services/core/java/com/android/server/location/listeners/PendingIntentListenerRegistration.java @@ -16,63 +16,47 @@ package com.android.server.location.listeners; -import android.annotation.Nullable; +import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR; + import android.app.PendingIntent; -import android.location.util.identity.CallerIdentity; import android.util.Log; /** * A registration that works with PendingIntent keys, and registers a CancelListener to - * automatically remove the registration if the PendingIntent is canceled. The key for this - * registration must either be a {@link PendingIntent} or a {@link PendingIntentKey}. + * automatically remove the registration if the PendingIntent is canceled. * - * @param request type + * @param key type * @param listener type */ -public abstract class PendingIntentListenerRegistration extends - RemoteListenerRegistration implements PendingIntent.CancelListener { +public abstract class PendingIntentListenerRegistration extends + RemovableListenerRegistration implements PendingIntent.CancelListener { - /** - * Interface to allowed pending intent retrieval when keys are not themselves PendingIntents. - */ - public interface PendingIntentKey { - /** - * Returns the pending intent associated with this key. - */ - PendingIntent getPendingIntent(); + protected PendingIntentListenerRegistration(TListener listener) { + super(DIRECT_EXECUTOR, listener); } - protected PendingIntentListenerRegistration(@Nullable TRequest request, - CallerIdentity callerIdentity, TListener listener) { - super(request, callerIdentity, listener); + protected abstract PendingIntent getPendingIntentFromKey(TKey key); + + @Override + protected void onRegister() { + super.onRegister(); + + if (!getPendingIntentFromKey(getKey()).addCancelListener(DIRECT_EXECUTOR, this)) { + remove(); + } } @Override - protected final void onRemovableListenerRegister() { - getPendingIntentFromKey(getKey()).registerCancelListener(this); - onPendingIntentListenerRegister(); + protected void onUnregister() { + getPendingIntentFromKey(getKey()).removeCancelListener(this); + + super.onUnregister(); } - @Override - protected final void onRemovableListenerUnregister() { - onPendingIntentListenerUnregister(); - getPendingIntentFromKey(getKey()).unregisterCancelListener(this); - } - - /** - * May be overridden in place of {@link #onRemovableListenerRegister()}. - */ - protected void onPendingIntentListenerRegister() {} - - /** - * May be overridden in place of {@link #onRemovableListenerUnregister()}. - */ - protected void onPendingIntentListenerUnregister() {} - @Override protected void onOperationFailure(ListenerOperation operation, Exception e) { if (e instanceof PendingIntent.CanceledException) { - Log.w(getOwner().getTag(), "registration " + this + " removed", e); + Log.w(getTag(), "registration " + this + " removed", e); remove(); } else { super.onOperationFailure(operation, e); @@ -81,21 +65,10 @@ public abstract class PendingIntentListenerRegistration ext @Override public void onCanceled(PendingIntent intent) { - if (Log.isLoggable(getOwner().getTag(), Log.DEBUG)) { - Log.d(getOwner().getTag(), - "pending intent registration " + getIdentity() + " canceled"); + if (Log.isLoggable(getTag(), Log.DEBUG)) { + Log.d(getTag(), "pending intent registration " + this + " canceled"); } remove(); } - - private PendingIntent getPendingIntentFromKey(Object key) { - if (key instanceof PendingIntent) { - return (PendingIntent) key; - } else if (key instanceof PendingIntentKey) { - return ((PendingIntentKey) key).getPendingIntent(); - } else { - throw new IllegalArgumentException("key must be PendingIntent or PendingIntentKey"); - } - } -} +} \ No newline at end of file diff --git a/services/core/java/com/android/server/location/listeners/RemoteListenerRegistration.java b/services/core/java/com/android/server/location/listeners/RemoteListenerRegistration.java deleted file mode 100644 index 4eca577dcf4f5..0000000000000 --- a/services/core/java/com/android/server/location/listeners/RemoteListenerRegistration.java +++ /dev/null @@ -1,75 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.server.location.listeners; - - -import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR; - -import android.annotation.Nullable; -import android.location.util.identity.CallerIdentity; -import android.os.Process; - -import com.android.internal.annotations.VisibleForTesting; -import com.android.server.FgThread; - -import java.util.Objects; -import java.util.concurrent.Executor; - -/** - * A listener registration representing a remote (possibly from a different process) listener. - * Listeners from a different process will be run on a direct executor, since the x-process listener - * invocation should already be asynchronous. Listeners from the same process will be run on a - * normal executor, since in-process listener invocation may be synchronous. - * - * @param request type - * @param listener type - */ -public abstract class RemoteListenerRegistration extends - RemovableListenerRegistration { - - @VisibleForTesting - public static final Executor IN_PROCESS_EXECUTOR = FgThread.getExecutor(); - - private static Executor chooseExecutor(CallerIdentity identity) { - // if a client is in the same process as us, binder calls will execute synchronously and - // we shouldn't run callbacks directly since they might be run under lock and deadlock - if (identity.getPid() == Process.myPid()) { - // there's a slight loophole here for pending intents - pending intent callbacks can - // always be run on the direct executor since they're always asynchronous, but honestly - // you shouldn't be using pending intent callbacks within the same process anyways - return IN_PROCESS_EXECUTOR; - } else { - return DIRECT_EXECUTOR; - } - } - - private final CallerIdentity mIdentity; - - protected RemoteListenerRegistration(@Nullable TRequest request, CallerIdentity identity, - TListener listener) { - super(chooseExecutor(identity), request, listener); - mIdentity = Objects.requireNonNull(identity); - } - - /** - * Returns the listener identity. - */ - public final CallerIdentity getIdentity() { - return mIdentity; - } -} - diff --git a/services/core/java/com/android/server/location/listeners/RemovableListenerRegistration.java b/services/core/java/com/android/server/location/listeners/RemovableListenerRegistration.java index 618ff24b873b0..3c302fbfaa63e 100644 --- a/services/core/java/com/android/server/location/listeners/RemovableListenerRegistration.java +++ b/services/core/java/com/android/server/location/listeners/RemovableListenerRegistration.java @@ -20,22 +20,23 @@ import android.annotation.Nullable; import java.util.Objects; import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicBoolean; /** * A listener registration that stores its own key, and thus can remove itself. By default it will * remove itself if any checked exception occurs on listener execution. * - * @param request type + * @param key type * @param listener type */ -public abstract class RemovableListenerRegistration extends - RequestListenerRegistration { +public abstract class RemovableListenerRegistration extends + ListenerRegistration { - private volatile @Nullable Object mKey; + @Nullable private volatile TKey mKey; + private final AtomicBoolean mRemoved = new AtomicBoolean(false); - protected RemovableListenerRegistration(Executor executor, @Nullable TRequest request, - TListener listener) { - super(executor, request, listener); + protected RemovableListenerRegistration(Executor executor, TListener listener) { + super(executor, listener); } /** @@ -43,46 +44,76 @@ public abstract class RemovableListenerRegistration extends * with. Often this is easiest to accomplish by defining registration subclasses as non-static * inner classes of the multiplexer they are to be used with. */ - protected abstract ListenerMultiplexer getOwner(); + protected abstract ListenerMultiplexer getOwner(); /** * Returns the key associated with this registration. May not be invoked before * {@link #onRegister(Object)} or after {@link #onUnregister()}. */ - protected final Object getKey() { + protected final TKey getKey() { return Objects.requireNonNull(mKey); } /** - * Removes this registration. Does nothing if invoked before {@link #onRegister(Object)} or - * after {@link #onUnregister()}. It is safe to invoke this from within either function. + * Convenience method equivalent to invoking {@link #remove(boolean)} with the + * {@code immediately} parameter set to true. */ public final void remove() { - Object key = mKey; - if (key != null) { - getOwner().removeRegistration(key, this); + remove(true); + } + + /** + * Removes this registration. If the {@code immediately} parameter is true, all pending listener + * invocations will fail. If the {@code immediately} parameter is false, listener invocations + * that were scheduled before remove was invoked (including invocations scheduled within {@link + * #onRemove(boolean)}) will continue, but any listener invocations scheduled after remove was + * invoked will fail. + * + *

Only the first call to this method will ever go through (and so {@link #onRemove(boolean)} + * will only ever be invoked once). + * + *

Does nothing if invoked before {@link #onRegister()} or after {@link #onUnregister()}. + */ + public final void remove(boolean immediately) { + TKey key = mKey; + if (key != null && !mRemoved.getAndSet(true)) { + onRemove(immediately); + if (immediately) { + getOwner().removeRegistration(key, this); + } else { + executeOperation(listener -> getOwner().removeRegistration(key, this)); + } } } + /** + * Invoked just before this registration is removed due to {@link #remove(boolean)}, on the same + * thread as the responsible {@link #remove(boolean)} call. + * + *

This method will only ever be invoked once, no matter how many calls to {@link + * #remove(boolean)} are made, as any registration can only be removed once. + */ + protected void onRemove(boolean immediately) {} + @Override protected final void onRegister(Object key) { - mKey = Objects.requireNonNull(key); - onRemovableListenerRegister(); + super.onRegister(key); + mKey = (TKey) Objects.requireNonNull(key); + onRegister(); } + /** + * May be overridden by subclasses. Invoked when registration occurs. Invoked while holding the + * owning multiplexer's internal lock. + * + *

If overridden you must ensure the superclass method is invoked (usually as the first thing + * in the overridden method). + */ + protected void onRegister() {} + @Override - protected final void onUnregister() { - onRemovableListenerUnregister(); + protected void onUnregister() { mKey = null; + super.onUnregister(); } - - /** - * May be overridden in place of {@link #onRegister(Object)}. - */ - protected void onRemovableListenerRegister() {} - - /** - * May be overridden in place of {@link #onUnregister()}. - */ - protected void onRemovableListenerUnregister() {} } diff --git a/services/core/java/com/android/server/location/listeners/RequestListenerRegistration.java b/services/core/java/com/android/server/location/listeners/RequestListenerRegistration.java deleted file mode 100644 index 0c2fc9142d92d..0000000000000 --- a/services/core/java/com/android/server/location/listeners/RequestListenerRegistration.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.server.location.listeners; - -import java.util.concurrent.Executor; - -/** - * A listener registration object which includes an associated request. - * - * @param request type - * @param listener type - */ -public class RequestListenerRegistration extends - ListenerRegistration { - - private final TRequest mRequest; - - protected RequestListenerRegistration(Executor executor, TRequest request, - TListener listener) { - super(executor, listener); - mRequest = request; - } - - /** - * Returns the request associated with this listener, or null if one wasn't supplied. - */ - public TRequest getRequest() { - return mRequest; - } - - @Override - public String toString() { - if (mRequest == null) { - return "[]"; - } else { - return mRequest.toString(); - } - } -} - diff --git a/services/core/java/com/android/server/location/provider/LocationProviderManager.java b/services/core/java/com/android/server/location/provider/LocationProviderManager.java index 549fd49180230..a69a079b679d9 100644 --- a/services/core/java/com/android/server/location/provider/LocationProviderManager.java +++ b/services/core/java/com/android/server/location/provider/LocationProviderManager.java @@ -67,7 +67,6 @@ import android.location.provider.ProviderProperties; import android.location.provider.ProviderRequest; import android.location.util.identity.CallerIdentity; import android.os.Binder; -import android.os.Build; import android.os.Bundle; import android.os.CancellationSignal; import android.os.IBinder; @@ -115,7 +114,7 @@ import com.android.server.location.injector.SettingsHelper.UserSettingChangedLis import com.android.server.location.injector.UserInfoHelper; import com.android.server.location.injector.UserInfoHelper.UserListener; import com.android.server.location.listeners.ListenerMultiplexer; -import com.android.server.location.listeners.RemoteListenerRegistration; +import com.android.server.location.listeners.RemovableListenerRegistration; import com.android.server.location.settings.LocationSettings; import com.android.server.location.settings.LocationUserSettings; @@ -124,8 +123,10 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.util.ArrayList; import java.util.Collection; +import java.util.NoSuchElementException; import java.util.Objects; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.Executor; import java.util.function.Predicate; /** @@ -354,44 +355,60 @@ public class LocationProviderManager extends public void deliverOnFlushComplete(int requestCode) {} } - protected abstract class Registration extends RemoteListenerRegistration { + protected abstract class Registration extends RemovableListenerRegistration { + private final LocationRequest mBaseRequest; + private final CallerIdentity mIdentity; private final @PermissionLevel int mPermissionLevel; // we cache these values because checking/calculating on the fly is more expensive + @GuardedBy("mMultiplexerLock") private boolean mPermitted; + @GuardedBy("mMultiplexerLock") private boolean mForeground; + @GuardedBy("mMultiplexerLock") private LocationRequest mProviderLocationRequest; + @GuardedBy("mMultiplexerLock") private boolean mIsUsingHighPower; - private @Nullable Location mLastLocation = null; + @Nullable private Location mLastLocation = null; - protected Registration(LocationRequest request, CallerIdentity identity, + protected Registration(LocationRequest request, CallerIdentity identity, Executor executor, LocationTransport transport, @PermissionLevel int permissionLevel) { - super(Objects.requireNonNull(request), identity, transport); + super(executor, transport); Preconditions.checkArgument(identity.getListenerId() != null); Preconditions.checkArgument(permissionLevel > PERMISSION_NONE); Preconditions.checkArgument(!request.getWorkSource().isEmpty()); + mBaseRequest = Objects.requireNonNull(request); + mIdentity = Objects.requireNonNull(identity); mPermissionLevel = permissionLevel; mProviderLocationRequest = request; } - @GuardedBy("mLock") - @Override - protected final void onRemovableListenerRegister() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); + public final CallerIdentity getIdentity() { + return mIdentity; + } + + public final LocationRequest getRequest() { + synchronized (mMultiplexerLock) { + return mProviderLocationRequest; } + } + + @GuardedBy("mMultiplexerLock") + @Override + protected void onRegister() { + super.onRegister(); if (D) { Log.d(TAG, mName + " provider added registration from " + getIdentity() + " -> " + getRequest()); } - EVENT_LOG.logProviderClientRegistered(mName, getIdentity(), super.getRequest()); + EVENT_LOG.logProviderClientRegistered(mName, getIdentity(), mBaseRequest); // initialization order is important as there are ordering dependencies mPermitted = mLocationPermissionsHelper.hasLocationPermissions(mPermissionLevel, @@ -400,110 +417,72 @@ public class LocationProviderManager extends mProviderLocationRequest = calculateProviderLocationRequest(); mIsUsingHighPower = isUsingHighPower(); - onProviderListenerRegister(); - if (mForeground) { EVENT_LOG.logProviderClientForeground(mName, getIdentity()); } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override - protected final void onRemovableListenerUnregister() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - - onProviderListenerUnregister(); - + protected void onUnregister() { EVENT_LOG.logProviderClientUnregistered(mName, getIdentity()); if (D) { Log.d(TAG, mName + " provider removed registration from " + getIdentity()); } + + super.onUnregister(); } - /** - * Subclasses may override this instead of {@link #onRemovableListenerRegister()}. - */ - @GuardedBy("mLock") - protected void onProviderListenerRegister() {} - - /** - * Subclasses may override this instead of {@link #onRemovableListenerUnregister()}. - */ - @GuardedBy("mLock") - protected void onProviderListenerUnregister() {} - + @GuardedBy("mMultiplexerLock") @Override - protected final void onActive() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - + protected void onActive() { EVENT_LOG.logProviderClientActive(mName, getIdentity()); if (!getRequest().isHiddenFromAppOps()) { mAppOpsHelper.startOpNoThrow(OP_MONITOR_LOCATION, getIdentity()); } onHighPowerUsageChanged(); - - onProviderListenerActive(); } + @GuardedBy("mMultiplexerLock") @Override - protected final void onInactive() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - + protected void onInactive() { onHighPowerUsageChanged(); if (!getRequest().isHiddenFromAppOps()) { mAppOpsHelper.finishOp(OP_MONITOR_LOCATION, getIdentity()); } - onProviderListenerInactive(); - EVENT_LOG.logProviderClientInactive(mName, getIdentity()); } - /** - * Subclasses may override this instead of {@link #onActive()}. - */ - @GuardedBy("mLock") - protected void onProviderListenerActive() {} - - /** - * Subclasses may override this instead of {@link #onInactive()} ()}. - */ - @GuardedBy("mLock") - protected void onProviderListenerInactive() {} - - @Override - public final LocationRequest getRequest() { - return mProviderLocationRequest; - } - - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") final void setLastDeliveredLocation(@Nullable Location location) { mLastLocation = location; } - @GuardedBy("mLock") public final Location getLastDeliveredLocation() { - return mLastLocation; + synchronized (mMultiplexerLock) { + return mLastLocation; + } } public @PermissionLevel int getPermissionLevel() { - return mPermissionLevel; + synchronized (mMultiplexerLock) { + return mPermissionLevel; + } } public final boolean isForeground() { - return mForeground; + synchronized (mMultiplexerLock) { + return mForeground; + } } public final boolean isPermitted() { - return mPermitted; + synchronized (mMultiplexerLock) { + return mPermitted; + } } public final void flush(int requestCode) { @@ -519,13 +498,14 @@ public class LocationProviderManager extends return LocationProviderManager.this; } - @GuardedBy("mLock") final boolean onProviderPropertiesChanged() { - onHighPowerUsageChanged(); - return false; + synchronized (mMultiplexerLock) { + onHighPowerUsageChanged(); + return false; + } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private void onHighPowerUsageChanged() { boolean isUsingHighPower = isUsingHighPower(); if (isUsingHighPower != mIsUsingHighPower) { @@ -541,12 +521,7 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") private boolean isUsingHighPower() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - ProviderProperties properties = getProperties(); if (properties == null) { return false; @@ -557,30 +532,28 @@ public class LocationProviderManager extends && properties.getPowerUsage() == ProviderProperties.POWER_USAGE_HIGH; } - @GuardedBy("mLock") final boolean onLocationPermissionsChanged(@Nullable String packageName) { - if (packageName == null || getIdentity().getPackageName().equals(packageName)) { - return onLocationPermissionsChanged(); - } + synchronized (mMultiplexerLock) { + if (packageName == null || getIdentity().getPackageName().equals(packageName)) { + return onLocationPermissionsChanged(); + } - return false; + return false; + } } - @GuardedBy("mLock") final boolean onLocationPermissionsChanged(int uid) { - if (getIdentity().getUid() == uid) { - return onLocationPermissionsChanged(); - } + synchronized (mMultiplexerLock) { + if (getIdentity().getUid() == uid) { + return onLocationPermissionsChanged(); + } - return false; + return false; + } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private boolean onLocationPermissionsChanged() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - boolean permitted = mLocationPermissionsHelper.hasLocationPermissions(mPermissionLevel, getIdentity()); if (permitted != mPermitted) { @@ -603,82 +576,73 @@ public class LocationProviderManager extends return false; } - @GuardedBy("mLock") final boolean onAdasGnssLocationEnabledChanged(int userId) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - - if (getIdentity().getUserId() == userId) { - return onProviderLocationRequestChanged(); - } - - return false; - } - - @GuardedBy("mLock") - final boolean onForegroundChanged(int uid, boolean foreground) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - - if (getIdentity().getUid() == uid && foreground != mForeground) { - if (D) { - Log.v(TAG, mName + " provider uid " + uid + " foreground = " + foreground); + synchronized (mMultiplexerLock) { + if (getIdentity().getUserId() == userId) { + return onProviderLocationRequestChanged(); } - mForeground = foreground; - - if (mForeground) { - EVENT_LOG.logProviderClientForeground(mName, getIdentity()); - } else { - EVENT_LOG.logProviderClientBackground(mName, getIdentity()); - } - - // note that onProviderLocationRequestChanged() is always called - return onProviderLocationRequestChanged() - || mLocationPowerSaveModeHelper.getLocationPowerSaveMode() - == LOCATION_MODE_FOREGROUND_ONLY; - } - - return false; - } - - @GuardedBy("mLock") - final boolean onProviderLocationRequestChanged() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - - LocationRequest newRequest = calculateProviderLocationRequest(); - if (mProviderLocationRequest.equals(newRequest)) { return false; } - - LocationRequest oldRequest = mProviderLocationRequest; - mProviderLocationRequest = newRequest; - onHighPowerUsageChanged(); - updateService(); - - // if bypass state has changed then the active state may have changed - return oldRequest.isBypass() != newRequest.isBypass(); } + final boolean onForegroundChanged(int uid, boolean foreground) { + synchronized (mMultiplexerLock) { + if (getIdentity().getUid() == uid && foreground != mForeground) { + if (D) { + Log.v(TAG, mName + " provider uid " + uid + " foreground = " + foreground); + } + + mForeground = foreground; + + if (mForeground) { + EVENT_LOG.logProviderClientForeground(mName, getIdentity()); + } else { + EVENT_LOG.logProviderClientBackground(mName, getIdentity()); + } + + // note that onProviderLocationRequestChanged() is always called + return onProviderLocationRequestChanged() + || mLocationPowerSaveModeHelper.getLocationPowerSaveMode() + == LOCATION_MODE_FOREGROUND_ONLY; + } + + return false; + } + } + + final boolean onProviderLocationRequestChanged() { + synchronized (mMultiplexerLock) { + LocationRequest newRequest = calculateProviderLocationRequest(); + if (mProviderLocationRequest.equals(newRequest)) { + return false; + } + + LocationRequest oldRequest = mProviderLocationRequest; + mProviderLocationRequest = newRequest; + onHighPowerUsageChanged(); + updateService(); + + // if bypass state has changed then the active state may have changed + return oldRequest.isBypass() != newRequest.isBypass(); + } + } + + @GuardedBy("mMultiplexerLock") private LocationRequest calculateProviderLocationRequest() { - LocationRequest baseRequest = super.getRequest(); - LocationRequest.Builder builder = new LocationRequest.Builder(baseRequest); + LocationRequest.Builder builder = new LocationRequest.Builder(mBaseRequest); if (mPermissionLevel < PERMISSION_FINE) { builder.setQuality(LocationRequest.QUALITY_LOW_POWER); - if (baseRequest.getIntervalMillis() < MIN_COARSE_INTERVAL_MS) { + if (mBaseRequest.getIntervalMillis() < MIN_COARSE_INTERVAL_MS) { builder.setIntervalMillis(MIN_COARSE_INTERVAL_MS); } - if (baseRequest.getMinUpdateIntervalMillis() < MIN_COARSE_INTERVAL_MS) { + if (mBaseRequest.getMinUpdateIntervalMillis() < MIN_COARSE_INTERVAL_MS) { builder.setMinUpdateIntervalMillis(MIN_COARSE_INTERVAL_MS); } } - boolean locationSettingsIgnored = baseRequest.isLocationSettingsIgnored(); + boolean locationSettingsIgnored = mBaseRequest.isLocationSettingsIgnored(); if (locationSettingsIgnored) { // if we are not currently allowed use location settings ignored, disable it if (!mSettingsHelper.getIgnoreSettingsAllowlist().contains( @@ -690,7 +654,7 @@ public class LocationProviderManager extends builder.setLocationSettingsIgnored(locationSettingsIgnored); } - boolean adasGnssBypass = baseRequest.isAdasGnssBypass(); + boolean adasGnssBypass = mBaseRequest.isAdasGnssBypass(); if (adasGnssBypass) { // if we are not currently allowed use adas gnss bypass, disable it if (!GPS_PROVIDER.equals(mName)) { @@ -710,7 +674,7 @@ public class LocationProviderManager extends if (!locationSettingsIgnored && !isThrottlingExempt()) { // throttle in the background if (!mForeground) { - builder.setIntervalMillis(max(baseRequest.getIntervalMillis(), + builder.setIntervalMillis(max(mBaseRequest.getIntervalMillis(), mSettingsHelper.getBackgroundThrottleIntervalMs())); } } @@ -727,8 +691,7 @@ public class LocationProviderManager extends return mLocationManagerInternal.isProvider(null, getIdentity()); } - @GuardedBy("mLock") - abstract @Nullable ListenerOperation acceptLocationChange( + @Nullable abstract ListenerOperation acceptLocationChange( LocationResult fineLocationResult); @Override @@ -769,13 +732,19 @@ public class LocationProviderManager extends final ExternalWakeLockReleaser mWakeLockReleaser; private volatile ProviderTransport mProviderTransport; + + @GuardedBy("mMultiplexerLock") private int mNumLocationsDelivered = 0; + @GuardedBy("mMultiplexerLock") private long mExpirationRealtimeMs = Long.MAX_VALUE; protected LocationRegistration( - LocationRequest request, CallerIdentity identity, TTransport transport, + LocationRequest request, + CallerIdentity identity, + Executor executor, + TTransport transport, @PermissionLevel int permissionLevel) { - super(request, identity, transport, permissionLevel); + super(request, identity, executor, transport, permissionLevel); mProviderTransport = transport; mWakeLock = Objects.requireNonNull(mContext.getSystemService(PowerManager.class)) .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, WAKELOCK_TAG); @@ -789,9 +758,13 @@ public class LocationProviderManager extends mProviderTransport = null; } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected final void onProviderListenerRegister() { + protected void onRegister() { + super.onRegister(); + long registerTimeMs = SystemClock.elapsedRealtime(); mExpirationRealtimeMs = getRequest().getExpirationRealtimeMs(registerTimeMs); @@ -810,8 +783,6 @@ public class LocationProviderManager extends // start listening for provider enabled/disabled events addEnabledListener(this); - onLocationListenerRegister(); - // if the provider is currently disabled, let the client know immediately int userId = getIdentity().getUserId(); if (!isEnabled(userId)) { @@ -819,9 +790,11 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected final void onProviderListenerUnregister() { + protected void onUnregister() { // stop listening for provider enabled/disabled events removeEnabledListener(this); @@ -830,24 +803,16 @@ public class LocationProviderManager extends mAlarmHelper.cancel(this); } - onLocationListenerUnregister(); + super.onUnregister(); } - /** - * Subclasses may override this instead of {@link #onRemovableListenerRegister()}. - */ - @GuardedBy("mLock") - protected void onLocationListenerRegister() {} - - /** - * Subclasses may override this instead of {@link #onRemovableListenerUnregister()}. - */ - @GuardedBy("mLock") - protected void onLocationListenerUnregister() {} - - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected final void onProviderListenerActive() { + protected void onActive() { + super.onActive(); + // a new registration may not get a location immediately, the provider request may be // delayed. therefore we deliver a historical location if available. since delivering an // older location could be considered a breaking change for some applications, we only @@ -883,21 +848,17 @@ public class LocationProviderManager extends + " expired at " + TimeUtils.formatRealtime(mExpirationRealtimeMs)); } - synchronized (mLock) { + synchronized (mMultiplexerLock) { // no need to remove alarm after it's fired mExpirationRealtimeMs = Long.MAX_VALUE; remove(); } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override @Nullable ListenerOperation acceptLocationChange( LocationResult fineLocationResult) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - // check expiration time - alarm is not guaranteed to go off at the right time, // especially for short intervals if (SystemClock.elapsedRealtime() >= mExpirationRealtimeMs) { @@ -1017,9 +978,7 @@ public class LocationProviderManager extends + " finished after " + mNumLocationsDelivered + " updates"); } - synchronized (mLock) { - remove(); - } + remove(); } } } @@ -1049,12 +1008,18 @@ public class LocationProviderManager extends LocationListenerRegistration(LocationRequest request, CallerIdentity identity, LocationListenerTransport transport, @PermissionLevel int permissionLevel) { - super(request, identity, transport, permissionLevel); + super(request, identity, + identity.isMyProcess() ? FgThread.getExecutor() : DIRECT_EXECUTOR, transport, + permissionLevel); } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onLocationListenerRegister() { + protected void onRegister() { + super.onRegister(); + try { ((IBinder) getKey()).linkToDeath(this, 0); } catch (RemoteException e) { @@ -1062,10 +1027,22 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onLocationListenerUnregister() { - ((IBinder) getKey()).unlinkToDeath(this, 0); + protected void onUnregister() { + try { + ((IBinder) getKey()).unlinkToDeath(this, 0); + } catch (NoSuchElementException e) { + // the only way this exception can occur should be if another exception has been + // thrown prior to registration completing, and that exception is currently + // unwinding the call stack and causing this cleanup. since that exception should + // crash us anyways, drop this exception so we're not hiding the original exception. + Log.w(getTag(), "failed to unregister binder death listener", e); + } + + super.onUnregister(); } @Override @@ -1083,9 +1060,7 @@ public class LocationProviderManager extends private void onTransportFailure(Exception e) { if (e instanceof RemoteException) { Log.w(TAG, mName + " provider registration " + getIdentity() + " removed", e); - synchronized (mLock) { - remove(); - } + remove(); } else { throw new AssertionError(e); } @@ -1098,9 +1073,7 @@ public class LocationProviderManager extends Log.d(TAG, mName + " provider registration " + getIdentity() + " died"); } - synchronized (mLock) { - remove(); - } + remove(); } catch (RuntimeException e) { // the caller may swallow runtime exceptions, so we rethrow as assertion errors to // ensure the crash is seen @@ -1115,21 +1088,27 @@ public class LocationProviderManager extends LocationPendingIntentRegistration(LocationRequest request, CallerIdentity identity, LocationPendingIntentTransport transport, @PermissionLevel int permissionLevel) { - super(request, identity, transport, permissionLevel); + super(request, identity, DIRECT_EXECUTOR, transport, permissionLevel); } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onLocationListenerRegister() { + protected void onRegister() { + super.onRegister(); if (!((PendingIntent) getKey()).addCancelListener(DIRECT_EXECUTOR, this)) { remove(); } } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onLocationListenerUnregister() { + protected void onUnregister() { ((PendingIntent) getKey()).removeCancelListener(this); + super.onUnregister(); } @Override @@ -1147,9 +1126,7 @@ public class LocationProviderManager extends private void onTransportFailure(Exception e) { if (e instanceof PendingIntent.CanceledException) { Log.w(TAG, mName + " provider registration " + getIdentity() + " removed", e); - synchronized (mLock) { - remove(); - } + remove(); } else { throw new AssertionError(e); } @@ -1161,25 +1138,32 @@ public class LocationProviderManager extends Log.d(TAG, mName + " provider registration " + getIdentity() + " canceled"); } - synchronized (mLock) { - remove(); - } + remove(); } } protected final class GetCurrentLocationListenerRegistration extends Registration implements IBinder.DeathRecipient, OnAlarmListener { + @GuardedBy("mMultiplexerLock") private long mExpirationRealtimeMs = Long.MAX_VALUE; GetCurrentLocationListenerRegistration(LocationRequest request, CallerIdentity identity, LocationTransport transport, int permissionLevel) { - super(request, identity, transport, permissionLevel); + super(request, + identity, + identity.isMyProcess() ? FgThread.getExecutor() : DIRECT_EXECUTOR, + transport, + permissionLevel); } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onProviderListenerRegister() { + protected void onRegister() { + super.onRegister(); + try { ((IBinder) getKey()).linkToDeath(this, 0); } catch (RemoteException e) { @@ -1202,20 +1186,36 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onProviderListenerUnregister() { + protected void onUnregister() { // remove alarm for expiration if (mExpirationRealtimeMs < Long.MAX_VALUE) { mAlarmHelper.cancel(this); } - ((IBinder) getKey()).unlinkToDeath(this, 0); + try { + ((IBinder) getKey()).unlinkToDeath(this, 0); + } catch (NoSuchElementException e) { + // the only way this exception can occur should be if another exception has been + // thrown prior to registration completing, and that exception is currently + // unwinding the call stack and causing this cleanup. since that exception should + // crash us anyways, drop this exception so we're not hiding the original exception. + Log.w(getTag(), "failed to unregister binder death listener", e); + } + + super.onUnregister(); } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onProviderListenerActive() { + protected void onActive() { + super.onActive(); + Location lastLocation = getLastLocationUnsafe( getIdentity().getUserId(), getPermissionLevel(), @@ -1226,17 +1226,19 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onProviderListenerInactive() { + protected void onInactive() { // if we go inactive for any reason, fail immediately executeOperation(acceptLocationChange(null)); + super.onInactive(); } + @GuardedBy("mMultiplexerLock") void deliverNull() { - synchronized (mLock) { - executeOperation(acceptLocationChange(null)); - } + executeOperation(acceptLocationChange(null)); } @Override @@ -1246,21 +1248,17 @@ public class LocationProviderManager extends + " expired at " + TimeUtils.formatRealtime(mExpirationRealtimeMs)); } - synchronized (mLock) { + synchronized (mMultiplexerLock) { // no need to remove alarm after it's fired mExpirationRealtimeMs = Long.MAX_VALUE; executeOperation(acceptLocationChange(null)); } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override @Nullable ListenerOperation acceptLocationChange( @Nullable LocationResult fineLocationResult) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - // check expiration time - alarm is not guaranteed to go off at the right time, // especially for short intervals if (SystemClock.elapsedRealtime() >= mExpirationRealtimeMs) { @@ -1311,9 +1309,7 @@ public class LocationProviderManager extends // on failure we're automatically removed anyways, no need to attempt removal // again if (success) { - synchronized (mLock) { - remove(); - } + remove(); } } }; @@ -1324,9 +1320,7 @@ public class LocationProviderManager extends Exception e) { if (e instanceof RemoteException) { Log.w(TAG, mName + " provider registration " + getIdentity() + " removed", e); - synchronized (mLock) { - remove(); - } + remove(); } else { throw new AssertionError(e); } @@ -1339,9 +1333,7 @@ public class LocationProviderManager extends Log.d(TAG, mName + " provider registration " + getIdentity() + " died"); } - synchronized (mLock) { - remove(); - } + remove(); } catch (RuntimeException e) { // the caller may swallow runtime exceptions, so we rethrow as assertion errors to // ensure the crash is seen @@ -1350,23 +1342,21 @@ public class LocationProviderManager extends } } - protected final Object mLock = new Object(); - protected final String mName; - private final @Nullable PassiveLocationProviderManager mPassiveManager; + @Nullable private final PassiveLocationProviderManager mPassiveManager; protected final Context mContext; - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private @State int mState; // maps of user id to value - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private final SparseBooleanArray mEnabled; // null or not present means unknown - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private final SparseArray mLastLocations; - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private final ArrayList mEnabledListeners; private final CopyOnWriteArrayList mProviderRequestListeners; @@ -1418,14 +1408,14 @@ public class LocationProviderManager extends private final ScreenInteractiveChangedListener mScreenInteractiveChangedListener = this::onScreenInteractiveChanged; - // acquiring mLock makes operations on mProvider atomic, but is otherwise unnecessary + // acquiring mMultiplexerLock makes operations on mProvider atomic, but is otherwise unnecessary protected final MockableLocationProvider mProvider; - @GuardedBy("mLock") - private @Nullable OnAlarmListener mDelayedRegister; + @GuardedBy("mMultiplexerLock") + @Nullable private OnAlarmListener mDelayedRegister; - @GuardedBy("mLock") - private @Nullable StateChangedListener mStateChangedListener; + @GuardedBy("mMultiplexerLock") + @Nullable private StateChangedListener mStateChangedListener; public LocationProviderManager(Context context, Injector injector, String name, @Nullable PassiveLocationProviderManager passiveManager) { @@ -1453,19 +1443,14 @@ public class LocationProviderManager extends mLocationUsageLogger = injector.getLocationUsageLogger(); mLocationFudger = new LocationFudger(mSettingsHelper.getCoarseLocationAccuracyM()); - mProvider = new MockableLocationProvider(mLock); + mProvider = new MockableLocationProvider(mMultiplexerLock); // set listener last, since this lets our reference escape mProvider.getController().setListener(this); } - @Override - public String getTag() { - return TAG; - } - public void startManager(@Nullable StateChangedListener listener) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState == STATE_STOPPED); mState = STATE_STARTED; mStateChangedListener = listener; @@ -1485,7 +1470,7 @@ public class LocationProviderManager extends } public void stopManager() { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState == STATE_STARTED); mState = STATE_STOPPING; @@ -1522,11 +1507,11 @@ public class LocationProviderManager extends return mProvider.getState(); } - public @Nullable CallerIdentity getProviderIdentity() { + @Nullable public CallerIdentity getProviderIdentity() { return mProvider.getState().identity; } - public @Nullable ProviderProperties getProperties() { + @Nullable public ProviderProperties getProperties() { return mProvider.getState().properties; } @@ -1543,7 +1528,7 @@ public class LocationProviderManager extends Preconditions.checkArgument(userId >= 0); - synchronized (mLock) { + synchronized (mMultiplexerLock) { int index = mEnabled.indexOfKey(userId); if (index < 0) { // this generally shouldn't occur, but might be possible due to race conditions @@ -1558,14 +1543,14 @@ public class LocationProviderManager extends } public void addEnabledListener(ProviderEnabledListener listener) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); mEnabledListeners.add(listener); } } public void removeEnabledListener(ProviderEnabledListener listener) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); mEnabledListeners.remove(listener); } @@ -1582,7 +1567,7 @@ public class LocationProviderManager extends } public void setRealProvider(@Nullable AbstractLocationProvider provider) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); final long identity = Binder.clearCallingIdentity(); @@ -1595,7 +1580,7 @@ public class LocationProviderManager extends } public void setMockProvider(@Nullable MockLocationProvider provider) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); EVENT_LOG.logProviderMocked(mName, provider != null); @@ -1622,7 +1607,7 @@ public class LocationProviderManager extends } public void setMockProviderAllowed(boolean enabled) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { if (!mProvider.isMock()) { throw new IllegalArgumentException(mName + " provider is not a test provider"); } @@ -1637,7 +1622,7 @@ public class LocationProviderManager extends } public void setMockProviderLocation(Location location) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { if (!mProvider.isMock()) { throw new IllegalArgumentException(mName + " provider is not a test provider"); } @@ -1659,7 +1644,7 @@ public class LocationProviderManager extends } } - public @Nullable Location getLastLocation(LastLocationRequest request, + @Nullable public Location getLastLocation(LastLocationRequest request, CallerIdentity identity, @PermissionLevel int permissionLevel) { request = calculateLastLocationRequest(request, identity); @@ -1732,7 +1717,7 @@ public class LocationProviderManager extends * location, even if the permissionLevel is coarse. You are responsible for coarsening the * location if necessary. */ - public @Nullable Location getLastLocationUnsafe(int userId, + @Nullable public Location getLastLocationUnsafe(int userId, @PermissionLevel int permissionLevel, boolean isBypass, long maximumAgeMs) { if (userId == UserHandle.USER_ALL) { @@ -1756,7 +1741,7 @@ public class LocationProviderManager extends Preconditions.checkArgument(userId >= 0); Location location; - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); LastLocation lastLocation = mLastLocations.get(userId); if (lastLocation == null) { @@ -1778,7 +1763,7 @@ public class LocationProviderManager extends } public void injectLastLocation(Location location, int userId) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); if (getLastLocationUnsafe(userId, PERMISSION_FINE, false, Long.MAX_VALUE) == null) { setLastLocation(location, userId); @@ -1800,7 +1785,7 @@ public class LocationProviderManager extends Preconditions.checkArgument(userId >= 0); - synchronized (mLock) { + synchronized (mMultiplexerLock) { LastLocation lastLocation = mLastLocations.get(userId); if (lastLocation == null) { lastLocation = new LastLocation(); @@ -1814,7 +1799,7 @@ public class LocationProviderManager extends } } - public @Nullable ICancellationSignal getCurrentLocation(LocationRequest request, + @Nullable public ICancellationSignal getCurrentLocation(LocationRequest request, CallerIdentity identity, int permissionLevel, ILocationCallback callback) { if (request.getDurationMillis() > MAX_GET_CURRENT_LOCATION_TIMEOUT_MS) { request = new LocationRequest.Builder(request) @@ -1829,7 +1814,7 @@ public class LocationProviderManager extends new GetCurrentLocationTransport(callback), permissionLevel); - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); final long ident = Binder.clearCallingIdentity(); try { @@ -1849,9 +1834,7 @@ public class LocationProviderManager extends () -> { final long ident = Binder.clearCallingIdentity(); try { - synchronized (mLock) { - removeRegistration(callback.asBinder(), registration); - } + removeRegistration(callback.asBinder(), registration); } catch (RuntimeException e) { // since this is within a oneway binder transaction there is nowhere // for exceptions to go - move onto another thread to crash system @@ -1885,7 +1868,7 @@ public class LocationProviderManager extends new LocationListenerTransport(listener), permissionLevel); - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); final long ident = Binder.clearCallingIdentity(); try { @@ -1904,7 +1887,7 @@ public class LocationProviderManager extends new LocationPendingIntentTransport(mContext, pendingIntent), permissionLevel); - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); final long identity = Binder.clearCallingIdentity(); try { @@ -1916,42 +1899,38 @@ public class LocationProviderManager extends } public void flush(ILocationListener listener, int requestCode) { - synchronized (mLock) { - final long identity = Binder.clearCallingIdentity(); - try { - boolean flushed = updateRegistration(listener.asBinder(), registration -> { - registration.flush(requestCode); - return false; - }); - if (!flushed) { - throw new IllegalArgumentException("unregistered listener cannot be flushed"); - } - } finally { - Binder.restoreCallingIdentity(identity); + final long identity = Binder.clearCallingIdentity(); + try { + boolean flushed = updateRegistration(listener.asBinder(), registration -> { + registration.flush(requestCode); + return false; + }); + if (!flushed) { + throw new IllegalArgumentException("unregistered listener cannot be flushed"); } + } finally { + Binder.restoreCallingIdentity(identity); } } public void flush(PendingIntent pendingIntent, int requestCode) { - synchronized (mLock) { - final long identity = Binder.clearCallingIdentity(); - try { - boolean flushed = updateRegistration(pendingIntent, registration -> { - registration.flush(requestCode); - return false; - }); - if (!flushed) { - throw new IllegalArgumentException( - "unregistered pending intent cannot be flushed"); - } - } finally { - Binder.restoreCallingIdentity(identity); + final long identity = Binder.clearCallingIdentity(); + try { + boolean flushed = updateRegistration(pendingIntent, registration -> { + registration.flush(requestCode); + return false; + }); + if (!flushed) { + throw new IllegalArgumentException( + "unregistered pending intent cannot be flushed"); } + } finally { + Binder.restoreCallingIdentity(identity); } } public void unregisterLocationRequest(ILocationListener listener) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); final long identity = Binder.clearCallingIdentity(); try { @@ -1963,7 +1942,7 @@ public class LocationProviderManager extends } public void unregisterLocationRequest(PendingIntent pendingIntent) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { Preconditions.checkState(mState != STATE_STOPPED); final long identity = Binder.clearCallingIdentity(); try { @@ -1974,13 +1953,9 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected void onRegister() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - mSettingsHelper.addOnBackgroundThrottleIntervalChangedListener( mBackgroundThrottleIntervalChangedListener); mSettingsHelper.addOnBackgroundThrottlePackageWhitelistChangedListener( @@ -1997,13 +1972,9 @@ public class LocationProviderManager extends mScreenInteractiveHelper.addListener(mScreenInteractiveChangedListener); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected void onUnregister() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - mSettingsHelper.removeOnBackgroundThrottleIntervalChangedListener( mBackgroundThrottleIntervalChangedListener); mSettingsHelper.removeOnBackgroundThrottlePackageWhitelistChangedListener( @@ -2019,13 +1990,9 @@ public class LocationProviderManager extends mScreenInteractiveHelper.removeListener(mScreenInteractiveChangedListener); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected void onRegistrationAdded(Object key, Registration registration) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - mLocationUsageLogger.logLocationApiUsage( LocationStatsEnums.USAGE_STARTED, LocationStatsEnums.API_REQUEST_LOCATION_UPDATES, @@ -2038,23 +2005,21 @@ public class LocationProviderManager extends null, registration.isForeground()); } - @GuardedBy("mLock") + // TODO: remove suppression when GuardedBy analysis can recognize lock from super class + @SuppressWarnings("GuardedBy") + @GuardedBy("mMultiplexerLock") @Override - protected void onRegistrationReplaced(Object key, Registration oldRegistration, - Registration newRegistration) { + protected void onRegistrationReplaced(Object oldKey, Registration oldRegistration, + Object newKey, Registration newRegistration) { // by saving the last delivered location state we are able to potentially delay the // resulting provider request longer and save additional power newRegistration.setLastDeliveredLocation(oldRegistration.getLastDeliveredLocation()); - super.onRegistrationReplaced(key, oldRegistration, newRegistration); + super.onRegistrationReplaced(oldKey, oldRegistration, newKey, newRegistration); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected void onRegistrationRemoved(Object key, Registration registration) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - mLocationUsageLogger.logLocationApiUsage( LocationStatsEnums.USAGE_ENDED, LocationStatsEnums.API_REQUEST_LOCATION_UPDATES, @@ -2067,21 +2032,17 @@ public class LocationProviderManager extends null, registration.isForeground()); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected boolean registerWithService(ProviderRequest request, Collection registrations) { return reregisterWithService(ProviderRequest.EMPTY_REQUEST, request, registrations); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected boolean reregisterWithService(ProviderRequest oldRequest, ProviderRequest newRequest, Collection registrations) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - // calculate how long the new request should be delayed before sending it off to the // provider, under the assumption that once we send the request off, the provider will // immediately attempt to deliver a new location satisfying that request. @@ -2117,7 +2078,7 @@ public class LocationProviderManager extends mDelayedRegister = new OnAlarmListener() { @Override public void onAlarm() { - synchronized (mLock) { + synchronized (mMultiplexerLock) { if (mDelayedRegister == this) { mDelayedRegister = null; setProviderRequest(newRequest); @@ -2135,17 +2096,13 @@ public class LocationProviderManager extends return true; } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected void unregisterWithService() { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - setProviderRequest(ProviderRequest.EMPTY_REQUEST); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") void setProviderRequest(ProviderRequest request) { if (mDelayedRegister != null) { mAlarmHelper.cancel(mDelayedRegister); @@ -2166,13 +2123,9 @@ public class LocationProviderManager extends }); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected boolean isActive(Registration registration) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - if (!registration.isPermitted()) { return false; } @@ -2236,13 +2189,9 @@ public class LocationProviderManager extends return true; } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override protected ProviderRequest mergeRegistrations(Collection registrations) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - long intervalMs = ProviderRequest.INTERVAL_DISABLED; int quality = LocationRequest.QUALITY_LOW_POWER; long maxUpdateDelayMs = Long.MAX_VALUE; @@ -2307,7 +2256,7 @@ public class LocationProviderManager extends .build(); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") protected long calculateRequestDelayMillis(long newIntervalMs, Collection registrations) { // calculate the minimum delay across all registrations, ensuring that it is not more than @@ -2349,7 +2298,7 @@ public class LocationProviderManager extends } private void onUserChanged(int userId, int change) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { if (mState == STATE_STOPPED) { return; } @@ -2372,15 +2321,13 @@ public class LocationProviderManager extends private void onLocationUserSettingsChanged(int userId, LocationUserSettings oldSettings, LocationUserSettings newSettings) { if (oldSettings.isAdasGnssLocationEnabled() != newSettings.isAdasGnssLocationEnabled()) { - synchronized (mLock) { - updateRegistrations( - registration -> registration.onAdasGnssLocationEnabledChanged(userId)); - } + updateRegistrations( + registration -> registration.onAdasGnssLocationEnabledChanged(userId)); } } private void onLocationEnabledChanged(int userId) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { if (mState == STATE_STOPPED) { return; } @@ -2390,88 +2337,64 @@ public class LocationProviderManager extends } private void onScreenInteractiveChanged(boolean screenInteractive) { - synchronized (mLock) { - switch (mLocationPowerSaveModeHelper.getLocationPowerSaveMode()) { - case LOCATION_MODE_GPS_DISABLED_WHEN_SCREEN_OFF: - if (!GPS_PROVIDER.equals(mName)) { - break; - } - // fall through - case LOCATION_MODE_THROTTLE_REQUESTS_WHEN_SCREEN_OFF: - // fall through - case LOCATION_MODE_ALL_DISABLED_WHEN_SCREEN_OFF: - updateRegistrations(registration -> true); + switch (mLocationPowerSaveModeHelper.getLocationPowerSaveMode()) { + case LOCATION_MODE_GPS_DISABLED_WHEN_SCREEN_OFF: + if (!GPS_PROVIDER.equals(mName)) { break; - default: - break; - } + } + // fall through + case LOCATION_MODE_THROTTLE_REQUESTS_WHEN_SCREEN_OFF: + // fall through + case LOCATION_MODE_ALL_DISABLED_WHEN_SCREEN_OFF: + updateRegistrations(registration -> true); + break; + default: + break; } } private void onBackgroundThrottlePackageWhitelistChanged() { - synchronized (mLock) { - updateRegistrations(Registration::onProviderLocationRequestChanged); - } + updateRegistrations(Registration::onProviderLocationRequestChanged); } private void onBackgroundThrottleIntervalChanged() { - synchronized (mLock) { - updateRegistrations(Registration::onProviderLocationRequestChanged); - } + updateRegistrations(Registration::onProviderLocationRequestChanged); } private void onLocationPowerSaveModeChanged(@LocationPowerSaveMode int locationPowerSaveMode) { - synchronized (mLock) { - // this is rare, just assume everything has changed to keep it simple - updateRegistrations(registration -> true); - } + // this is rare, just assume everything has changed to keep it simple + updateRegistrations(registration -> true); } private void onAppForegroundChanged(int uid, boolean foreground) { - synchronized (mLock) { - updateRegistrations(registration -> registration.onForegroundChanged(uid, foreground)); - } + updateRegistrations(registration -> registration.onForegroundChanged(uid, foreground)); } private void onAdasAllowlistChanged() { - synchronized (mLock) { - updateRegistrations(Registration::onProviderLocationRequestChanged); - } + updateRegistrations(Registration::onProviderLocationRequestChanged); } private void onIgnoreSettingsWhitelistChanged() { - synchronized (mLock) { - updateRegistrations(Registration::onProviderLocationRequestChanged); - } + updateRegistrations(Registration::onProviderLocationRequestChanged); } private void onLocationPackageBlacklistChanged(int userId) { - synchronized (mLock) { - updateRegistrations(registration -> registration.getIdentity().getUserId() == userId); - } + updateRegistrations(registration -> registration.getIdentity().getUserId() == userId); } private void onLocationPermissionsChanged(@Nullable String packageName) { - synchronized (mLock) { - updateRegistrations( - registration -> registration.onLocationPermissionsChanged(packageName)); - } + updateRegistrations( + registration -> registration.onLocationPermissionsChanged(packageName)); } private void onLocationPermissionsChanged(int uid) { - synchronized (mLock) { - updateRegistrations(registration -> registration.onLocationPermissionsChanged(uid)); - } + updateRegistrations(registration -> registration.onLocationPermissionsChanged(uid)); } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override public void onStateChanged( AbstractLocationProvider.State oldState, AbstractLocationProvider.State newState) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - if (oldState.allowed != newState.allowed) { onEnabledChanged(UserHandle.USER_ALL); } @@ -2487,13 +2410,9 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") @Override public void onReportLocation(LocationResult locationResult) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - LocationResult filtered; if (mPassiveManager != null) { filtered = locationResult.filter(location -> { @@ -2549,12 +2468,8 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private void onUserStarted(int userId) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - if (userId == UserHandle.USER_NULL) { return; } @@ -2572,12 +2487,8 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private void onUserStopped(int userId) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - if (userId == UserHandle.USER_NULL) { return; } @@ -2592,12 +2503,8 @@ public class LocationProviderManager extends } } - @GuardedBy("mLock") + @GuardedBy("mMultiplexerLock") private void onEnabledChanged(int userId) { - if (Build.IS_DEBUGGABLE) { - Preconditions.checkState(Thread.holdsLock(mLock)); - } - if (userId == UserHandle.USER_NULL) { // used during initialization - ignore since many lower level operations (checking // settings for instance) do not support the null user @@ -2697,7 +2604,7 @@ public class LocationProviderManager extends } public void dump(FileDescriptor fd, IndentingPrintWriter ipw, String[] args) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { ipw.print(mName); ipw.print(" provider"); if (mProvider.isMock()) { @@ -2738,10 +2645,10 @@ public class LocationProviderManager extends private static class LastLocation { - private @Nullable Location mFineLocation; - private @Nullable Location mCoarseLocation; - private @Nullable Location mFineBypassLocation; - private @Nullable Location mCoarseBypassLocation; + @Nullable private Location mFineLocation; + @Nullable private Location mCoarseLocation; + @Nullable private Location mFineBypassLocation; + @Nullable private Location mCoarseBypassLocation; LastLocation() {} @@ -2765,7 +2672,7 @@ public class LocationProviderManager extends mCoarseLocation = null; } - public @Nullable Location get(@PermissionLevel int permissionLevel, + @Nullable public Location get(@PermissionLevel int permissionLevel, boolean isBypass) { switch (permissionLevel) { case PERMISSION_FINE: @@ -2862,7 +2769,7 @@ public class LocationProviderManager extends private static class GatedCallback implements Runnable { @GuardedBy("this") - private @Nullable Runnable mCallback; + @Nullable private Runnable mCallback; @GuardedBy("this") private boolean mGate; diff --git a/services/core/java/com/android/server/location/provider/PassiveLocationProviderManager.java b/services/core/java/com/android/server/location/provider/PassiveLocationProviderManager.java index b35af4f6475c1..0cb4f9e0a0ac9 100644 --- a/services/core/java/com/android/server/location/provider/PassiveLocationProviderManager.java +++ b/services/core/java/com/android/server/location/provider/PassiveLocationProviderManager.java @@ -54,7 +54,7 @@ public class PassiveLocationProviderManager extends LocationProviderManager { * Reports a new location to passive location provider clients. */ public void updateLocation(LocationResult locationResult) { - synchronized (mLock) { + synchronized (mMultiplexerLock) { PassiveLocationProvider passive = (PassiveLocationProvider) mProvider.getProvider(); Preconditions.checkState(passive != null); diff --git a/services/tests/mockingservicestests/src/com/android/server/location/listeners/ListenerMultiplexerTest.java b/services/tests/mockingservicestests/src/com/android/server/location/listeners/ListenerMultiplexerTest.java index d7fef604d25b1..5b927061a6553 100644 --- a/services/tests/mockingservicestests/src/com/android/server/location/listeners/ListenerMultiplexerTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/location/listeners/ListenerMultiplexerTest.java @@ -58,8 +58,10 @@ public class ListenerMultiplexerTest { void onRegistrationAdded(Consumer consumer, TestListenerRegistration registration); - void onRegistrationReplaced(Consumer consumer, - TestListenerRegistration oldRegistration, TestListenerRegistration newRegistration); + void onRegistrationReplaced(Consumer oldConsumer, + TestListenerRegistration oldRegistration, + Consumer newConsumer, + TestListenerRegistration newRegistration); void onRegistrationRemoved(Consumer consumer, TestListenerRegistration registration); @@ -93,10 +95,10 @@ public class ListenerMultiplexerTest { assertThat(mMultiplexer.mMergedRequest).isEqualTo(0); mMultiplexer.addListener(1, consumer); - mInOrder.verify(mCallbacks).onRegistrationRemoved(eq(consumer), - any(TestListenerRegistration.class)); mInOrder.verify(mCallbacks).onRegistrationReplaced(eq(consumer), - any(TestListenerRegistration.class), any(TestListenerRegistration.class)); + any(TestListenerRegistration.class), + eq(consumer), + any(TestListenerRegistration.class)); assertThat(mMultiplexer.mRegistered).isTrue(); assertThat(mMultiplexer.mMergedRequest).isEqualTo(1); @@ -115,10 +117,10 @@ public class ListenerMultiplexerTest { any(TestListenerRegistration.class)); mInOrder.verify(mCallbacks).onActive(); mMultiplexer.replaceListener(1, oldConsumer, consumer); - mInOrder.verify(mCallbacks).onRegistrationRemoved(eq(oldConsumer), + mInOrder.verify(mCallbacks).onRegistrationReplaced(eq(oldConsumer), + any(TestListenerRegistration.class), + eq(consumer), any(TestListenerRegistration.class)); - mInOrder.verify(mCallbacks).onRegistrationReplaced(eq(consumer), - any(TestListenerRegistration.class), any(TestListenerRegistration.class)); assertThat(mMultiplexer.mRegistered).isTrue(); assertThat(mMultiplexer.mMergedRequest).isEqualTo(1); @@ -352,13 +354,19 @@ public class ListenerMultiplexerTest { } private static class TestListenerRegistration extends - RequestListenerRegistration> { + ListenerRegistration> { + private final Integer mInteger; boolean mActive = true; protected TestListenerRegistration(Integer integer, Consumer consumer) { - super(DIRECT_EXECUTOR, integer, consumer); + super(DIRECT_EXECUTOR, consumer); + mInteger = integer; + } + + public Integer getInteger() { + return mInteger; } } @@ -375,11 +383,6 @@ public class ListenerMultiplexerTest { mCallbacks = callbacks; } - @Override - public String getTag() { - return "TestMultiplexer"; - } - public void addListener(Integer request, Consumer consumer) { putRegistration(consumer, new TestListenerRegistration(request, consumer)); } @@ -399,9 +402,9 @@ public class ListenerMultiplexerTest { removeRegistration(consumer, registration); } - public void setActive(Integer request, boolean active) { + public void setActive(Integer integer, boolean active) { updateRegistrations(testRegistration -> { - if (testRegistration.getRequest().equals(request)) { + if (testRegistration.getInteger().equals(integer)) { testRegistration.mActive = active; return true; } @@ -458,10 +461,11 @@ public class ListenerMultiplexerTest { } @Override - protected void onRegistrationReplaced(Consumer consumer, + protected void onRegistrationReplaced(Consumer oldKey, TestListenerRegistration oldRegistration, + Consumer newKey, TestListenerRegistration newRegistration) { - mCallbacks.onRegistrationReplaced(consumer, oldRegistration, newRegistration); + mCallbacks.onRegistrationReplaced(oldKey, oldRegistration, newKey, newRegistration); } @Override @@ -475,8 +479,8 @@ public class ListenerMultiplexerTest { Collection testRegistrations) { int max = Integer.MIN_VALUE; for (TestListenerRegistration registration : testRegistrations) { - if (registration.getRequest() > max) { - max = registration.getRequest(); + if (registration.getInteger() > max) { + max = registration.getInteger(); } } mMergeCount++; @@ -493,7 +497,7 @@ public class ListenerMultiplexerTest { @Override protected void onRegistrationAdded(Consumer consumer, TestListenerRegistration registration) { - addListener(registration.getRequest(), consumer); + addListener(registration.getInteger(), consumer); } } } diff --git a/services/tests/mockingservicestests/src/com/android/server/location/provider/LocationProviderManagerTest.java b/services/tests/mockingservicestests/src/com/android/server/location/provider/LocationProviderManagerTest.java index 71cc65b484ee1..0ac14432d1137 100644 --- a/services/tests/mockingservicestests/src/com/android/server/location/provider/LocationProviderManagerTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/location/provider/LocationProviderManagerTest.java @@ -32,7 +32,6 @@ import static com.android.server.location.LocationPermissions.PERMISSION_COARSE; import static com.android.server.location.LocationPermissions.PERMISSION_FINE; import static com.android.server.location.LocationUtils.createLocation; import static com.android.server.location.LocationUtils.createLocationResult; -import static com.android.server.location.listeners.RemoteListenerRegistration.IN_PROCESS_EXECUTOR; import static com.google.common.truth.Truth.assertThat; @@ -534,7 +533,7 @@ public class LocationProviderManagerTest { listener); CountDownLatch blocker = new CountDownLatch(1); - IN_PROCESS_EXECUTOR.execute(() -> { + FgThread.getExecutor().execute(() -> { try { blocker.await(); } catch (InterruptedException e) { @@ -661,7 +660,7 @@ public class LocationProviderManagerTest { listener); CountDownLatch blocker = new CountDownLatch(1); - IN_PROCESS_EXECUTOR.execute(() -> { + FgThread.getExecutor().execute(() -> { try { blocker.await(); } catch (InterruptedException e) {