Start geolocation timer after permission prompt

When requesting position estimates through the Geolocation API, the
caller may specify a timeout. If the location provider cannot provide an
estimate before the timeout expires, the returned promise is resolved
with a TIMEOUT error.

The Geolocation API specification requires that the timer is not started
until the page has received the geolocation permission. Prior to this
CL, Chrome would start the timer when the API call is made. This meant
that sometimes the TIMEOUT error would be received before the user had
made a selection in the permission prompt. This could also result in
multiple error callbacks being called for the same geolocation request.

To fix, CreateGeolocation will asynchronously return a PermissionStatus
when the permission prompt is complete. The renderer will delay
starting the GeoNotifier timer until a status is received that
indicates the permission was granted.

BUG=784886

Change-Id: I86294ebf1d212a164e4c3a2cb96c4ad27f759cb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/773575Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Conley Owens <cco3@google.com>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#646465}

content/browser/geolocation/geolocation_service_impl.cc

@@ -8,6 +8,7 @@
 #include "content/browser/permissions/permission_controller_impl.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/browser/render_frame_host.h"
+#include "mojo/public/cpp/bindings/callback_helpers.h"
 #include "third_party/blink/public/mojom/feature_policy/feature_policy.mojom.h"
 
 namespace content {
@@ -35,8 +36,6 @@ void GeolocationServiceImplContext::RequestPermission(
   request_id_ = permission_controller_->RequestPermission(
       PermissionType::GEOLOCATION, render_frame_host,
       render_frame_host->GetLastCommittedOrigin().GetURL(), user_gesture,
-      // NOTE: The permission request is canceled in the destructor, so it is
-      // safe to pass |this| as Unretained.
       base::Bind(&GeolocationServiceImplContext::HandlePermissionStatus,
                  weak_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -71,23 +70,33 @@ void GeolocationServiceImpl::Bind(
 
 void GeolocationServiceImpl::CreateGeolocation(
     mojo::InterfaceRequest<device::mojom::Geolocation> request,
-    bool user_gesture) {
+    bool user_gesture,
+    CreateGeolocationCallback callback) {
   if (!render_frame_host_->IsFeatureEnabled(
           blink::mojom::FeaturePolicyFeature::kGeolocation)) {
+    std::move(callback).Run(blink::mojom::PermissionStatus::DENIED);
     return;
   }
 
+  // If the geolocation service is destroyed before the callback is run, ensure
+  // it is called with DENIED status.
+  auto scoped_callback = mojo::WrapCallbackWithDefaultInvokeIfNotRun(
+      std::move(callback), blink::mojom::PermissionStatus::DENIED);
+
   binding_set_.dispatch_context()->RequestPermission(
       render_frame_host_, user_gesture,
       // There is an assumption here that the GeolocationServiceImplContext will
       // outlive the GeolocationServiceImpl.
       base::Bind(&GeolocationServiceImpl::CreateGeolocationWithPermissionStatus,
-                 base::Unretained(this), base::Passed(&request)));
+                 base::Unretained(this), base::Passed(&request),
+                 base::Passed(&scoped_callback)));
 }
 
 void GeolocationServiceImpl::CreateGeolocationWithPermissionStatus(
     device::mojom::GeolocationRequest request,
+    CreateGeolocationCallback callback,
     blink::mojom::PermissionStatus permission_status) {
+  std::move(callback).Run(permission_status);
   if (permission_status != blink::mojom::PermissionStatus::GRANTED)
     return;
 

content/browser/geolocation/geolocation_service_impl.h

@@ -60,12 +60,14 @@ class CONTENT_EXPORT GeolocationServiceImpl
   // This may not be called a second time until the Geolocation instance has
   // been created.
   void CreateGeolocation(device::mojom::GeolocationRequest request,
-                         bool user_gesture) override;
+                         bool user_gesture,
+                         CreateGeolocationCallback callback) override;
 
  private:
   // Creates the Geolocation Service.
   void CreateGeolocationWithPermissionStatus(
       device::mojom::GeolocationRequest request,
+      CreateGeolocationCallback callback,
       blink::mojom::PermissionStatus permission_status);
 
   device::mojom::GeolocationContext* geolocation_context_;

content/browser/geolocation/geolocation_service_impl_unittest.cc

@@ -156,11 +156,15 @@ TEST_F(GeolocationServiceTest, PermissionGrantedPolicyViolation) {
   CreateEmbeddedFrameAndGeolocationService(/*allow_via_feature_policy=*/false);
 
   permission_manager()->SetRequestCallback(
-      base::Bind([](const PermissionCallback& callback) {
+      base::BindRepeating([](const PermissionCallback& callback) {
         ADD_FAILURE() << "Permissions checked unexpectedly.";
       }));
   GeolocationPtr geolocation;
-  service()->CreateGeolocation(mojo::MakeRequest(&geolocation), true);
+  service()->CreateGeolocation(
+      mojo::MakeRequest(&geolocation), true,
+      base::BindRepeating([](blink::mojom::PermissionStatus status) {
+        EXPECT_EQ(blink::mojom::PermissionStatus::DENIED, status);
+      }));
 
   base::RunLoop loop;
   geolocation.set_connection_error_handler(loop.QuitClosure());
@@ -176,11 +180,15 @@ TEST_F(GeolocationServiceTest, PermissionGrantedNoPolicyViolation) {
   CreateEmbeddedFrameAndGeolocationService(/*allow_via_feature_policy=*/true);
 
   permission_manager()->SetRequestCallback(
-      base::Bind([](const PermissionCallback& callback) {
+      base::BindRepeating([](const PermissionCallback& callback) {
         callback.Run(PermissionStatus::GRANTED);
       }));
   GeolocationPtr geolocation;
-  service()->CreateGeolocation(mojo::MakeRequest(&geolocation), true);
+  service()->CreateGeolocation(
+      mojo::MakeRequest(&geolocation), true,
+      base::BindRepeating([](blink::mojom::PermissionStatus status) {
+        EXPECT_EQ(blink::mojom::PermissionStatus::GRANTED, status);
+      }));
 
   base::RunLoop loop;
   geolocation.set_connection_error_handler(base::BindOnce(
@@ -199,11 +207,15 @@ TEST_F(GeolocationServiceTest, PermissionGrantedNoPolicyViolation) {
 TEST_F(GeolocationServiceTest, PermissionGrantedSync) {
   CreateEmbeddedFrameAndGeolocationService(/*allow_via_feature_policy=*/true);
   permission_manager()->SetRequestCallback(
-      base::Bind([](const PermissionCallback& callback) {
+      base::BindRepeating([](const PermissionCallback& callback) {
         callback.Run(PermissionStatus::GRANTED);
       }));
   GeolocationPtr geolocation;
-  service()->CreateGeolocation(mojo::MakeRequest(&geolocation), true);
+  service()->CreateGeolocation(
+      mojo::MakeRequest(&geolocation), true,
+      base::BindRepeating([](blink::mojom::PermissionStatus status) {
+        EXPECT_EQ(blink::mojom::PermissionStatus::GRANTED, status);
+      }));
 
   base::RunLoop loop;
   geolocation.set_connection_error_handler(base::BindOnce(
@@ -222,11 +234,15 @@ TEST_F(GeolocationServiceTest, PermissionGrantedSync) {
 TEST_F(GeolocationServiceTest, PermissionDeniedSync) {
   CreateEmbeddedFrameAndGeolocationService(/*allow_via_feature_policy=*/true);
   permission_manager()->SetRequestCallback(
-      base::Bind([](const PermissionCallback& callback) {
+      base::BindRepeating([](const PermissionCallback& callback) {
         callback.Run(PermissionStatus::DENIED);
       }));
   GeolocationPtr geolocation;
-  service()->CreateGeolocation(mojo::MakeRequest(&geolocation), true);
+  service()->CreateGeolocation(
+      mojo::MakeRequest(&geolocation), true,
+      base::BindRepeating([](blink::mojom::PermissionStatus status) {
+        EXPECT_EQ(blink::mojom::PermissionStatus::DENIED, status);
+      }));
 
   base::RunLoop loop;
   geolocation.set_connection_error_handler(loop.QuitClosure());
@@ -241,7 +257,7 @@ TEST_F(GeolocationServiceTest, PermissionGrantedAsync) {
   CreateEmbeddedFrameAndGeolocationService(/*allow_via_feature_policy=*/true);
   permission_manager()->SetRequestId(42);
   permission_manager()->SetRequestCallback(
-      base::Bind([](const PermissionCallback& permission_callback) {
+      base::BindRepeating([](const PermissionCallback& permission_callback) {
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE, base::BindOnce(
                            [](const PermissionCallback& callback) {
@@ -250,7 +266,11 @@ TEST_F(GeolocationServiceTest, PermissionGrantedAsync) {
                            permission_callback));
       }));
   GeolocationPtr geolocation;
-  service()->CreateGeolocation(mojo::MakeRequest(&geolocation), true);
+  service()->CreateGeolocation(
+      mojo::MakeRequest(&geolocation), true,
+      base::BindRepeating([](blink::mojom::PermissionStatus status) {
+        EXPECT_EQ(blink::mojom::PermissionStatus::GRANTED, status);
+      }));
 
   base::RunLoop loop;
   geolocation.set_connection_error_handler(base::BindOnce(
@@ -270,7 +290,7 @@ TEST_F(GeolocationServiceTest, PermissionDeniedAsync) {
   CreateEmbeddedFrameAndGeolocationService(/*allow_via_feature_policy=*/true);
   permission_manager()->SetRequestId(42);
   permission_manager()->SetRequestCallback(
-      base::Bind([](const PermissionCallback& permission_callback) {
+      base::BindRepeating([](const PermissionCallback& permission_callback) {
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE, base::BindOnce(
                            [](const PermissionCallback& callback) {
@@ -279,7 +299,11 @@ TEST_F(GeolocationServiceTest, PermissionDeniedAsync) {
                            permission_callback));
       }));
   GeolocationPtr geolocation;
-  service()->CreateGeolocation(mojo::MakeRequest(&geolocation), true);
+  service()->CreateGeolocation(
+      mojo::MakeRequest(&geolocation), true,
+      base::BindRepeating([](blink::mojom::PermissionStatus status) {
+        EXPECT_EQ(blink::mojom::PermissionStatus::DENIED, status);
+      }));
 
   base::RunLoop loop;
   geolocation.set_connection_error_handler(loop.QuitClosure());
@@ -294,7 +318,11 @@ TEST_F(GeolocationServiceTest, ServiceClosedBeforePermissionResponse) {
   CreateEmbeddedFrameAndGeolocationService(/*allow_via_feature_policy=*/true);
   permission_manager()->SetRequestId(42);
   GeolocationPtr geolocation;
-  service()->CreateGeolocation(mojo::MakeRequest(&geolocation), true);
+  service()->CreateGeolocation(
+      mojo::MakeRequest(&geolocation), true,
+      base::BindRepeating([](blink::mojom::PermissionStatus) {
+        ADD_FAILURE() << "PositionStatus received unexpectedly.";
+      }));
   // Don't immediately respond to the request.
   permission_manager()->SetRequestCallback(base::DoNothing());
 

third_party/blink/public/mojom/geolocation/geolocation_service.mojom

@@ -5,14 +5,17 @@
 module blink.mojom;
 
 import "services/device/public/mojom/geolocation.mojom";
+import "third_party/blink/public/mojom/permissions/permission_status.mojom";
 
 // GeolocationService provides a Geolocation. It exists in order to examine the
 // environment (permissions, etc.) before creating a Geolocation.
 // There should only be one GeolocationService instance that creates many
 // Geolocation instances.  There is one GeolocationService per RenderFrameHost.
 interface GeolocationService {
-  // Creates a Geolocation.
+  // Creates a Geolocation. Returns the status of the geolocation permission
+  // request (GRANTED or DENIED).
   // This may not be called a second time until the Geolocation instance has
   // been created.
-  CreateGeolocation(device.mojom.Geolocation& request, bool user_gesture);
+  CreateGeolocation(device.mojom.Geolocation& request, bool user_gesture)
+      => (PermissionStatus status);
 };

third_party/blink/renderer/modules/geolocation/geolocation.cc

@@ -241,9 +241,7 @@ void Geolocation::StartRequest(GeoNotifier* notifier) {
   if (HaveSuitableCachedPosition(notifier->Options())) {
     notifier->SetUseCachedPosition();
   } else {
-    if (notifier->Options()->timeout() > 0)
-      StartUpdating(notifier);
-    notifier->StartTimer();
+    StartUpdating(notifier);
   }
 }
 
@@ -268,9 +266,7 @@ void Geolocation::RequestUsesCachedPosition(GeoNotifier* notifier) {
   if (one_shots_.Contains(notifier)) {
     one_shots_.erase(notifier);
   } else if (watchers_->Contains(notifier)) {
-    if (notifier->Options()->timeout() > 0)
-      StartUpdating(notifier);
-    notifier->StartTimer();
+    StartUpdating(notifier);
   }
 
   if (!HasListeners())
@@ -439,24 +435,27 @@ void Geolocation::StartUpdating(GeoNotifier* notifier) {
     if (geolocation_)
       geolocation_->SetHighAccuracy(true);
   }
-  UpdateGeolocationConnection();
+  UpdateGeolocationConnection(notifier);
 }
 
 void Geolocation::StopUpdating() {
   updating_ = false;
-  UpdateGeolocationConnection();
+  UpdateGeolocationConnection(nullptr);
   enable_high_accuracy_ = false;
 }
 
-void Geolocation::UpdateGeolocationConnection() {
+void Geolocation::UpdateGeolocationConnection(GeoNotifier* notifier) {
   if (!GetExecutionContext() || !GetPage() || !GetPage()->IsPageVisible() ||
       !updating_) {
     geolocation_.reset();
     disconnected_geolocation_ = true;
     return;
   }
-  if (geolocation_)
+  if (geolocation_) {
+    if (notifier)
+      notifier->StartTimer();
     return;
+  }
 
   // See https://bit.ly/2S0zRAS for task types.
   scoped_refptr<base::SingleThreadTaskRunner> task_runner =
@@ -467,7 +466,9 @@ void Geolocation::UpdateGeolocationConnection() {
                                                   invalidator, task_runner);
   geolocation_service_->CreateGeolocation(
       MakeRequest(&geolocation_, invalidator, std::move(task_runner)),
-      LocalFrame::HasTransientUserActivation(GetFrame()));
+      LocalFrame::HasTransientUserActivation(GetFrame()),
+      WTF::Bind(&Geolocation::OnGeolocationPermissionStatusUpdated,
+                WrapWeakPersistent(this), WrapWeakPersistent(notifier)));
 
   geolocation_.set_connection_error_handler(WTF::Bind(
       &Geolocation::OnGeolocationConnectionError, WrapWeakPersistent(this)));
@@ -496,7 +497,7 @@ void Geolocation::OnPositionUpdated(
 }
 
 void Geolocation::PageVisibilityChanged() {
-  UpdateGeolocationConnection();
+  UpdateGeolocationConnection(nullptr);
 }
 
 bool Geolocation::HasPendingActivity() const {
@@ -514,4 +515,15 @@ void Geolocation::OnGeolocationConnectionError() {
   HandleError(error);
 }
 
+void Geolocation::OnGeolocationPermissionStatusUpdated(
+    GeoNotifier* notifier,
+    mojom::PermissionStatus status) {
+  if (notifier && status == mojom::PermissionStatus::GRANTED) {
+    // Avoid starting the notifier timer if the notifier has already been
+    // removed.
+    if (DoesOwnNotifier(notifier))
+      notifier->StartTimer();
+  }
+}
+
 }  // namespace blink

third_party/blink/renderer/modules/geolocation/geolocation.h

@@ -47,6 +47,10 @@
 
 namespace blink {
 
+namespace mojom {
+enum class PermissionStatus;
+}  // namespace mojom
+
 class Document;
 class LocalFrame;
 class ExecutionContext;
@@ -176,7 +180,7 @@ class MODULES_EXPORT Geolocation final
 
   void StopUpdating();
 
-  void UpdateGeolocationConnection();
+  void UpdateGeolocationConnection(GeoNotifier*);
   void QueryNextPosition();
 
   // Attempts to obtain a position for the given notifier, either by using
@@ -196,6 +200,9 @@ class MODULES_EXPORT Geolocation final
 
   void OnGeolocationConnectionError();
 
+  void OnGeolocationPermissionStatusUpdated(GeoNotifier*,
+                                            mojom::PermissionStatus);
+
   GeoNotifierSet one_shots_;
   TraceWrapperMember<GeolocationWatchers> watchers_;
   // GeoNotifiers that are in the middle of invocation.

third_party/blink/web_tests/geolocation-api/resources/geolocation-mock.js

@@ -19,6 +19,11 @@ class GeolocationMock {
     */
     this.geoposition_ = null;
 
+    /**
+     * While true, position requests will result in a timeout error.
+     */
+    this.shouldTimeout_ = false;
+
     /**
      * A pending request for permission awaiting a decision to be set via a
      * setGeolocationPermission call.
@@ -62,6 +67,11 @@ class GeolocationMock {
    * setGeolocationPositionUnavailableError().
    */
   queryNextPosition() {
+    if (this.shouldTimeout_) {
+      // Return a promise that will never be resolved. Since no geoposition is
+      // returned, the request will eventually time out.
+      return new Promise((resolve, reject) => {});
+    }
     if (!this.geoposition_) {
       this.setGeolocationPositionUnavailableError(
           'Test error: position not set before call to queryNextPosition()');
@@ -117,6 +127,13 @@ class GeolocationMock {
         device.mojom.Geoposition.ErrorCode.POSITION_UNAVAILABLE;
   }
 
+  /**
+   * Sets whether geolocation requests should cause timeout errors.
+   */
+  setGeolocationTimeoutError(shouldTimeout) {
+    this.shouldTimeout_ = shouldTimeout;
+  }
+
   /**
    * Reject any connection requests for the geolocation service. This will
    * trigger a connection error in the client.
@@ -133,6 +150,11 @@ class GeolocationMock {
   createGeolocation(request, user_gesture) {
     switch (this.permissionStatus_) {
      case blink.mojom.PermissionStatus.ASK:
+      return new Promise((resolve, reject) => {
+        setTimeout(() => {
+          resolve(this.createGeolocation(request, user_gesture));
+        }, 50);
+      });
       setTimeout(() => { this.createGeolocation(request, user_gesture)}, 50);
       break;
 
@@ -143,6 +165,7 @@ class GeolocationMock {
      default:
       request.close();
     }
+    return Promise.resolve(this.permissionStatus_);
   }
 
   /**

third_party/blink/web_tests/geolocation-api/timeout-clear-watch.html

@@ -14,8 +14,8 @@ description("Tests that when a watch times out and is cleared from the error cal
 
 var error;
 
-let mock = geolocationMock;
-mock.setGeolocationPosition(51.478, -0.166, 100.0);
+geolocationMock.setGeolocationPermission(true);
+geolocationMock.setGeolocationTimeoutError(true);
 
 var watchId = navigator.geolocation.watchPosition(function() {
     testFailed('Success callback invoked unexpectedly');

third_party/blink/web_tests/geolocation-api/timeout-negative.html

@@ -14,8 +14,8 @@ description("Tests that when timeout is negative (and maximumAge is too), the er
 
 var error;
 
-let mock = geolocationMock;
-mock.setGeolocationPosition(51.478, -0.166, 100.0);
+geolocationMock.setGeolocationPermission(true);
+geolocationMock.setGeolocationTimeoutError(true);
 
 navigator.geolocation.getCurrentPosition(function(p) {
     testFailed('Success callback invoked unexpectedly');

third_party/blink/web_tests/geolocation-api/timeout-zero.html

@@ -14,7 +14,8 @@ description("Tests that when timeout is zero (and maximumAge is too), the error
 
 var error;
 
-geolocationMock.setGeolocationPosition(51.478, -0.166, 100.0);
+geolocationMock.setGeolocationPermission(true);
+geolocationMock.setGeolocationTimeoutError(true);
 
 navigator.geolocation.getCurrentPosition(function(p) {
     testFailed('Success callback invoked unexpectedly');