From 5da87b27f6225394630d153b99136adbf24ceb67 Mon Sep 17 00:00:00 2001
From: Alvin Ji <alvinji@chromium.org>
Date: Wed, 29 Apr 2026 08:57:43 -0700
Subject: [PATCH] Fix location accuracy labeling race condition on Android

Capture the requested accuracy intent at the time of the location
request in `LocationProviderGmsCore` and `LocationProviderAndroid`. This
prevents a race condition where an in-flight callback could misreport
its accuracy if a new request with different requirements was initiated
in the meantime.

Change-Id: Ia9aebb6b9dd4d791ea96dcf8e29770acdecdb266
Bug: 501683745
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7794376
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1622489}
---

diff --git a/services/BUILD.gn b/services/BUILD.gn
index c61236ee..07dd02ab 100644
--- a/services/BUILD.gn
+++ b/services/BUILD.gn
@@ -199,6 +199,7 @@
       "//services/device/serial:java",
       "//services/shape_detection:shape_detection_java",
       "//skia/public/mojom:mojom_java",
+      "//third_party/androidx:androidx_annotation_annotation_java",
       "//third_party/google-truth:google_truth_java",
     ]
 
diff --git a/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java b/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java
index 6726ce7..added8cb 100644
--- a/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java
+++ b/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java
@@ -29,20 +29,20 @@
 import java.util.List;
 
 /**
- * This is a LocationProvider using Android APIs [1]. It is a separate class for clarity
- * so that it can manage all processing completely on the UI thread. The container class
- * ensures that the start/stop calls into this class are done on the UI thread.
+ * This is a LocationProvider using Android APIs [1]. It is a separate class for clarity so that it
+ * can manage all processing completely on the UI thread. The container class ensures that the
+ * start/stop calls into this class are done on the UI thread.
  *
- * [1] https://developer.android.com/reference/android/location/package-summary.html
+ * <p>[1] https://developer.android.com/reference/android/location/package-summary.html
  */
 @NullMarked
-public class LocationProviderAndroid implements LocationListener, LocationProvider {
+public class LocationProviderAndroid implements LocationProvider {
     private static final String TAG = "LocationProvider";
 
     private @Nullable LocationManager mLocationManager;
+    private @Nullable LocationListener mLocationListener;
     private boolean mIsRunning;
     private boolean mEffectiveHighAccuracy;
-    private boolean mRequestedHighAccuracy;
 
     private final Context mContext;
 
@@ -57,8 +57,8 @@
     @Override
     public void start(boolean enableHighAccuracy) {
         ThreadUtils.assertOnUiThread();
-        mRequestedHighAccuracy = enableHighAccuracy;
-        mEffectiveHighAccuracy = mRequestedHighAccuracy;
+        final boolean requestedHighAccuracy = enableHighAccuracy;
+        mEffectiveHighAccuracy = requestedHighAccuracy;
 
         // Checking app-level permission here and override the `mEffectiveHighAccuracy`
         // so we can make sure `Geolocation.AndroidLocationProvider.` is logged with
@@ -69,7 +69,7 @@
         }
 
         unregisterFromLocationUpdates();
-        registerForLocationUpdates();
+        registerForLocationUpdates(requestedHighAccuracy);
     }
 
     @Override
@@ -84,37 +84,6 @@
         return mIsRunning;
     }
 
-    @Override
-    public void onLocationChanged(Location location) {
-        // Callbacks from the system location service are queued to this thread, so it's
-        // possible that we receive callbacks after unregistering. At this point, the
-        // native object will no longer exist. Using `mRequestedHighAccuracy` for
-        // location update because `mEffectiveHighAccuracy` can be overridden by app-level
-        // permission check.
-        if (mIsRunning) {
-            if (location.hasAccuracy()) {
-                final String histogramName =
-                        "Geolocation.AndroidLocationProvider"
-                                + (mEffectiveHighAccuracy
-                                        ? ".HighAccuracyHint"
-                                        : ".LowAccuracyHint")
-                                + ".Accuracy";
-                RecordHistogram.recordCount100000Histogram(
-                        histogramName, (int) location.getAccuracy());
-            }
-            LocationProviderAdapter.onNewLocationAvailable(location, mRequestedHighAccuracy);
-        }
-    }
-
-    @Override
-    public void onStatusChanged(String provider, int status, Bundle extras) {}
-
-    @Override
-    public void onProviderEnabled(String provider) {}
-
-    @Override
-    public void onProviderDisabled(String provider) {}
-
     public void setLocationManagerForTesting(LocationManager manager) {
         var oldValue = mLocationManager;
         mLocationManager = manager;
@@ -130,7 +99,7 @@
     }
 
     /** Registers this object with the location service. */
-    private void registerForLocationUpdates() {
+    private void registerForLocationUpdates(boolean requestedHighAccuracy) {
         createLocationManagerIfNeeded();
         if (mLocationManager == null) {
             Log.e(TAG, "Could not get location manager.");
@@ -141,6 +110,41 @@
         assert !mIsRunning;
         mIsRunning = true;
 
+        mLocationListener =
+                new LocationListener() {
+                    @Override
+                    public void onLocationChanged(Location location) {
+                        // Callbacks from the system location service are queued to this thread, so
+                        // it's possible that we receive callbacks after unregistering. At this
+                        // point, the native object will no longer exist. Using
+                        // `requestedHighAccuracy` for location update because
+                        // `mEffectiveHighAccuracy` can be overridden by app-level permission check.
+                        if (mIsRunning && mLocationListener == this) {
+                            if (location.hasAccuracy()) {
+                                final String histogramName =
+                                        "Geolocation.AndroidLocationProvider"
+                                                + (mEffectiveHighAccuracy
+                                                        ? ".HighAccuracyHint"
+                                                        : ".LowAccuracyHint")
+                                                + ".Accuracy";
+                                RecordHistogram.recordCount100000Histogram(
+                                        histogramName, (int) location.getAccuracy());
+                            }
+                            LocationProviderAdapter.onNewLocationAvailable(
+                                    location, requestedHighAccuracy);
+                        }
+                    }
+
+                    @Override
+                    public void onStatusChanged(String provider, int status, Bundle extras) {}
+
+                    @Override
+                    public void onProviderEnabled(String provider) {}
+
+                    @Override
+                    public void onProviderDisabled(String provider) {}
+                };
+
         // We're running on the main thread. The C++ side is responsible to
         // bounce notifications to the Geolocation thread as they arrive in the mainLooper.
         try {
@@ -162,7 +166,7 @@
                 }
             }
             mLocationManager.requestLocationUpdates(
-                    0, 0, criteria, this, ThreadUtils.getUiThreadLooper());
+                    0, 0, criteria, mLocationListener, ThreadUtils.getUiThreadLooper());
         } catch (SecurityException e) {
             Log.e(
                     TAG,
@@ -186,7 +190,10 @@
         if (!mIsRunning) return;
         mIsRunning = false;
         assumeNonNull(mLocationManager);
-        mLocationManager.removeUpdates(this);
+        if (mLocationListener != null) {
+            mLocationManager.removeUpdates(mLocationListener);
+            mLocationListener = null;
+        }
     }
 
     @RequiresNonNull("mLocationManager")
diff --git a/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java b/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
index bc684aa7..cc3e9cdf 100644
--- a/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
+++ b/services/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
@@ -43,7 +43,6 @@
     private final Context mContext;
     private final FusedLocationProviderClient mClient;