Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 43a368dc authored by Soonil Nagarkar's avatar Soonil Nagarkar
Browse files

Fix bug in getCurrentLocation cancellation 

Cancellation was not being propagated to the system server due to a
misunderstanding on how CancellationSignal functions. There is no
noticable impact for the user, as cancellation did properly prevent any
further locations, however this would have resulted in limited extra
power draw since the system server would not have canceled the
associated provider location request. Impact is limited since
getCurrentLocation() was limited to a 30s duration anyways.

Bug: 168666216
Test: manual + presubmits
Change-Id: I5c77db4cc56cce1b984587f65d2bcfb488436bb8
parent 191ab1cb

location/java/android/location/ILocationManager.aidl

+1 −1
Original line number Diff line number Diff line
@@ -47,7 +47,7 @@ import com.android.internal.location.ProviderProperties; 
interface ILocationManager

{

    Location getLastLocation(String provider, String packageName, String attributionTag);

    void getCurrentLocation(String provider, in LocationRequest request, in ICancellationSignal cancellationSignal, in ILocationCallback callback, String packageName, String attributionTag, String listenerId);

    @nullable ICancellationSignal getCurrentLocation(String provider, in LocationRequest request, in ILocationCallback callback, String packageName, String attributionTag, String listenerId);



    void registerLocationListener(String provider, in LocationRequest request, in ILocationListener listener, String packageName, String attributionTag, String listenerId);

    void unregisterLocationListener(in ILocationListener listener);

location/java/android/location/LocationManager.java

+19 −28
Original line number Diff line number Diff line
@@ -783,22 +783,25 @@ public class LocationManager { 
        Preconditions.checkArgument(provider != null, "invalid null provider");

        Preconditions.checkArgument(locationRequest != null, "invalid null location request");



        ICancellationSignal remoteCancellationSignal = CancellationSignal.createTransport();

        GetCurrentLocationTransport transport = new GetCurrentLocationTransport(executor, consumer,

                remoteCancellationSignal);



        if (cancellationSignal != null) {

            cancellationSignal.throwIfCanceled();

            cancellationSignal.setOnCancelListener(transport::cancel);

        }



        GetCurrentLocationTransport transport = new GetCurrentLocationTransport(executor, consumer,

                cancellationSignal);



        ICancellationSignal cancelRemote;

        try {

            mService.getCurrentLocation(provider, locationRequest, remoteCancellationSignal,

                    transport, mContext.getPackageName(), mContext.getAttributionTag(),

                    AppOpsManager.toReceiverId(consumer));

            cancelRemote = mService.getCurrentLocation(provider,

                    locationRequest, transport, mContext.getPackageName(),

                    mContext.getAttributionTag(), AppOpsManager.toReceiverId(consumer));

        } catch (RemoteException e) {

            throw e.rethrowFromSystemServer();

        }



        if (cancellationSignal != null) {

            cancellationSignal.setRemote(cancelRemote);

        }

    }



    /**
@@ -2519,7 +2522,7 @@ public class LocationManager { 
    }



    private static class GetCurrentLocationTransport extends ILocationCallback.Stub implements

            ListenerExecutor {

            ListenerExecutor, CancellationSignal.OnCancelListener {



        private final Executor mExecutor;
@@ -2527,33 +2530,22 @@ public class LocationManager { 
        @Nullable

        private Consumer<Location> mConsumer;



        @GuardedBy("this")

        @Nullable

        private ICancellationSignal mRemoteCancellationSignal;



        GetCurrentLocationTransport(Executor executor, Consumer<Location> consumer,

                ICancellationSignal remoteCancellationSignal) {

                @Nullable CancellationSignal cancellationSignal) {

            Preconditions.checkArgument(executor != null, "illegal null executor");

            Preconditions.checkArgument(consumer != null, "illegal null consumer");

            mExecutor = executor;

            mConsumer = consumer;

            mRemoteCancellationSignal = remoteCancellationSignal;



            if (cancellationSignal != null) {

                cancellationSignal.setOnCancelListener(this);

            }

        }



        public void cancel() {

            ICancellationSignal cancellationSignal;

        @Override

        public void onCancel() {

            synchronized (this) {

                cancellationSignal = mRemoteCancellationSignal;

                mConsumer = null;

                mRemoteCancellationSignal = null;

            }



            if (cancellationSignal != null) {

                try {

                    cancellationSignal.cancel();

                } catch (RemoteException e) {

                    throw e.rethrowFromSystemServer();

                }

            }

        }
@@ -2563,7 +2555,6 @@ public class LocationManager { 
            synchronized (this) {

                consumer = mConsumer;

                mConsumer = null;

                mRemoteCancellationSignal = null;

            }



            executeSafely(mExecutor, () -> consumer, listener -> listener.accept(location));

services/core/java/com/android/server/location/LocationManagerService.java

+5 −5
Original line number Diff line number Diff line
@@ -701,10 +701,11 @@ public class LocationManagerService extends ILocationManager.Stub { 
        return location;

    }



    @Nullable

    @Override

    public void getCurrentLocation(String provider, LocationRequest request,

            ICancellationSignal cancellationTransport, ILocationCallback consumer,

            String packageName, String attributionTag, String listenerId) {

    public ICancellationSignal getCurrentLocation(String provider, LocationRequest request,

            ILocationCallback consumer, String packageName, String attributionTag,

            String listenerId) {

        CallerIdentity identity = CallerIdentity.fromBinder(mContext, packageName, attributionTag,

                listenerId);

        int permissionLevel = LocationPermissions.getPermissionLevel(mContext, identity.getUid(),
@@ -721,8 +722,7 @@ public class LocationManagerService extends ILocationManager.Stub { 
        Preconditions.checkArgument(manager != null,

                "provider \"" + provider + "\" does not exist");



        manager.getCurrentLocation(request, identity, permissionLevel, cancellationTransport,

                consumer);

        return manager.getCurrentLocation(request, identity, permissionLevel, consumer);

    }



    @Override

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

+17 −18
Original line number Diff line number Diff line
@@ -1424,9 +1424,9 @@ class LocationProviderManager extends 
        }

    }



    public void getCurrentLocation(LocationRequest request, CallerIdentity callerIdentity,

            int permissionLevel, ICancellationSignal cancellationTransport,

            ILocationCallback callback) {

    @Nullable

    public ICancellationSignal getCurrentLocation(LocationRequest request,

            CallerIdentity callerIdentity, int permissionLevel, ILocationCallback callback) {

        if (request.getDurationMillis() > GET_CURRENT_LOCATION_MAX_TIMEOUT_MS) {

            request = new LocationRequest.Builder(request)

                    .setDurationMillis(GET_CURRENT_LOCATION_MAX_TIMEOUT_MS)
@@ -1444,15 +1444,15 @@ class LocationProviderManager extends 
            if (mSettingsHelper.isLocationPackageBlacklisted(callerIdentity.getUserId(),

                    callerIdentity.getPackageName())) {

                registration.deliverLocation(null);

                return;

                return null;

            }

            if (!mUserInfoHelper.isCurrentUserId(callerIdentity.getUserId())) {

                registration.deliverLocation(null);

                return;

                return null;

            }

            if (!request.isLocationSettingsIgnored() && !isEnabled(callerIdentity.getUserId())) {

                registration.deliverLocation(null);

                return;

                return null;

            }



            Location lastLocation = getLastLocationUnsafe(callerIdentity.getUserId(),
@@ -1463,13 +1463,13 @@ class LocationProviderManager extends 
                                - lastLocation.getElapsedRealtimeNanos());

                if (locationAgeMs < MAX_CURRENT_LOCATION_AGE_MS) {

                    registration.deliverLocation(lastLocation);

                    return;

                    return null;

                }



                if (!mAppForegroundHelper.isAppForeground(Binder.getCallingUid())

                        && locationAgeMs < mSettingsHelper.getBackgroundThrottleIntervalMs()) {

                    registration.deliverLocation(null);

                    return;

                    return null;

                }

            }
@@ -1482,16 +1482,15 @@ class LocationProviderManager extends 
            }

        }



        CancellationSignal cancellationSignal = CancellationSignal.fromTransport(

                cancellationTransport);

        if (cancellationSignal != null) {

        ICancellationSignal cancelTransport = CancellationSignal.createTransport();

        CancellationSignal cancellationSignal = CancellationSignal.fromTransport(cancelTransport);

        cancellationSignal.setOnCancelListener(SingleUseCallback.wrap(

                () -> {

                    synchronized (mLock) {

                        removeRegistration(callback.asBinder(), registration);

                    }

                }));

        }

        return cancelTransport;

    }



    public void sendExtraCommand(int uid, int pid, String command, Bundle extras) {

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

+7 −19
Original line number Diff line number Diff line
@@ -66,7 +66,6 @@ import android.location.LocationManagerInternal.ProviderEnabledListener; 
import android.location.LocationRequest;

import android.location.util.identity.CallerIdentity;

import android.os.Bundle;

import android.os.CancellationSignal;

import android.os.Handler;

import android.os.ICancellationSignal;

import android.os.IRemoteCallback;
@@ -616,9 +615,7 @@ public class LocationProviderManagerTest { 


        ILocationCallback listener = createMockGetCurrentLocationListener();

        LocationRequest locationRequest = new LocationRequest.Builder(0).build();

        ICancellationSignal cancellationSignal = CancellationSignal.createTransport();

        mManager.getCurrentLocation(locationRequest, IDENTITY,

                PERMISSION_FINE, cancellationSignal, listener);

        mManager.getCurrentLocation(locationRequest, IDENTITY, PERMISSION_FINE, listener);



        Location loc = createLocation(NAME, mRandom);

        mProvider.setProviderLocation(loc);
@@ -632,9 +629,8 @@ public class LocationProviderManagerTest { 
    public void testGetCurrentLocation_Cancel() throws Exception {

        ILocationCallback listener = createMockGetCurrentLocationListener();

        LocationRequest locationRequest = new LocationRequest.Builder(0).build();

        ICancellationSignal cancellationSignal = CancellationSignal.createTransport();

        mManager.getCurrentLocation(locationRequest, IDENTITY,

                PERMISSION_FINE, cancellationSignal, listener);

        ICancellationSignal cancellationSignal = mManager.getCurrentLocation(locationRequest,

                IDENTITY, PERMISSION_FINE, listener);



        cancellationSignal.cancel();

        mProvider.setProviderLocation(createLocation(NAME, mRandom));
@@ -646,9 +642,7 @@ public class LocationProviderManagerTest { 
    public void testGetCurrentLocation_ProviderDisabled() throws Exception {

        ILocationCallback listener = createMockGetCurrentLocationListener();

        LocationRequest locationRequest = new LocationRequest.Builder(0).build();

        ICancellationSignal cancellationSignal = CancellationSignal.createTransport();

        mManager.getCurrentLocation(locationRequest, IDENTITY,

                PERMISSION_FINE, cancellationSignal, listener);

        mManager.getCurrentLocation(locationRequest, IDENTITY, PERMISSION_FINE, listener);



        mProvider.setProviderAllowed(false);

        mProvider.setProviderAllowed(true);
@@ -662,9 +656,7 @@ public class LocationProviderManagerTest { 


        ILocationCallback listener = createMockGetCurrentLocationListener();

        LocationRequest locationRequest = new LocationRequest.Builder(0).build();

        ICancellationSignal cancellationSignal = CancellationSignal.createTransport();

        mManager.getCurrentLocation(locationRequest, IDENTITY,

                PERMISSION_FINE, cancellationSignal, listener);

        mManager.getCurrentLocation(locationRequest, IDENTITY, PERMISSION_FINE, listener);



        mProvider.setProviderAllowed(true);

        mProvider.setProviderLocation(createLocation(NAME, mRandom));
@@ -680,9 +672,7 @@ public class LocationProviderManagerTest { 


        ILocationCallback listener = createMockGetCurrentLocationListener();

        LocationRequest locationRequest = new LocationRequest.Builder(0).build();

        ICancellationSignal cancellationSignal = CancellationSignal.createTransport();

        mManager.getCurrentLocation(locationRequest, IDENTITY,

                PERMISSION_FINE, cancellationSignal, listener);

        mManager.getCurrentLocation(locationRequest, IDENTITY, PERMISSION_FINE, listener);



        verify(listener, times(1)).onLocation(locationCaptor.capture());

        assertThat(locationCaptor.getValue()).isEqualTo(loc);
@@ -692,9 +682,7 @@ public class LocationProviderManagerTest { 
    public void testGetCurrentLocation_Timeout() throws Exception {

        ILocationCallback listener = createMockGetCurrentLocationListener();

        LocationRequest locationRequest = new LocationRequest.Builder(0).build();

        ICancellationSignal cancellationSignal = CancellationSignal.createTransport();

        mManager.getCurrentLocation(locationRequest, IDENTITY,

                PERMISSION_FINE, cancellationSignal, listener);

        mManager.getCurrentLocation(locationRequest, IDENTITY, PERMISSION_FINE, listener);



        ArgumentCaptor<OnAlarmListener> listenerCapture = ArgumentCaptor.forClass(

                OnAlarmListener.class);