From 6af009ab20d9b6445785c465a1e7ea88c1f9a259 Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Wed, 19 Aug 2020 13:07:15 -0700 Subject: DO NOT MERGE: Fix NPE bug in LocationManager Both null and non-null GnssRequest objects may be present in the list of requests to merge. Bug: 165375330 Test: presubmits Change-Id: I13284b1c32f1ad1fce39ad1ca2e4298194197015 --- location/java/android/location/LocationManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'location') diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java index 2a2aaea035ff..05a9863cb266 100644 --- a/location/java/android/location/LocationManager.java +++ b/location/java/android/location/LocationManager.java @@ -3039,7 +3039,7 @@ public class LocationManager { protected GnssRequest merge(@NonNull List requests) { Preconditions.checkArgument(!requests.isEmpty()); for (GnssRequest request : requests) { - if (request.isFullTracking()) { + if (request != null && request.isFullTracking()) { return request; } } -- cgit v1.2.3 From 05f32ddfabe1a41b0ecfd5220d774db165cdf341 Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Tue, 22 Sep 2020 09:14:49 -0700 Subject: 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 --- .../java/android/location/ILocationManager.aidl | 6 ++-- .../java/android/location/LocationManager.java | 42 +++++++--------------- 2 files changed, 16 insertions(+), 32 deletions(-) (limited to 'location') diff --git a/location/java/android/location/ILocationManager.aidl b/location/java/android/location/ILocationManager.aidl index 75ea0cbada92..7ed5b579ebb4 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 05a9863cb266..6bf6034bbbf4 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); } -- cgit v1.2.3 From 2d6bcbecd3db5a2983f1a17b4c75641f6b2fee1b Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Thu, 24 Sep 2020 09:35:49 -0700 Subject: DO NOT MERGE Local timeout should cancel remote work Ensure that when getCurrentLocation() times out locally, remote work in system server is also canceled. Bug: 168666216 Test: manual Change-Id: Idde2156323c3fca0ed94ff886a4277122c598753 --- .../java/android/location/LocationManager.java | 23 ++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) (limited to 'location') diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java index 6bf6034bbbf4..b77a249d0fe9 100644 --- a/location/java/android/location/LocationManager.java +++ b/location/java/android/location/LocationManager.java @@ -732,7 +732,7 @@ public class LocationManager { mContext.getAttributionTag(), transport.getListenerId()); if (cancelRemote != null) { transport.register(mContext.getSystemService(AlarmManager.class), - cancellationSignal); + cancellationSignal, cancelRemote); if (cancellationSignal != null) { cancellationSignal.setRemote(cancelRemote); } @@ -2571,7 +2571,8 @@ public class LocationManager { } public synchronized void register(AlarmManager alarmManager, - CancellationSignal cancellationSignal) { + CancellationSignal cancellationSignal, + ICancellationSignal remoteCancellationSignal) { if (mConsumer == null) { return; } @@ -2587,15 +2588,21 @@ public class LocationManager { if (cancellationSignal != null) { cancellationSignal.setOnCancelListener(this); } + + mRemoteCancellationSignal = remoteCancellationSignal; } @Override public void onCancel() { + synchronized (this) { + mRemoteCancellationSignal = null; + } remove(); } private Consumer remove() { Consumer consumer; + ICancellationSignal cancellationSignal; synchronized (this) { mExecutor = null; consumer = mConsumer; @@ -2605,6 +2612,18 @@ 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; -- cgit v1.2.3