BackForwardCache: Unblocklist pages requesting Geolocation permission.

This CL allows pages which have requested Geolocation
permission from entering BFCache, and verifies that the inflight
geolocation requests from renderer are canceled when the page enters
bfcache.

This CL doesn't implement the request cancel logic.
* The Blink code [1] already has this implemented. *

This CL is just adding browser_test to verify the cancel logic.

We are replacing the "real" implementations of the device service
counterpart to a "fake" one so we can:
- decouple the browser_test from relying on a real GPS device
- "keep the request inflight": renderer requestPosition, but the "fake"
  service can be Pause()-d to not respond to the request, so we can test
  that renderer would forcibly cancel the request when put into BFCache
  on that case.
- count the active number of mojo-channels renderer<->device_svc,
  to confirm that the request has been cancelled.

[1] src/third_party/blink/renderer/modules/geolocation/geolocation.cc

Bug: 989847
Change-Id: I9b5b5af2ee50418ff92738560039967fcb20b8a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730153
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org>
Reviewed-by: Lowell Manners <lowell@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694645}

content/browser/back_forward_cache_browsertest.cc

@@ -29,6 +29,7 @@
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/controllable_http_response.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "services/device/public/cpp/test/scoped_geolocation_overrider.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/public/common/scheduler/web_scheduler_tracked_feature.h"
 
@@ -1858,4 +1859,134 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   web_contents()->GetController().GoToIndex(0);
   crash_observer.Wait();
 }
+
+class GeolocationBackForwardCacheBrowserTest
+    : public BackForwardCacheBrowserTest {
+ protected:
+  GeolocationBackForwardCacheBrowserTest() : geo_override_(0.0, 0.0) {}
+
+  device::ScopedGeolocationOverrider geo_override_;
+};
+
+// Test that a page which has queried geolocation in the past, but have no
+// active geolocation query, can be bfcached.
+IN_PROC_BROWSER_TEST_F(GeolocationBackForwardCacheBrowserTest,
+                       CacheAfterGeolocationRequest) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL url_a(embedded_test_server()->GetURL("/title1.html"));
+  const GURL url_b(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+  // 1) Navigate to A.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a));
+  RenderFrameHostImpl* rfh_a = current_frame_host();
+
+  // Query current position, and wait for the query to complete.
+  EXPECT_EQ("received", EvalJs(rfh_a, R"(
+      new Promise(resolve => {
+        navigator.geolocation.getCurrentPosition(() => resolve('received'));
+      });
+  )"));
+
+  RenderFrameDeletedObserver deleted(rfh_a);
+
+  // 2) Navigate away.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b));
+
+  // The page has no inflight geolocation request when we navigated away,
+  // so it should have been cached.
+  EXPECT_FALSE(deleted.deleted());
+  EXPECT_TRUE(rfh_a->is_in_back_forward_cache());
+}
+
+// Test that a page which has an inflight geolocation query can be bfcached,
+// and verify that the page does not observe any geolocation while the page
+// was inside bfcache.
+IN_PROC_BROWSER_TEST_F(GeolocationBackForwardCacheBrowserTest,
+                       CancelGeolocationRequestInFlight) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL url_a(embedded_test_server()->GetURL("/title1.html"));
+  const GURL url_b(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+  // 1) Navigate to A.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a));
+  RenderFrameHostImpl* rfh_a = current_frame_host();
+
+  // Continuously query current geolocation.
+  EXPECT_TRUE(ExecJs(rfh_a, R"(
+    window.longitude_log = [];
+    window.err_log = [];
+    window.wait_for_first_position = new Promise(resolve => {
+      navigator.geolocation.watchPosition(
+        pos => {
+          window.longitude_log.push(pos.coords.longitude);
+          resolve("resolved");
+        },
+        err => window.err_log.push(err)
+      );
+    })
+  )"));
+  geo_override_.UpdateLocation(0.0, 0.0);
+  EXPECT_EQ("resolved", EvalJs(rfh_a, "window.wait_for_first_position"));
+
+  // Pause resolving Geoposition queries to keep the request inflight.
+  geo_override_.Pause();
+  geo_override_.UpdateLocation(1.0, 1.0);
+  EXPECT_EQ(1u, geo_override_.GetGeolocationInstanceCount());
+
+  // 2) Navigate away.
+  base::RunLoop loop_until_close;
+  geo_override_.SetGeolocationCloseCallback(loop_until_close.QuitClosure());
+
+  RenderFrameDeletedObserver deleted(rfh_a);
+  EXPECT_TRUE(NavigateToURL(shell(), url_b));
+
+  loop_until_close.Run();
+
+  // The page has no inflight geolocation request when we navigated away,
+  // so it should have been cached.
+  EXPECT_FALSE(deleted.deleted());
+  EXPECT_TRUE(rfh_a->is_in_back_forward_cache());
+
+  // Resume resolving Geoposition queries.
+  geo_override_.Resume();
+
+  // We update the location while the page is BFCached, but this location should
+  // not be observed.
+  geo_override_.UpdateLocation(2.0, 2.0);
+
+  // 3) Navigate back to A.
+
+  // The location when navigated back can be observed
+  geo_override_.UpdateLocation(3.0, 3.0);
+
+  web_contents()->GetController().GoBack();
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_EQ(rfh_a, current_frame_host());
+  EXPECT_FALSE(rfh_a->is_in_back_forward_cache());
+
+  // Wait for an update after the user navigates back to A.
+  EXPECT_EQ("resolved", EvalJs(rfh_a, R"(
+    window.wait_for_position_after_resume = new Promise(resolve => {
+      navigator.geolocation.watchPosition(
+        pos => {
+          window.longitude_log.push(pos.coords.longitude);
+          resolve("resolved");
+        },
+        err => window.err_log.push(err)
+      );
+    })
+  )"));
+
+  EXPECT_LE(0, EvalJs(rfh_a, "longitude_log.indexOf(0.0)").ExtractInt())
+      << "Geoposition before the page is put into BFCache should be visible";
+  EXPECT_EQ(-1, EvalJs(rfh_a, "longitude_log.indexOf(1.0)").ExtractInt())
+      << "Geoposition while the page is put into BFCache should be invisible";
+  EXPECT_EQ(-1, EvalJs(rfh_a, "longitude_log.indexOf(2.0)").ExtractInt())
+      << "Geoposition while the page is put into BFCache should be invisible";
+  EXPECT_LT(0, EvalJs(rfh_a, "longitude_log.indexOf(3.0)").ExtractInt())
+      << "Geoposition when the page is restored from BFCache should be visible";
+  EXPECT_EQ(0, EvalJs(rfh_a, "err_log.length"))
+      << "watchPosition API should have reported no errors";
+}
+
 }  // namespace content

content/browser/frame_host/back_forward_cache.cc

@@ -106,8 +106,6 @@ uint64_t GetDisallowedFeatures() {
           WebSchedulerTrackedFeature::kOutstandingIndexedDBTransaction) |
       ToFeatureBit(
           WebSchedulerTrackedFeature::kHasScriptableFramesInMultipleTabs) |
-      ToFeatureBit(
-          WebSchedulerTrackedFeature::kRequestedGeolocationPermission) |
       ToFeatureBit(
           WebSchedulerTrackedFeature::kRequestedNotificationsPermission) |
       ToFeatureBit(WebSchedulerTrackedFeature::kRequestedMIDIPermission) |

services/device/public/cpp/test/scoped_geolocation_overrider.cc

@@ -4,6 +4,7 @@
 
 #include <vector>
 
+#include "base/containers/unique_ptr_adapters.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
@@ -28,8 +29,14 @@ class ScopedGeolocationOverrider::FakeGeolocationContext
   void UpdateLocation(const mojom::Geoposition& position);
   const mojom::Geoposition& GetGeoposition() const;
 
+  void Pause();
+  void Resume();
+
+  size_t GetGeolocationInstanceCount() const;
+
   void BindForOverrideService(
       mojo::PendingReceiver<mojom::GeolocationContext> receiver);
+  void OnDisconnect(FakeGeolocation* impl);
 
   // mojom::GeolocationContext implementation:
   void BindGeolocation(
@@ -37,28 +44,40 @@ class ScopedGeolocationOverrider::FakeGeolocationContext
   void SetOverride(mojom::GeopositionPtr geoposition) override;
   void ClearOverride() override;
 
+  bool is_paused() const { return is_paused_; }
+  void set_close_callback(base::RepeatingClosure callback) {
+    close_callback_ = std::move(callback);
+  }
+
  private:
   mojom::Geoposition position_;
   mojom::GeopositionPtr override_position_;
-  std::vector<std::unique_ptr<FakeGeolocation>> impls_;
+  std::set<std::unique_ptr<FakeGeolocation>, base::UniquePtrComparator> impls_;
   mojo::ReceiverSet<mojom::GeolocationContext> context_receivers_;
+  bool is_paused_ = false;
+  base::RepeatingClosure close_callback_;
 };
 
 class ScopedGeolocationOverrider::FakeGeolocation : public mojom::Geolocation {
  public:
   FakeGeolocation(mojo::PendingReceiver<mojom::Geolocation> receiver,
-                  const FakeGeolocationContext* context);
+                  FakeGeolocationContext* context);
   ~FakeGeolocation() override;
 
-  void UpdateLocation(const mojom::Geoposition& position);
+  void OnDisconnect();
+  void OnResume();
+
+  void UpdateLocation();
 
   // mojom::Geolocation implementation:
   void QueryNextPosition(QueryNextPositionCallback callback) override;
   void SetHighAccuracy(bool high_accuracy) override;
 
  private:
-  const FakeGeolocationContext* context_;
-  bool has_new_position_;
+  void RunPositionCallbackIfNeeded();
+
+  FakeGeolocationContext* context_;
+  bool needs_update_ = true;
   QueryNextPositionCallback position_callback_;
   mojo::Receiver<mojom::Geolocation> receiver_{this};
 };
@@ -111,6 +130,23 @@ void ScopedGeolocationOverrider::UpdateLocation(double latitude,
   UpdateLocation(position);
 }
 
+void ScopedGeolocationOverrider::Pause() {
+  geolocation_context_->Pause();
+}
+
+void ScopedGeolocationOverrider::Resume() {
+  geolocation_context_->Resume();
+}
+
+size_t ScopedGeolocationOverrider::GetGeolocationInstanceCount() const {
+  return geolocation_context_->GetGeolocationInstanceCount();
+}
+
+void ScopedGeolocationOverrider::SetGeolocationCloseCallback(
+    base::RepeatingClosure closure) {
+  geolocation_context_->set_close_callback(std::move(closure));
+}
+
 ScopedGeolocationOverrider::FakeGeolocationContext::FakeGeolocationContext(
     const mojom::Geoposition& position)
     : position_(position) {
@@ -130,10 +166,21 @@ void ScopedGeolocationOverrider::FakeGeolocationContext::UpdateLocation(
     position_.valid = true;
 
   for (auto& impl : impls_) {
-    impl->UpdateLocation(position_);
+    impl->UpdateLocation();
   }
 }
 
+void ScopedGeolocationOverrider::FakeGeolocationContext::OnDisconnect(
+    FakeGeolocation* impl) {
+  // Note: We can't use set::erase() here, since FakeGeolocation* is not
+  //       the impls_::key_type.
+  auto it = impls_.find(impl);
+  impls_.erase(it);
+
+  if (!close_callback_.is_null())
+    close_callback_.Run();
+}
+
 const mojom::Geoposition&
 ScopedGeolocationOverrider::FakeGeolocationContext::GetGeoposition() const {
   if (!override_position_.is_null())
@@ -149,8 +196,7 @@ void ScopedGeolocationOverrider::FakeGeolocationContext::BindForOverrideService(
 
 void ScopedGeolocationOverrider::FakeGeolocationContext::BindGeolocation(
     mojo::PendingReceiver<mojom::Geolocation> receiver) {
-  impls_.push_back(
-      std::make_unique<FakeGeolocation>(std::move(receiver), this));
+  impls_.insert(std::make_unique<FakeGeolocation>(std::move(receiver), this));
 }
 
 void ScopedGeolocationOverrider::FakeGeolocationContext::SetOverride(
@@ -164,7 +210,7 @@ void ScopedGeolocationOverrider::FakeGeolocationContext::SetOverride(
     override_position_->valid = true;
 
   for (auto& impl : impls_) {
-    impl->UpdateLocation(*override_position_);
+    impl->UpdateLocation();
   }
 }
 
@@ -172,22 +218,61 @@ void ScopedGeolocationOverrider::FakeGeolocationContext::ClearOverride() {
   override_position_.reset();
 }
 
+void ScopedGeolocationOverrider::FakeGeolocationContext::Pause() {
+  is_paused_ = true;
+}
+
+void ScopedGeolocationOverrider::FakeGeolocationContext::Resume() {
+  is_paused_ = false;
+  for (auto& impl : impls_) {
+    impl->OnResume();
+  }
+}
+
+size_t ScopedGeolocationOverrider::FakeGeolocationContext::
+    GetGeolocationInstanceCount() const {
+  return impls_.size();
+}
+
 ScopedGeolocationOverrider::FakeGeolocation::FakeGeolocation(
     mojo::PendingReceiver<mojom::Geolocation> receiver,
-    const FakeGeolocationContext* context)
-    : context_(context), has_new_position_(true) {
+    FakeGeolocationContext* context)
+    : context_(context) {
   receiver_.Bind(std::move(receiver));
+  receiver_.set_disconnect_handler(
+      base::BindOnce(&ScopedGeolocationOverrider::FakeGeolocation::OnDisconnect,
+                     base::Unretained(this)));
 }
 
 ScopedGeolocationOverrider::FakeGeolocation::~FakeGeolocation() {}
 
-void ScopedGeolocationOverrider::FakeGeolocation::UpdateLocation(
-    const mojom::Geoposition& position) {
-  has_new_position_ = true;
-  if (!position_callback_.is_null()) {
-    std::move(position_callback_).Run(position.Clone());
-    has_new_position_ = false;
-  }
+void ScopedGeolocationOverrider::FakeGeolocation::OnDisconnect() {
+  context_->OnDisconnect(this);
+}
+
+void ScopedGeolocationOverrider::FakeGeolocation::OnResume() {
+  DCHECK(!context_->is_paused());
+  RunPositionCallbackIfNeeded();
+}
+
+void ScopedGeolocationOverrider::FakeGeolocation::
+    RunPositionCallbackIfNeeded() {
+  // No need to run position callback if paused or no new position pending.
+  if (context_->is_paused() || !needs_update_)
+    return;
+
+  if (position_callback_.is_null())
+    return;
+
+  std::move(position_callback_).Run(context_->GetGeoposition().Clone());
+  needs_update_ = false;
+}
+
+void ScopedGeolocationOverrider::FakeGeolocation::UpdateLocation() {
+  // Needs update for new position.
+  needs_update_ = true;
+
+  RunPositionCallbackIfNeeded();
 }
 
 void ScopedGeolocationOverrider::FakeGeolocation::QueryNextPosition(
@@ -195,10 +280,7 @@ void ScopedGeolocationOverrider::FakeGeolocation::QueryNextPosition(
   // Pending callbacks might be overrided.
   position_callback_ = std::move(callback);
 
-  if (has_new_position_) {
-    std::move(position_callback_).Run(context_->GetGeoposition().Clone());
-    has_new_position_ = false;
-  }
+  RunPositionCallbackIfNeeded();
 }
 
 void ScopedGeolocationOverrider::FakeGeolocation::SetHighAccuracy(

services/device/public/cpp/test/scoped_geolocation_overrider.h

@@ -25,6 +25,25 @@ class ScopedGeolocationOverrider {
   void UpdateLocation(const mojom::Geoposition& position);
   void UpdateLocation(double latitude, double longitude);
 
+  // Pause resolving Geolocation queries to keep request inflight.
+  // After |Pause()| call, Geolocation::QueryNextPosition does not respond,
+  // allowing us to test behavior in the middle of the request.
+  void Pause();
+
+  // Resume resolving Geolocation queries.
+  // Send the paused Geolocation::QueryNextPosition response.
+  void Resume();
+
+  // Count number of active FakeGeolocation instances, which is equal to the
+  // number of active consumer Remote<Geolocation>s.
+  // This is used to verify if consumers properly close the connections when
+  // they should no longer be listening.
+  size_t GetGeolocationInstanceCount() const;
+
+  // Register callback to be notified when a Remote<Geolocation> is cleared and
+  // the corresponding fake Geolocation instance is disposed.
+  void SetGeolocationCloseCallback(base::RepeatingClosure closure);
+
  private:
   class FakeGeolocation;
   class FakeGeolocationContext;