From 05f32ddfabe1a41b0ecfd5220d774db165cdf341 Mon Sep 17 00:00:00 2001
From: Soonil Nagarkar <sooniln@google.com>
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<Location> remove() {
             Consumer<Location> 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