Not to use handler thread in GnssGeofenceProvider

- Synchronize the calls to native methods with a lock.
- When native calls come back to GnssLocationProvider, make sure to post
tasks on the background thread so the lock is released.

Bug: 116788068
Change-Id: I613c9bb7190ce19100b2bc154e3cda92bf44e2a7
Fixes: 116788068
Test: atest GnssGeofenceProviderTest

3 files changed, 72 insertions, 76 deletions

diff --git a/services/core/java/com/android/server/location/GnssGeofenceProvider.java b/services/core/java/com/android/server/location/GnssGeofenceProvider.java
index 6ac4aeb7f9ea..a84b0b1c4335 100644
--- a/services/core/java/com/android/server/location/GnssGeofenceProvider.java
+++ b/services/core/java/com/android/server/location/GnssGeofenceProvider.java
@@ -1,18 +1,12 @@
 package com.android.server.location;
 
 import android.location.IGpsGeofenceHardware;
-import android.os.Handler;
-import android.os.IBinder;
-import android.os.Looper;
 import android.util.Log;
 import android.util.SparseArray;
 
+import com.android.internal.annotations.GuardedBy;
 import com.android.internal.annotations.VisibleForTesting;
 
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.FutureTask;
-
 /**
  * Manages GNSS Geofence operations.
  */
@@ -34,26 +28,26 @@ class GnssGeofenceProvider extends IGpsGeofenceHardware.Stub {
         public boolean paused;
     }
 
+    private final Object mLock = new Object();
+    @GuardedBy("mLock")
     private final GnssGeofenceProviderNative mNative;
+    @GuardedBy("mLock")
     private final SparseArray<GeofenceEntry> mGeofenceEntries = new SparseArray<>();
-    private final Handler mHandler;
 
-    GnssGeofenceProvider(Looper looper) {
-        this(looper, new GnssGeofenceProviderNative());
+    GnssGeofenceProvider() {
+        this(new GnssGeofenceProviderNative());
     }
 
     @VisibleForTesting
-    GnssGeofenceProvider(Looper looper, GnssGeofenceProviderNative gnssGeofenceProviderNative) {
-        mHandler = new Handler(looper);
+    GnssGeofenceProvider(GnssGeofenceProviderNative gnssGeofenceProviderNative) {
         mNative = gnssGeofenceProviderNative;
     }
 
-    // TODO(b/37460011): use this method in HAL death recovery.
     void resumeIfStarted() {
         if (DEBUG) {
             Log.d(TAG, "resumeIfStarted");
         }
-        mHandler.post(() -> {
+        synchronized (mLock) {
             for (int i = 0; i < mGeofenceEntries.size(); i++) {
                 GeofenceEntry entry = mGeofenceEntries.valueAt(i);
                 boolean added = mNative.addGeofence(entry.geofenceId, entry.latitude,
@@ -65,30 +59,21 @@ class GnssGeofenceProvider extends IGpsGeofenceHardware.Stub {
                     mNative.pauseGeofence(entry.geofenceId);
                 }
             }
-        });
-    }
-
-    private boolean runOnHandlerThread(Callable<Boolean> callable) {
-        FutureTask<Boolean> futureTask = new FutureTask<>(callable);
-        mHandler.post(futureTask);
-        try {
-            return futureTask.get();
-        } catch (InterruptedException | ExecutionException e) {
-            Log.e(TAG, "Failed running callable.", e);
         }
-        return false;
     }
 
     @Override
     public boolean isHardwareGeofenceSupported() {
-        return runOnHandlerThread(mNative::isGeofenceSupported);
+        synchronized (mLock) {
+            return mNative.isGeofenceSupported();
+        }
     }
 
     @Override
     public boolean addCircularHardwareGeofence(int geofenceId, double latitude,
             double longitude, double radius, int lastTransition, int monitorTransitions,
             int notificationResponsiveness, int unknownTimer) {
-        return runOnHandlerThread(() -> {
+        synchronized (mLock) {
             boolean added = mNative.addGeofence(geofenceId, latitude, longitude, radius,
                     lastTransition, monitorTransitions, notificationResponsiveness,
                     unknownTimer);
@@ -105,23 +90,23 @@ class GnssGeofenceProvider extends IGpsGeofenceHardware.Stub {
                 mGeofenceEntries.put(geofenceId, entry);
             }
             return added;
-        });
+        }
     }
 
     @Override
     public boolean removeHardwareGeofence(int geofenceId) {
-        return runOnHandlerThread(() -> {
+        synchronized (mLock) {
             boolean removed = mNative.removeGeofence(geofenceId);
             if (removed) {
                 mGeofenceEntries.remove(geofenceId);
             }
             return removed;
-        });
+        }
     }
 
     @Override
     public boolean pauseHardwareGeofence(int geofenceId) {
-        return runOnHandlerThread(() -> {
+        synchronized (mLock) {
             boolean paused = mNative.pauseGeofence(geofenceId);
             if (paused) {
                 GeofenceEntry entry = mGeofenceEntries.get(geofenceId);
@@ -130,12 +115,12 @@ class GnssGeofenceProvider extends IGpsGeofenceHardware.Stub {
                 }
             }
             return paused;
-        });
+        }
     }
 
     @Override
     public boolean resumeHardwareGeofence(int geofenceId, int monitorTransitions) {
-        return runOnHandlerThread(() -> {
+        synchronized (mLock) {
             boolean resumed = mNative.resumeGeofence(geofenceId, monitorTransitions);
             if (resumed) {
                 GeofenceEntry entry = mGeofenceEntries.get(geofenceId);
@@ -145,7 +130,7 @@ class GnssGeofenceProvider extends IGpsGeofenceHardware.Stub {
                 }
             }
             return resumed;
-        });
+        }
     }
 
     @VisibleForTesting
diff --git a/services/core/java/com/android/server/location/GnssLocationProvider.java b/services/core/java/com/android/server/location/GnssLocationProvider.java
index d5e4681a0d90..330d1d5de1aa 100644
--- a/services/core/java/com/android/server/location/GnssLocationProvider.java
+++ b/services/core/java/com/android/server/location/GnssLocationProvider.java
@@ -762,7 +762,7 @@ public class GnssLocationProvider extends LocationProviderInterface
                 looper, this);
         mHandler.post(mGnssSatelliteBlacklistHelper::updateSatelliteBlacklist);
         mGnssBatchingProvider = new GnssBatchingProvider();
-        mGnssGeofenceProvider = new GnssGeofenceProvider(looper);
+        mGnssGeofenceProvider = new GnssGeofenceProvider();
     }
 
     /**
@@ -1824,7 +1824,7 @@ public class GnssLocationProvider extends LocationProviderInterface
     /**
      * Converts the GPS HAL status to the internal Geofence Hardware status.
      */
-    private int getGeofenceStatus(int status) {
+    private static int getGeofenceStatus(int status) {
         switch (status) {
             case GPS_GEOFENCE_OPERATION_SUCCESS:
                 return GeofenceHardware.GEOFENCE_SUCCESS;
@@ -1849,75 +1849,87 @@ public class GnssLocationProvider extends LocationProviderInterface
      */
     private void reportGeofenceTransition(int geofenceId, Location location, int transition,
             long transitionTimestamp) {
-        if (mGeofenceHardwareImpl == null) {
-            mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
-        }
+        mHandler.post(() -> {
+            if (mGeofenceHardwareImpl == null) {
+                mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
+            }
 
-        mGeofenceHardwareImpl.reportGeofenceTransition(
-                geofenceId,
-                location,
-                transition,
-                transitionTimestamp,
-                GeofenceHardware.MONITORING_TYPE_GPS_HARDWARE,
-                FusedBatchOptions.SourceTechnologies.GNSS);
+            mGeofenceHardwareImpl.reportGeofenceTransition(
+                    geofenceId,
+                    location,
+                    transition,
+                    transitionTimestamp,
+                    GeofenceHardware.MONITORING_TYPE_GPS_HARDWARE,
+                    FusedBatchOptions.SourceTechnologies.GNSS);
+        });
     }
 
     /**
      * called from native code to report GPS status change.
      */
     private void reportGeofenceStatus(int status, Location location) {
-        if (mGeofenceHardwareImpl == null) {
-            mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
-        }
-        int monitorStatus = GeofenceHardware.MONITOR_CURRENTLY_UNAVAILABLE;
-        if (status == GPS_GEOFENCE_AVAILABLE) {
-            monitorStatus = GeofenceHardware.MONITOR_CURRENTLY_AVAILABLE;
-        }
-        mGeofenceHardwareImpl.reportGeofenceMonitorStatus(
-                GeofenceHardware.MONITORING_TYPE_GPS_HARDWARE,
-                monitorStatus,
-                location,
-                FusedBatchOptions.SourceTechnologies.GNSS);
+        mHandler.post(() -> {
+            if (mGeofenceHardwareImpl == null) {
+                mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
+            }
+            int monitorStatus = GeofenceHardware.MONITOR_CURRENTLY_UNAVAILABLE;
+            if (status == GPS_GEOFENCE_AVAILABLE) {
+                monitorStatus = GeofenceHardware.MONITOR_CURRENTLY_AVAILABLE;
+            }
+            mGeofenceHardwareImpl.reportGeofenceMonitorStatus(
+                    GeofenceHardware.MONITORING_TYPE_GPS_HARDWARE,
+                    monitorStatus,
+                    location,
+                    FusedBatchOptions.SourceTechnologies.GNSS);
+        });
     }
 
     /**
      * called from native code - Geofence Add callback
      */
     private void reportGeofenceAddStatus(int geofenceId, int status) {
-        if (mGeofenceHardwareImpl == null) {
-            mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
-        }
-        mGeofenceHardwareImpl.reportGeofenceAddStatus(geofenceId, getGeofenceStatus(status));
+        mHandler.post(() -> {
+            if (mGeofenceHardwareImpl == null) {
+                mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
+            }
+            mGeofenceHardwareImpl.reportGeofenceAddStatus(geofenceId, getGeofenceStatus(status));
+        });
     }
 
     /**
      * called from native code - Geofence Remove callback
      */
     private void reportGeofenceRemoveStatus(int geofenceId, int status) {
-        if (mGeofenceHardwareImpl == null) {
-            mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
-        }
-        mGeofenceHardwareImpl.reportGeofenceRemoveStatus(geofenceId, getGeofenceStatus(status));
+        mHandler.post(() -> {
+            if (mGeofenceHardwareImpl == null) {
+                mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
+            }
+            mGeofenceHardwareImpl.reportGeofenceRemoveStatus(geofenceId, getGeofenceStatus(status));
+        });
     }
 
     /**
      * called from native code - Geofence Pause callback
      */
     private void reportGeofencePauseStatus(int geofenceId, int status) {
-        if (mGeofenceHardwareImpl == null) {
-            mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
-        }
-        mGeofenceHardwareImpl.reportGeofencePauseStatus(geofenceId, getGeofenceStatus(status));
+        mHandler.post(() -> {
+            if (mGeofenceHardwareImpl == null) {
+                mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
+            }
+            mGeofenceHardwareImpl.reportGeofencePauseStatus(geofenceId, getGeofenceStatus(status));
+        });
     }
 
     /**
      * called from native code - Geofence Resume callback
      */
     private void reportGeofenceResumeStatus(int geofenceId, int status) {
-        if (mGeofenceHardwareImpl == null) {
-            mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
-        }
-        mGeofenceHardwareImpl.reportGeofenceResumeStatus(geofenceId, getGeofenceStatus(status));
+        mHandler.post(() -> {
+            if (mGeofenceHardwareImpl == null) {
+                mGeofenceHardwareImpl = GeofenceHardwareImpl.getInstance(mContext);
+            }
+            mGeofenceHardwareImpl.reportGeofenceResumeStatus(geofenceId, getGeofenceStatus(status));
+        });
     }
 
     //=============================================================
diff --git a/services/robotests/src/com/android/server/location/GnssGeofenceProviderTest.java b/services/robotests/src/com/android/server/location/GnssGeofenceProviderTest.java
index beb59414546a..30c73368da15 100644
--- a/services/robotests/src/com/android/server/location/GnssGeofenceProviderTest.java
+++ b/services/robotests/src/com/android/server/location/GnssGeofenceProviderTest.java
@@ -7,7 +7,6 @@ import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
-import android.os.Looper;
 import android.os.RemoteException;
 import android.platform.test.annotations.Presubmit;
 
@@ -44,7 +43,7 @@ public class GnssGeofenceProviderTest {
         when(mMockNative.pauseGeofence(anyInt())).thenReturn(true);
         when(mMockNative.removeGeofence(anyInt())).thenReturn(true);
         when(mMockNative.resumeGeofence(anyInt(), anyInt())).thenReturn(true);
-        mTestProvider = new GnssGeofenceProvider(Looper.myLooper(), mMockNative);
+        mTestProvider = new GnssGeofenceProvider(mMockNative);
         mTestProvider.addCircularHardwareGeofence(GEOFENCE_ID, LATITUDE,
                 LONGITUDE, RADIUS, LAST_TRANSITION, MONITOR_TRANSITIONS,
                 NOTIFICATION_RESPONSIVENESS,