From 94e14bcc2f78d07ca7e0d0dfa46a7fe6fe9226c0 Mon Sep 17 00:00:00 2001
From: Alvin Ji <alvinji@chromium.org>
Date: Thu, 21 May 2026 08:54:47 -0700
Subject: [PATCH] geolocation: Manage activity count via GeolocationProxy

Introduce GeolocationProxy to manage the lifetime of the Geolocation
Mojo connection between the renderer and GeolocationImpl.

Previously, GeolocationServiceImpl managed the activity count (affecting
UI indicators) based on the broker pipe lifetime. However, the renderer
could disconnect the broker pipe while keeping the data pipe open,
potentially leading to silent location access.

To fix this, GeolocationProxy is introduced as an intermediary that
forwards calls and monitors connection status. GeolocationServiceImpl
now tracks active proxies and manages the activity count based on their
lifetime, ensuring the location indicator remains active as long as the
data pipe is open.

Bug: 514489361
Change-Id: If37960ecc038f2cf9a57a49355d7d809be347155
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7862524
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1634321}
---

diff --git a/content/browser/geolocation/geolocation_service_impl.cc b/content/browser/geolocation/geolocation_service_impl.cc
index 312c511..d7bca56 100644
--- a/content/browser/geolocation/geolocation_service_impl.cc
+++ b/content/browser/geolocation/geolocation_service_impl.cc
@@ -12,6 +12,7 @@
 #include "components/content_settings/core/common/features.h"
 #include "content/browser/permissions/permission_controller_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
+#include "content/common/features.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/permission_controller.h"
 #include "content/public/browser/permission_descriptor_util.h"
@@ -29,6 +30,50 @@
 
 namespace content {
 
+// `GeolocationProxy` acts as a Mojo intermediary between the renderer client
+// and the backing `GeolocationImpl`. It forwards all calls and monitors
+// the connection lifetime of both ends.
+//
+// `GeolocationServiceImpl` uses this to track active sessions and manage
+// the browser-side activity count (UI location indicator). Disconnections on
+// either end trigger `OnProxyDisconnected` to update state.
+class GeolocationServiceImpl::GeolocationProxy
+    : public device::mojom::Geolocation {
+ public:
+  GeolocationProxy(
+      GeolocationServiceImpl* service_impl,
+      mojo::PendingRemote<device::mojom::Geolocation> geolocation_impl_remote,
+      mojo::PendingReceiver<device::mojom::Geolocation> renderer_receiver)
+      : service_impl_(service_impl),
+        geolocation_impl_remote_(std::move(geolocation_impl_remote)),
+        renderer_receiver_(this, std::move(renderer_receiver)) {
+    renderer_receiver_.set_disconnect_handler(base::BindOnce(
+        &GeolocationProxy::OnDisconnect, base::Unretained(this)));
+    geolocation_impl_remote_.set_disconnect_handler(base::BindOnce(
+        &GeolocationProxy::OnDisconnect, base::Unretained(this)));
+  }
+
+  // device::mojom::Geolocation:
+  void SetHighAccuracyHint(bool high_accuracy) override {
+    geolocation_impl_remote_->SetHighAccuracyHint(high_accuracy);
+  }
+  void QueryCachedPosition(QueryCachedPositionCallback callback) override {
+    geolocation_impl_remote_->QueryCachedPosition(std::move(callback));
+  }
+  void QueryNextPosition(QueryNextPositionCallback callback) override {
+    geolocation_impl_remote_->QueryNextPosition(std::move(callback));
+  }
+
+ private:
+  void OnDisconnect() { service_impl_->OnProxyDisconnected(this); }
+
+  // `GeolocationServiceImpl` owns `this` (via `active_proxies_`), so
+  // `service_impl_` is guaranteed to outlive `this`.
+  const raw_ptr<GeolocationServiceImpl> service_impl_;
+  mojo::Remote<device::mojom::Geolocation> geolocation_impl_remote_;
+  mojo::Receiver<device::mojom::Geolocation> renderer_receiver_;
+};
+
 namespace {
 
 using GeolocationPermissionLevel = device::mojom::GeolocationPermissionLevel;
@@ -174,9 +219,6 @@
     return;
   }
 
-  std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
-  IncrementActivityCount();
-
   requesting_origin_ =
       render_frame_host_->GetMainFrame()->GetLastCommittedOrigin();
   auto requesting_url =
@@ -184,10 +226,33 @@
 
   bool has_precise_permission =
       permission_level == GeolocationPermissionLevel::kPrecise;
-  geolocation_context->BindGeolocation(
-      std::move(receiver), requesting_origin_,
-      device::mojom::GeolocationClientId::kGeolocationServiceImpl,
-      has_precise_permission);
+
+  if (base::FeatureList::IsEnabled(features::kGeolocationProxy)) {
+    mojo::PendingRemote<device::mojom::Geolocation> geolocation_impl_remote;
+    geolocation_context->BindGeolocation(
+        geolocation_impl_remote.InitWithNewPipeAndPassReceiver(),
+        requesting_origin_,
+        device::mojom::GeolocationClientId::kGeolocationServiceImpl,
+        has_precise_permission);
+
+    if (active_proxies_.empty()) {
+      IncrementActivityCount();
+    }
+    active_proxies_.push_back(std::make_unique<GeolocationProxy>(
+        this, std::move(geolocation_impl_remote), std::move(receiver)));
+
+    std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
+  } else {
+    std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
+    // NOTE: Legacy behavior may leak the active frame count if multiple
+    // connections are created. Kept as-is for compatibility.
+    IncrementActivityCount();
+
+    geolocation_context->BindGeolocation(
+        std::move(receiver), requesting_origin_,
+        device::mojom::GeolocationClientId::kGeolocationServiceImpl,
+        has_precise_permission);
+  }
   subscription_id_ =
       PermissionControllerImpl::FromBrowserContext(
           render_frame_host_->GetBrowserContext())
@@ -218,14 +283,30 @@
     PermissionControllerImpl::FromBrowserContext(
         render_frame_host_->GetBrowserContext())
         ->UnsubscribeFromPermissionResultChange(subscription_id_);
-    DecrementActivityCount();
+    // When kGeolocationProxy is enabled, DecrementActivityCount is managed
+    // by the GeolocationProxy lifecycle.
+    if (!base::FeatureList::IsEnabled(features::kGeolocationProxy)) {
+      DecrementActivityCount();
+    }
   }
   geolocation_context->OnPermissionUpdated(requesting_origin_,
                                            permission_level);
 }
 
 void GeolocationServiceImpl::OnDisconnected() {
-  if (receiver_set_.empty()) {
+  // When kGeolocationProxy is enabled, we do not need to perform any cleanup
+  // here because active connection lifetimes are managed by the proxies.
+  if (!base::FeatureList::IsEnabled(features::kGeolocationProxy) &&
+      receiver_set_.empty()) {
+    DecrementActivityCount();
+  }
+}
+
+void GeolocationServiceImpl::OnProxyDisconnected(GeolocationProxy* proxy) {
+  std::erase_if(active_proxies_, [proxy](const auto& active_proxy) {
+    return active_proxy.get() == proxy;
+  });
+  if (active_proxies_.empty()) {
     DecrementActivityCount();
   }
 }
diff --git a/content/browser/geolocation/geolocation_service_impl.h b/content/browser/geolocation/geolocation_service_impl.h
index 227bbd9..a452fb2 100644
--- a/content/browser/geolocation/geolocation_service_impl.h
+++ b/content/browser/geolocation/geolocation_service_impl.h
@@ -77,6 +77,9 @@
   void OnDisconnected();
 
  private:
+  // Private helper to manage connection lifetimes.
+  class GeolocationProxy;
+
   // Creates the Geolocation Service.
   void CreateGeolocationWithPermissionResult(
       mojo::PendingReceiver<device::mojom::Geolocation> receiver,
@@ -88,6 +91,8 @@
 
   device::mojom::GeolocationContext* GetGeolocationContext();
 
+  void OnProxyDisconnected(GeolocationProxy* proxy);
+
   // Used to subscribe to permission status changes.
   PermissionController::SubscriptionId subscription_id_;
 
@@ -111,6 +116,10 @@
   // stopped because the permission status changed.
   bool is_sending_updates_ = false;
 
+  // Active proxies managing connections between the renderer and the backing

diff --git a/content/browser/geolocation/geolocation_service_impl_unittest.cc b/content/browser/geolocation/geolocation_service_impl_unittest.cc
index 45c19b52..7a7b44d 100644
--- a/content/browser/geolocation/geolocation_service_impl_unittest.cc
+++ b/content/browser/geolocation/geolocation_service_impl_unittest.cc
@@ -17,6 +17,8 @@
 #include "components/content_settings/core/common/content_settings_utils.h"
 #include "components/content_settings/core/common/features.h"
 #include "content/browser/permissions/permission_controller_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/common/features.h"
 #include "content/public/browser/device_service.h"
 #include "content/public/browser/permission_controller.h"
 #include "content/public/browser/permission_request_description.h"
@@ -461,5 +463,65 @@
   EXPECT_EQ(PermissionStatus::GRANTED, create_geolocation_future.Get());
 }
 
+TEST_P(GeolocationServiceTest, BrokerDisconnectDoesNotStopActivityCount) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kGeolocationProxy);
+
+  CreateEmbeddedFrameAndGeolocationService(
+      /*allow_via_permissions_policy=*/true);
+
+  permission_manager()->SetRequestCallback(
+      base::BindRepeating([](PermissionCallback permission_callback) {
+        base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+            FROM_HERE,
+            base::BindOnce(
+                std::move(permission_callback),
+                std::vector{content::PermissionResult(
+                    PermissionStatus::GRANTED,
+                    PermissionStatusSource::UNSPECIFIED,
+                    base::FeatureList::IsEnabled(
+                        content_settings::features::
+                            kApproximateGeolocationPermission)
+                        ? std::make_optional(GeolocationSetting{
+                              .approximate = PermissionOption::kAllowed,
+                              .precise = PermissionOption::kAllowed})
+                        : std::nullopt)}));
+      }));
+
+  mojo::Remote<Geolocation> geolocation;
+  TestFuture<blink::mojom::PermissionStatus> create_geolocation_future;
+  service_remote()->CreateGeolocation(
+      geolocation.BindNewPipeAndPassReceiver(), true,
+      blink::mojom::GeolocationAccuracy::kPrecise,
+      create_geolocation_future.GetCallback());
+
+  EXPECT_EQ(blink::mojom::PermissionStatus::GRANTED,
+            create_geolocation_future.Get());
+
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(web_contents());
+  EXPECT_TRUE(web_contents_impl->IsCapabilityActive(
+      WebContentsCapabilityType::kGeolocation));
+
+  // Disconnect the broker pipe.
+  service_remote().reset();
+  base::RunLoop().RunUntilIdle();
+
+  // Security/Privacy Check: Ensure the geolocation capability remains active
+  // as long as the data pipe is open, even if the broker pipe is disconnected.
+  // This guarantees the location indicator in the UI remains visible while
+  // the renderer can still receive updates.
+  EXPECT_TRUE(web_contents_impl->IsCapabilityActive(
+      WebContentsCapabilityType::kGeolocation));
+
+  // Close the data pipe.
+  geolocation.reset();
+  base::RunLoop().RunUntilIdle();
+
+  // Active state should now be false.
+  EXPECT_FALSE(web_contents_impl->IsCapabilityActive(
+      WebContentsCapabilityType::kGeolocation));
+}
+
 }  // namespace
 }  // namespace content