From 05f32ddfabe1a41b0ecfd5220d774db165cdf341 Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Tue, 22 Sep 2020 09:14:49 -0700 Subject: [PATCH] DO NOT MERGE 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 extra power draw since the system server would not have canceled the associated provider location request. This would have left getCurrentLocation() as equivalent to requestSingleUpdate() where it was supposed to be an all-around improvement that could save power. Bug: 168666216 Test: manual + presubmits Change-Id: I5c77db4cc56cce1b984587f65d2bcfb488436bb8 --- .../android/location/ILocationManager.aidl | 6 +-- .../android/location/LocationManager.java | 42 ++++++------------- .../location/LocationManagerService.java | 21 +++++----- 3 files changed, 26 insertions(+), 43 deletions(-) diff --git a/location/java/android/location/ILocationManager.aidl b/location/java/android/location/ILocationManager.aidl index 75ea0cbada92b..7ed5b579ebb4f 100644 --- a/location/java/android/location/ILocationManager.aidl +++ b/location/java/android/location/ILocationManager.aidl @@ -45,9 +45,9 @@ import com.android.internal.location.ProviderProperties; interface ILocationManager { Location getLastLocation(in LocationRequest request, String packageName, String featureId); - boolean getCurrentLocation(in LocationRequest request, - in ICancellationSignal cancellationSignal, in ILocationListener listener, - String packageName, String featureId, String listenerId); + @nullable ICancellationSignal getCurrentLocation(in LocationRequest request, + in ILocationListener listener, String packageName, String featureId, + String listenerId); void requestLocationUpdates(in LocationRequest request, in ILocationListener listener, in PendingIntent intent, String packageName, String featureId, String listenerId); diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java index 05a9863cb2667..6bf6034bbbf4e 100644 --- a/location/java/android/location/LocationManager.java +++ b/location/java/android/location/LocationManager.java @@ -726,16 +726,15 @@ public class LocationManager { cancellationSignal.throwIfCanceled(); } - ICancellationSignal remoteCancellationSignal = CancellationSignal.createTransport(); - try { - if (mService.getCurrentLocation(currentLocationRequest, remoteCancellationSignal, - transport, mContext.getPackageName(), mContext.getAttributionTag(), - transport.getListenerId())) { + ICancellationSignal cancelRemote = mService.getCurrentLocation( + currentLocationRequest, transport, mContext.getPackageName(), + mContext.getAttributionTag(), transport.getListenerId()); + if (cancelRemote != null) { transport.register(mContext.getSystemService(AlarmManager.class), - remoteCancellationSignal); + cancellationSignal); if (cancellationSignal != null) { - cancellationSignal.setOnCancelListener(transport::cancel); + cancellationSignal.setRemote(cancelRemote); } } else { transport.fail(); @@ -2540,7 +2539,7 @@ public class LocationManager { } private static class GetCurrentLocationTransport extends ILocationListener.Stub implements - AlarmManager.OnAlarmListener { + AlarmManager.OnAlarmListener, CancellationSignal.OnCancelListener { @GuardedBy("this") @Nullable @@ -2572,7 +2571,7 @@ public class LocationManager { } public synchronized void register(AlarmManager alarmManager, - ICancellationSignal remoteCancellationSignal) { + CancellationSignal cancellationSignal) { if (mConsumer == null) { return; } @@ -2585,16 +2584,18 @@ public class LocationManager { this, null); - mRemoteCancellationSignal = remoteCancellationSignal; + if (cancellationSignal != null) { + cancellationSignal.setOnCancelListener(this); + } } - public void cancel() { + @Override + public void onCancel() { remove(); } private Consumer remove() { Consumer consumer; - ICancellationSignal cancellationSignal; synchronized (this) { mExecutor = null; consumer = mConsumer; @@ -2604,18 +2605,6 @@ public class LocationManager { mAlarmManager.cancel(this); mAlarmManager = null; } - - // ensure only one cancel event will go through - cancellationSignal = mRemoteCancellationSignal; - mRemoteCancellationSignal = null; - } - - if (cancellationSignal != null) { - try { - cancellationSignal.cancel(); - } catch (RemoteException e) { - // ignore - } } return consumer; @@ -2637,11 +2626,6 @@ public class LocationManager { @Override public void onLocationChanged(Location location) { - synchronized (this) { - // save ourselves a pointless x-process call to cancel the location request - mRemoteCancellationSignal = null; - } - deliverResult(location); } diff --git a/services/core/java/com/android/server/location/LocationManagerService.java b/services/core/java/com/android/server/location/LocationManagerService.java index 9adde24a99e0c..5c9350a8452cb 100644 --- a/services/core/java/com/android/server/location/LocationManagerService.java +++ b/services/core/java/com/android/server/location/LocationManagerService.java @@ -2064,10 +2064,12 @@ public class LocationManagerService extends ILocationManager.Stub { } } + @Nullable @Override - public boolean getCurrentLocation(LocationRequest locationRequest, - ICancellationSignal remoteCancellationSignal, ILocationListener listener, - String packageName, String featureId, String listenerId) { + public ICancellationSignal getCurrentLocation(LocationRequest locationRequest, + ILocationListener listener, String packageName, String featureId, String listenerId) { + ICancellationSignal remoteCancellationSignal = CancellationSignal.createTransport(); + // side effect of validating locationRequest and packageName Location lastLocation = getLastLocation(locationRequest, packageName, featureId); if (lastLocation != null) { @@ -2077,17 +2079,17 @@ public class LocationManagerService extends ILocationManager.Stub { if (locationAgeMs < MAX_CURRENT_LOCATION_AGE_MS) { try { listener.onLocationChanged(lastLocation); - return true; + return remoteCancellationSignal; } catch (RemoteException e) { Log.w(TAG, e); - return false; + return null; } } if (!mAppForegroundHelper.isAppForeground(Binder.getCallingUid())) { if (locationAgeMs < mSettingsHelper.getBackgroundThrottleIntervalMs()) { // not allowed to request new locations, so we can't return anything - return false; + return null; } } } @@ -2095,11 +2097,8 @@ public class LocationManagerService extends ILocationManager.Stub { requestLocationUpdates(locationRequest, listener, null, packageName, featureId, listenerId); CancellationSignal cancellationSignal = CancellationSignal.fromTransport( remoteCancellationSignal); - if (cancellationSignal != null) { - cancellationSignal.setOnCancelListener( - () -> removeUpdates(listener, null)); - } - return true; + cancellationSignal.setOnCancelListener(() -> removeUpdates(listener, null)); + return remoteCancellationSignal; } @Override