Bug 1905717 - Add a timeout for geoclue -> GLS fallback. r=saschanaz a=RyanVM

Use 10s which feels it should be enough for geoclue to provide a
reasonable location, without feeling too slow for users.

Still if geoclue eventually gives us a proper location we'd update it
properly and stop the network fallback so this seems reasonable.

I made some naming tweaks for consistency but otherwise it should be
reasonably straight-forward.

Differential Revision: https://phabricator.services.mozilla.com/D215491

dom/system/linux/GeoclueLocationProvider.cpp

@@ -81,27 +81,7 @@ class GCLocProviderPriv final : public nsIGeolocationProvider,
 
   GCLocProviderPriv();
 
-  void UpdateLastPosition();
-
  private:
-  class LocationTimerCallback final : public nsITimerCallback, public nsINamed {
-   public:
-    NS_DECL_ISUPPORTS
-    NS_DECL_NSITIMERCALLBACK
-
-    explicit LocationTimerCallback(GCLocProviderPriv* aParent)
-        : mParent(aParent) {}
-
-    NS_IMETHOD GetName(nsACString& aName) override {
-      aName.AssignLiteral("GCLocProvider::LocationTimerCallback");
-      return NS_OK;
-    }
-
-   private:
-    ~LocationTimerCallback() = default;
-    WeakPtr<GCLocProviderPriv> mParent;
-  };
-
   enum class Accuracy { Unset, Low, High };
   // States:
   //   Uninit: The default / initial state, with no client proxy yet.
@@ -232,7 +212,6 @@ class GCLocProviderPriv final : public nsIGeolocationProvider,
   MOZ_CAN_RUN_SCRIPT static void GCManagerOwnerNotify(GObject* aObject,
                                                       GParamSpec* aPSpec,
                                                       gpointer aUserData);
-
   static void GCClientSignal(GDBusProxy* aProxy, gchar* aSenderName,
                              gchar* aSignalName, GVariant* aParameters,
                              gpointer aUserData);
@@ -242,8 +221,13 @@ class GCLocProviderPriv final : public nsIGeolocationProvider,
   static void ConnectLocationResponse(GObject* aObject, GAsyncResult* aResult,
                                       gpointer aUserData);
 
-  void SetLocationTimer();
+  void StartLastPositionTimer();
-  void StopLocationTimer();
+  void StopPositionTimer();
+  void UpdateLastPosition();
+
+  void StartMLSFallbackTimerIfNeeded();
+  void StopMLSFallbackTimer();
+  void MLSFallbackTimerFired();
 
   bool InDBusCall();
   bool InDBusStoppingCall();
@@ -266,10 +250,44 @@ class GCLocProviderPriv final : public nsIGeolocationProvider,
   nsCOMPtr<nsIGeolocationUpdate> mCallback;
   ClientState mClientState = ClientState::Uninit;
   RefPtr<nsIDOMGeoPosition> mLastPosition;
-  RefPtr<nsITimer> mLocationTimer;
+  RefPtr<nsITimer> mLastPositionTimer;
+  RefPtr<nsITimer> mMLSFallbackTimer;
   RefPtr<MLSFallback> mMLSFallback;
 };
 
+class GCLocWeakCallback final : public nsITimerCallback, public nsINamed {
+  using Method = void (GCLocProviderPriv::*)();
+
+ public:
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSITIMERCALLBACK
+
+  explicit GCLocWeakCallback(GCLocProviderPriv* aParent, const char* aName,
+                             Method aMethod)
+      : mParent(aParent), mName(aName), mMethod(aMethod) {}
+
+  NS_IMETHOD GetName(nsACString& aName) override {
+    aName = mName;
+    return NS_OK;
+  }
+
+ private:
+  ~GCLocWeakCallback() = default;
+  WeakPtr<GCLocProviderPriv> mParent;
+  const char* mName = nullptr;
+  Method mMethod = nullptr;
+};
+
+NS_IMPL_ISUPPORTS(GCLocWeakCallback, nsITimerCallback, nsINamed)
+
+NS_IMETHODIMP
+GCLocWeakCallback::Notify(nsITimer* aTimer) {
+  if (RefPtr parent = mParent.get()) {
+    (parent->*mMethod)();
+  }
+  return NS_OK;
+}
+
 //
 // GCLocProviderPriv
 //
@@ -310,7 +328,8 @@ void GCLocProviderPriv::Update(nsIDOMGeoPosition* aPosition) {
 
 void GCLocProviderPriv::UpdateLastPosition() {
   MOZ_DIAGNOSTIC_ASSERT(mLastPosition, "No last position to update");
-  StopLocationTimer();
+  StopPositionTimer();
+  StopMLSFallbackTimer();
   Update(mLastPosition);
 }
 
@@ -381,9 +400,9 @@ void GCLocProviderPriv::GetClientResponse(GDBusProxy* aProxy,
   RefPtr<GVariant> variant = dont_AddRef(
       g_dbus_proxy_call_finish(aProxy, aResult, getter_Transfers(error)));
   if (!variant) {
+    GCL_LOG(Error, "Failed to get client: %s\n", error->message);
     // if cancelled |self| might no longer be there
     if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
-      GCL_LOG(Error, "Failed to get client: %s\n", error->message);
       RefPtr self = static_cast<GCLocProviderPriv*>(aUserData);
       self->DBusProxyError(error.get(), true);
     }
@@ -602,6 +621,9 @@ void GCLocProviderPriv::StartClientResponse(GDBusProxy* aProxy,
   MOZ_DIAGNOSTIC_ASSERT(self->mClientState == ClientState::Starting,
                         "Client in a wrong state");
   GCLP_SETSTATE(self, Started);
+  // If we're started, and we don't get any location update in a reasonable
+  // amount of time, we fallback to MLS.
+  self->StartMLSFallbackTimerIfNeeded();
   self->MaybeRestartForAccuracy();
 }
 
@@ -689,6 +711,9 @@ void GCLocProviderPriv::GCClientSignal(GDBusProxy* aProxy, gchar* aSenderName,
                                        gchar* aSignalName,
                                        GVariant* aParameters,
                                        gpointer aUserData) {
+  GCL_LOG(Info, "%s: %s (%s)\n", __PRETTY_FUNCTION__, aSignalName,
+          GUniquePtr<gchar>(g_variant_print(aParameters, TRUE)).get());
+
   if (g_strcmp0(aSignalName, "LocationUpdated")) {
     return;
   }
@@ -802,23 +827,63 @@ void GCLocProviderPriv::ConnectLocationResponse(GObject* aObject,
   self->UpdateLastPosition();
 }
 
-void GCLocProviderPriv::SetLocationTimer() {
+void GCLocProviderPriv::StartLastPositionTimer() {
   MOZ_DIAGNOSTIC_ASSERT(mLastPosition, "no last position to report");
 
-  StopLocationTimer();
+  StopPositionTimer();
 
-  RefPtr<LocationTimerCallback> timerCallback = new LocationTimerCallback(this);
+  RefPtr timerCallback = new GCLocWeakCallback(
-  NS_NewTimerWithCallback(getter_AddRefs(mLocationTimer), timerCallback, 1000,
+      this, "UpdateLastPosition", &GCLocProviderPriv::UpdateLastPosition);
-                          nsITimer::TYPE_ONE_SHOT);
+  NS_NewTimerWithCallback(getter_AddRefs(mLastPositionTimer), timerCallback,
+                          1000, nsITimer::TYPE_ONE_SHOT);
 }
 
-void GCLocProviderPriv::StopLocationTimer() {
+void GCLocProviderPriv::StopPositionTimer() {
-  if (!mLocationTimer) {
+  if (!mLastPositionTimer) {
     return;
   }
 
-  mLocationTimer->Cancel();
+  mLastPositionTimer->Cancel();
-  mLocationTimer = nullptr;
+  mLastPositionTimer = nullptr;
+}
+
+void GCLocProviderPriv::StartMLSFallbackTimerIfNeeded() {
+  StopMLSFallbackTimer();
+  if (mLastPosition) {
+    // If we already have a location we're good.
+    return;
+  }
+
+  uint32_t delay = StaticPrefs::geo_provider_geoclue_mls_fallback_timeout_ms();
+  if (!delay) {
+    return;
+  }
+
+  RefPtr timerCallback = new GCLocWeakCallback(
+      this, "MLSFallbackTimerFired", &GCLocProviderPriv::MLSFallbackTimerFired);
+  NS_NewTimerWithCallback(getter_AddRefs(mMLSFallbackTimer), timerCallback,
+                          delay, nsITimer::TYPE_ONE_SHOT);
+}
+
+void GCLocProviderPriv::StopMLSFallbackTimer() {
+  if (!mMLSFallbackTimer) {
+    return;
+  }
+  mMLSFallbackTimer->Cancel();
+  mMLSFallbackTimer = nullptr;
+}
+
+void GCLocProviderPriv::MLSFallbackTimerFired() {
+  mMLSFallbackTimer = nullptr;
+
+  if (mMLSFallback || mLastPosition || mClientState != ClientState::Started) {
+    return;
+  }
+
+  GCL_LOG(Info,
+          "Didn't get a location in a reasonable amount of time, trying to "
+          "fall back to MLS");
+  FallbackToMLS();
 }
 
 // Did we made some D-Bus call and are still waiting for its response?
@@ -861,7 +926,8 @@ void GCLocProviderPriv::DoShutdown(bool aDeleteClient, bool aDeleteManager) {
       "deleting manager proxy requires deleting client one, too");
 
   // Invalidate the cached last position
-  StopLocationTimer();
+  StopPositionTimer();
+  StopMLSFallbackTimer();
   mLastPosition = nullptr;
 
   /*
@@ -955,10 +1021,10 @@ void GCLocProviderPriv::WatchStart() {
   if (mClientState == ClientState::Idle) {
     StartClient();
   } else if (mClientState == ClientState::Started) {
-    if (mLastPosition && !mLocationTimer) {
+    if (mLastPosition && !mLastPositionTimer) {
       GCL_LOG(Verbose,
-              "Will report the existing location if new one doesn't come up\n");
+              "Will report the existing position if new one doesn't come up\n");
-      SetLocationTimer();
+      StartLastPositionTimer();
     }
   } else if (mClientState == ClientState::SettingAccuracy) {
     GCLP_SETSTATE(this, SettingAccuracyForStart);
@@ -1016,19 +1082,6 @@ GCLocProviderPriv::SetHighAccuracy(bool aHigh) {
   return NS_OK;
 }
 
-NS_IMPL_ISUPPORTS(GCLocProviderPriv::LocationTimerCallback, nsITimerCallback,
-                  nsINamed)
-
-NS_IMETHODIMP
-GCLocProviderPriv::LocationTimerCallback::Notify(nsITimer* aTimer) {
-  if (mParent) {
-    RefPtr<GCLocProviderPriv> parent(mParent);
-    parent->UpdateLastPosition();
-  }
-
-  return NS_OK;
-}
-
 GeoclueLocationProvider::GeoclueLocationProvider() {
   mPriv = new GCLocProviderPriv;
 }

modules/libpref/init/StaticPrefList.yaml

@@ -5631,6 +5631,13 @@
   type: bool
   value: true
   mirror: always
+
+# Time in milliseconds after which geoclue will try to fallback to MLS if no
+# location is received after successful start.
+- name: geo.provider.geoclue.mls_fallback_timeout_ms
+  type: uint32_t
+  value: 10000
+  mirror: always
 #endif
 
 #---------------------------------------------------------------------------