diff options
10 files changed, 327 insertions, 38 deletions
diff --git a/services/core/java/com/android/server/LocationManagerService.java b/services/core/java/com/android/server/LocationManagerService.java index 3bc93cc13023..e342dd7977fb 100644 --- a/services/core/java/com/android/server/LocationManagerService.java +++ b/services/core/java/com/android/server/LocationManagerService.java @@ -649,7 +649,7 @@ public class LocationManagerService extends ILocationManager.Stub { mEnabled = new SparseArray<>(1); // initialize last since this lets our reference escape - mProvider = new MockableLocationProvider(mContext, mLock, this); + mProvider = new MockableLocationProvider(mLock, this); // we can assume all users start with disabled location state since the initial state // of all providers is disabled. no need to initialize mEnabled further. @@ -2699,7 +2699,7 @@ public class LocationManagerService extends ILocationManager.Stub { mProviderManagers.add(manager); } - manager.setMockProvider(new MockProvider(mContext, properties)); + manager.setMockProvider(new MockProvider(properties)); } } diff --git a/services/core/java/com/android/server/location/AbstractLocationProvider.java b/services/core/java/com/android/server/location/AbstractLocationProvider.java index 64bca78f294d..997f21c9b6a3 100644 --- a/services/core/java/com/android/server/location/AbstractLocationProvider.java +++ b/services/core/java/com/android/server/location/AbstractLocationProvider.java @@ -177,7 +177,6 @@ public abstract class AbstractLocationProvider { } } - protected final Context mContext; protected final Executor mExecutor; // we use a lock-free implementation to update state to ensure atomicity between updating the @@ -186,13 +185,11 @@ public abstract class AbstractLocationProvider { // before it was set, and should not miss any updates that occur after it was set). private final AtomicReference<InternalState> mInternalState; - protected AbstractLocationProvider(Context context, Executor executor) { - this(context, executor, Collections.singleton(context.getPackageName())); + protected AbstractLocationProvider(Executor executor, Context context) { + this(executor, Collections.singleton(context.getPackageName())); } - protected AbstractLocationProvider(Context context, Executor executor, - Set<String> packageNames) { - mContext = context; + protected AbstractLocationProvider(Executor executor, Set<String> packageNames) { mExecutor = executor; mInternalState = new AtomicReference<>( new InternalState(null, State.EMPTY_STATE.withProviderPackageNames(packageNames))); @@ -202,7 +199,7 @@ public abstract class AbstractLocationProvider { * Sets the listener and returns the state at the moment the listener was set. The listener can * expect to receive all state updates from after this point. */ - State setListener(@Nullable Listener listener) { + protected State setListener(@Nullable Listener listener) { return mInternalState.updateAndGet( internalState -> internalState.withListener(listener)).state; } @@ -210,14 +207,14 @@ public abstract class AbstractLocationProvider { /** * Retrieves the state of the provider. */ - State getState() { + public State getState() { return mInternalState.get().state; } /** * Sets the state of the provider to the new state. */ - void setState(State newState) { + protected void setState(State newState) { InternalState oldInternalState = mInternalState.getAndUpdate( internalState -> internalState.withState(newState)); if (newState.equals(oldInternalState.state)) { @@ -357,7 +354,7 @@ public abstract class AbstractLocationProvider { /** * Always invoked on the provider executor. */ - protected void onExtraCommand(int uid, int pid, String command, Bundle extras) {} + protected abstract void onExtraCommand(int uid, int pid, String command, Bundle extras); /** * Requests a provider to enable itself for the given user id. @@ -370,7 +367,7 @@ public abstract class AbstractLocationProvider { /** * Always invoked on the provider executor. */ - protected void onRequestSetAllowed(boolean allowed) {} + protected abstract void onRequestSetAllowed(boolean allowed); /** * Dumps debug or log information. May be invoked from any thread. diff --git a/services/core/java/com/android/server/location/GnssLocationProvider.java b/services/core/java/com/android/server/location/GnssLocationProvider.java index a7fd57afc86b..bcac4730cb8b 100644 --- a/services/core/java/com/android/server/location/GnssLocationProvider.java +++ b/services/core/java/com/android/server/location/GnssLocationProvider.java @@ -408,7 +408,7 @@ public class GnssLocationProvider extends AbstractLocationProvider implements // Available only on GNSS HAL 2.0 implementations and later. private GnssVisibilityControl mGnssVisibilityControl; - // Handler for processing events + private final Context mContext; private Handler mHandler; private final GnssNetworkConnectivityHandler mNetworkConnectivityHandler; @@ -625,10 +625,11 @@ public class GnssLocationProvider extends AbstractLocationProvider implements } public GnssLocationProvider(Context context) { - super(context, FgThread.getExecutor()); + super(FgThread.getExecutor(), context); ensureInitialized(); + mContext = context; mLooper = FgThread.getHandler().getLooper(); // Create a wake lock @@ -1212,6 +1213,11 @@ public class GnssLocationProvider extends AbstractLocationProvider implements } } + @Override + protected void onRequestSetAllowed(boolean allowed) { + // do nothing - the gnss provider is always allowed + } + private void deleteAidingData(Bundle extras) { int flags; diff --git a/services/core/java/com/android/server/location/LocationProviderProxy.java b/services/core/java/com/android/server/location/LocationProviderProxy.java index 1dee7a809c7e..19fb6694bbb5 100644 --- a/services/core/java/com/android/server/location/LocationProviderProxy.java +++ b/services/core/java/com/android/server/location/LocationProviderProxy.java @@ -134,6 +134,7 @@ public class LocationProviderProxy extends AbstractLocationProvider { // also used to synchronized any state changes (setEnabled, setProperties, setState, etc) private final Object mLock = new Object(); + private final Context mContext; private final ServiceWatcher mServiceWatcher; @GuardedBy("mLock") @@ -143,10 +144,11 @@ public class LocationProviderProxy extends AbstractLocationProvider { private LocationProviderProxy(Context context, String action, int enableOverlayResId, int nonOverlayPackageResId) { - // safe to use direct executor since none of our callbacks call back into any code above - // this provider - they simply forward to the proxy service - super(context, DIRECT_EXECUTOR); + // safe to use direct executor even though this class has internal locks - all of our + // callbacks go to oneway binder transactions which cannot possibly be re-entrant + super(DIRECT_EXECUTOR, Collections.emptySet()); + mContext = context; mServiceWatcher = new ServiceWatcher(context, FgThread.getHandler(), action, this::onBind, this::onUnbind, enableOverlayResId, nonOverlayPackageResId); diff --git a/services/core/java/com/android/server/location/MockProvider.java b/services/core/java/com/android/server/location/MockProvider.java index bcec8b12b371..b45b66017062 100644 --- a/services/core/java/com/android/server/location/MockProvider.java +++ b/services/core/java/com/android/server/location/MockProvider.java @@ -16,15 +16,18 @@ package com.android.server.location; +import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR; + import android.annotation.Nullable; -import android.content.Context; import android.location.Location; +import android.os.Bundle; import com.android.internal.location.ProviderProperties; import com.android.internal.location.ProviderRequest; import java.io.FileDescriptor; import java.io.PrintWriter; +import java.util.Collections; /** * A mock location provider used by LocationManagerService to implement test providers. @@ -35,10 +38,9 @@ public class MockProvider extends AbstractLocationProvider { @Nullable private Location mLocation; - public MockProvider(Context context, ProviderProperties properties) { - // using a direct executor is only acceptable because this class is so simple it is trivial - // to verify that it does not acquire any locks or re-enter LMS from callbacks - super(context, Runnable::run); + public MockProvider(ProviderProperties properties) { + // using a direct executor is ok because this class has no locks that could deadlock + super(DIRECT_EXECUTOR, Collections.emptySet()); setProperties(properties); } @@ -59,6 +61,9 @@ public class MockProvider extends AbstractLocationProvider { public void onSetRequest(ProviderRequest request) {} @Override + protected void onExtraCommand(int uid, int pid, String command, Bundle extras) {} + + @Override protected void onRequestSetAllowed(boolean allowed) { setAllowed(allowed); } diff --git a/services/core/java/com/android/server/location/MockableLocationProvider.java b/services/core/java/com/android/server/location/MockableLocationProvider.java index 5b4f008a581b..b0e133061fc6 100644 --- a/services/core/java/com/android/server/location/MockableLocationProvider.java +++ b/services/core/java/com/android/server/location/MockableLocationProvider.java @@ -16,8 +16,9 @@ package com.android.server.location; +import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR; + import android.annotation.Nullable; -import android.content.Context; import android.location.Location; import android.os.Bundle; @@ -68,10 +69,10 @@ public class MockableLocationProvider extends AbstractLocationProvider { * The client should expect that it may being to receive callbacks as soon as this constructor * is invoked. */ - public MockableLocationProvider(Context context, Object ownerLock, Listener listener) { + public MockableLocationProvider(Object ownerLock, Listener listener) { // using a direct executor is acceptable because all inbound calls are delegated to the // actual provider implementations which will use their own executors - super(context, Runnable::run, Collections.emptySet()); + super(DIRECT_EXECUTOR, Collections.emptySet()); mOwnerLock = ownerLock; mRequest = ProviderRequest.EMPTY_REQUEST; @@ -190,11 +191,6 @@ public class MockableLocationProvider extends AbstractLocationProvider { } } - @Override - public State getState() { - return super.getState(); - } - /** * Returns the current location request. */ @@ -204,6 +200,7 @@ public class MockableLocationProvider extends AbstractLocationProvider { } } + @Override protected void onSetRequest(ProviderRequest request) { synchronized (mOwnerLock) { if (request == mRequest) { @@ -218,6 +215,7 @@ public class MockableLocationProvider extends AbstractLocationProvider { } } + @Override protected void onExtraCommand(int uid, int pid, String command, Bundle extras) { synchronized (mOwnerLock) { if (mProvider != null) { @@ -226,6 +224,15 @@ public class MockableLocationProvider extends AbstractLocationProvider { } } + @Override + protected void onRequestSetAllowed(boolean allowed) { + synchronized (mOwnerLock) { + if (mProvider != null) { + mProvider.onRequestSetAllowed(allowed); + } + } + } + /** * Dumps the current provider implementation. */ diff --git a/services/core/java/com/android/server/location/PassiveProvider.java b/services/core/java/com/android/server/location/PassiveProvider.java index ef157a39fa28..54dffff8b1df 100644 --- a/services/core/java/com/android/server/location/PassiveProvider.java +++ b/services/core/java/com/android/server/location/PassiveProvider.java @@ -16,9 +16,12 @@ package com.android.server.location; +import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR; + import android.content.Context; import android.location.Criteria; import android.location.Location; +import android.os.Bundle; import com.android.internal.location.ProviderProperties; import com.android.internal.location.ProviderRequest; @@ -49,9 +52,8 @@ public class PassiveProvider extends AbstractLocationProvider { private volatile boolean mReportLocation; public PassiveProvider(Context context) { - // using a direct executor is only acceptable because this class is so simple it is trivial - // to verify that it does not acquire any locks or re-enter LMS from callbacks - super(context, Runnable::run); + // using a direct executor is ok because this class has no locks that could deadlock + super(DIRECT_EXECUTOR, context); mReportLocation = false; @@ -59,15 +61,26 @@ public class PassiveProvider extends AbstractLocationProvider { setAllowed(true); } + /** + * Pass a location into the passive provider. + */ + public void updateLocation(Location location) { + if (mReportLocation) { + reportLocation(location); + } + } + @Override public void onSetRequest(ProviderRequest request) { mReportLocation = request.reportLocation; } - public void updateLocation(Location location) { - if (mReportLocation) { - reportLocation(location); - } + @Override + protected void onExtraCommand(int uid, int pid, String command, Bundle extras) {} + + @Override + protected void onRequestSetAllowed(boolean allowed) { + // do nothing - the passive provider is always allowed } @Override diff --git a/services/tests/servicestests/Android.bp b/services/tests/servicestests/Android.bp index 556f6362d872..f99081024494 100644 --- a/services/tests/servicestests/Android.bp +++ b/services/tests/servicestests/Android.bp @@ -30,6 +30,7 @@ android_test { "services.usage", "guava", "androidx.test.core", + "androidx.test.ext.truth", "androidx.test.runner", "androidx.test.rules", "mockito-target-minus-junit4", diff --git a/services/tests/servicestests/src/com/android/server/location/MockableLocationProviderTest.java b/services/tests/servicestests/src/com/android/server/location/MockableLocationProviderTest.java new file mode 100644 index 000000000000..6fafe113d90d --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/location/MockableLocationProviderTest.java @@ -0,0 +1,213 @@ +/* + * 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; + +import static androidx.test.ext.truth.location.LocationSubject.assertThat; + +import static com.android.internal.location.ProviderRequest.EMPTY_REQUEST; + +import static com.google.common.truth.Truth.assertThat; + +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import android.location.Criteria; +import android.location.Location; +import android.platform.test.annotations.Presubmit; + +import androidx.test.filters.SmallTest; +import androidx.test.runner.AndroidJUnit4; + +import com.android.internal.location.ProviderProperties; +import com.android.internal.location.ProviderRequest; +import com.android.server.location.test.FakeProvider; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.LinkedList; +import java.util.List; + +@Presubmit +@SmallTest +@RunWith(AndroidJUnit4.class) +public class MockableLocationProviderTest { + + private Object mLock; + private ListenerCapture mListener; + + private AbstractLocationProvider mRealProvider; + private MockProvider mMockProvider; + + private MockableLocationProvider mProvider; + + @Before + public void setUp() { + mLock = new Object(); + mListener = new ListenerCapture(); + + mRealProvider = spy(new FakeProvider()); + mMockProvider = spy(new MockProvider(new ProviderProperties( + false, + false, + false, + false, + true, + true, + true, + Criteria.POWER_LOW, + Criteria.ACCURACY_FINE))); + + mProvider = new MockableLocationProvider(mLock, mListener); + mProvider.setRealProvider(mRealProvider); + } + + @Test + public void testSetProvider() { + assertThat(mProvider.getProvider()).isEqualTo(mRealProvider); + + mProvider.setMockProvider(mMockProvider); + assertThat(mProvider.getProvider()).isEqualTo(mMockProvider); + + mProvider.setMockProvider(null); + assertThat(mProvider.getProvider()).isEqualTo(mRealProvider); + + mProvider.setRealProvider(null); + assertThat(mProvider.getProvider()).isNull(); + } + + @Test + public void testSetRequest() { + assertThat(mProvider.getCurrentRequest()).isEqualTo(EMPTY_REQUEST); + verify(mRealProvider, times(1)).onSetRequest(EMPTY_REQUEST); + + ProviderRequest request = new ProviderRequest.Builder().setInterval(1).build(); + mProvider.setRequest(request); + + assertThat(mProvider.getCurrentRequest()).isEqualTo(request); + verify(mRealProvider, times(1)).onSetRequest(request); + + mProvider.setMockProvider(mMockProvider); + assertThat(mProvider.getCurrentRequest()).isEqualTo(request); + verify(mRealProvider, times(2)).onSetRequest(EMPTY_REQUEST); + verify(mMockProvider, times(1)).onSetRequest(request); + + mProvider.setMockProvider(null); + assertThat(mProvider.getCurrentRequest()).isEqualTo(request); + verify(mMockProvider, times(1)).onSetRequest(EMPTY_REQUEST); + verify(mRealProvider, times(2)).onSetRequest(request); + + mProvider.setRealProvider(null); + assertThat(mProvider.getCurrentRequest()).isEqualTo(request); + verify(mRealProvider, times(3)).onSetRequest(EMPTY_REQUEST); + } + + @Test + public void testRequestSetAllowed() { + mProvider.requestSetAllowed(true); + verify(mRealProvider, times(1)).onRequestSetAllowed(true); + + mProvider.setMockProvider(mMockProvider); + mProvider.requestSetAllowed(true); + verify(mMockProvider, times(1)).onRequestSetAllowed(true); + } + + @Test + public void testSendExtraCommand() { + mProvider.sendExtraCommand(0, 0, "command", null); + verify(mRealProvider, times(1)).onExtraCommand(0, 0, "command", null); + + mProvider.setMockProvider(mMockProvider); + mProvider.sendExtraCommand(0, 0, "command", null); + verify(mMockProvider, times(1)).onExtraCommand(0, 0, "command", null); + } + + @Test + public void testSetState() { + assertThat(mProvider.getState().allowed).isFalse(); + + AbstractLocationProvider.State newState; + + mRealProvider.setAllowed(true); + newState = mListener.getNextNewState(); + assertThat(newState).isNotNull(); + assertThat(newState.allowed).isTrue(); + + mProvider.setMockProvider(mMockProvider); + newState = mListener.getNextNewState(); + assertThat(newState).isNotNull(); + assertThat(newState.allowed).isFalse(); + + mMockProvider.setAllowed(true); + newState = mListener.getNextNewState(); + assertThat(newState).isNotNull(); + assertThat(newState.allowed).isTrue(); + + mRealProvider.setAllowed(false); + assertThat(mListener.getNextNewState()).isNull(); + + mProvider.setMockProvider(null); + newState = mListener.getNextNewState(); + assertThat(newState).isNotNull(); + assertThat(newState.allowed).isFalse(); + } + + @Test + public void testReportLocation() { + Location realLocation = new Location("real"); + Location mockLocation = new Location("mock"); + + mRealProvider.reportLocation(realLocation); + assertThat(mListener.getNextLocation()).isEqualTo(realLocation); + + mProvider.setMockProvider(mMockProvider); + mRealProvider.reportLocation(realLocation); + mMockProvider.reportLocation(mockLocation); + assertThat(mListener.getNextLocation()).isEqualTo(mockLocation); + } + + private class ListenerCapture implements AbstractLocationProvider.Listener { + + private final LinkedList<AbstractLocationProvider.State> mNewStates = new LinkedList<>(); + private final LinkedList<Location> mLocations = new LinkedList<>(); + + @Override + public void onStateChanged(AbstractLocationProvider.State oldState, + AbstractLocationProvider.State newState) { + assertThat(Thread.holdsLock(mLock)).isTrue(); + mNewStates.add(newState); + } + + private AbstractLocationProvider.State getNextNewState() { + return mNewStates.poll(); + } + + @Override + public void onReportLocation(Location location) { + assertThat(Thread.holdsLock(mLock)).isTrue(); + mLocations.add(location); + } + + private Location getNextLocation() { + return mLocations.poll(); + } + + @Override + public void onReportLocation(List<Location> locations) {} + } +} diff --git a/services/tests/servicestests/src/com/android/server/location/test/FakeProvider.java b/services/tests/servicestests/src/com/android/server/location/test/FakeProvider.java new file mode 100644 index 000000000000..762080fe5745 --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/location/test/FakeProvider.java @@ -0,0 +1,45 @@ +/* + * 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.test; + +import android.os.Bundle; + +import com.android.internal.location.ProviderRequest; +import com.android.server.location.AbstractLocationProvider; + +import java.io.FileDescriptor; +import java.io.PrintWriter; +import java.util.Collections; + +public class FakeProvider extends AbstractLocationProvider { + + public FakeProvider() { + super(Runnable::run, Collections.emptySet()); + } + + @Override + protected void onSetRequest(ProviderRequest request) {} + + @Override + protected void onExtraCommand(int uid, int pid, String command, Bundle extras) {} + + @Override + protected void onRequestSetAllowed(boolean allowed) {} + + @Override + public void dump(FileDescriptor fd, PrintWriter pw, String[] args) {} +} |