From 43a368dc19f87669d3e319136d166b80d4f83d50 Mon Sep 17 00:00:00 2001
From: Soonil Nagarkar <sooniln@google.com>
Date: Mon, 21 Sep 2020 13:36:24 -0700
Subject: 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
---
 .../java/android/location/ILocationManager.aidl    |  2 +-
 .../java/android/location/LocationManager.java     | 47 +++++++++-------------
 2 files changed, 20 insertions(+), 29 deletions(-)

(limited to 'location/java')

diff --git a/location/java/android/location/ILocationManager.aidl b/location/java/android/location/ILocationManager.aidl
index c1a98eabb2b7..31dcd7e65663 100644
--- a/location/java/android/location/ILocationManager.aidl
+++ b/location/java/android/location/ILocationManager.aidl
@@ -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);
diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java
index d7ef24170c24..0f6dddba842b 100644
--- a/location/java/android/location/LocationManager.java
+++ b/location/java/android/location/LocationManager.java
@@ -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));
-- 
cgit v1.2.3-59-g8ed1b