Bug 826102 - Make IPC geolocation listener never use cached values and never skip a value. r=dougt

dom/ipc/ContentParent.cpp

@@ -2005,14 +2005,20 @@ ContentParent::RecvAddGeolocationListener(const IPC::Principal& aPrincipal)
   }
 #endif
 
-  if (mGeolocationWatchID == -1) {
-    nsCOMPtr<nsIDOMGeoGeolocation> geo = do_GetService("@mozilla.org/geolocation;1");
-    if (!geo) {
-      return true;
-    }
-    jsval dummy = JSVAL_VOID;
-    geo->WatchPosition(this, nullptr, dummy, nullptr, &mGeolocationWatchID);
+  // To ensure no geolocation updates are skipped, we always force the
+  // creation of a new listener.
+  RecvRemoveGeolocationListener();
+
+  nsCOMPtr<nsIDOMGeoGeolocation> geo = do_GetService("@mozilla.org/geolocation;1");
+  if (!geo) {
+    return true;
   }
+
+  nsRefPtr<nsGeolocation> geosvc = static_cast<nsGeolocation*>(geo.get());
+  nsAutoPtr<mozilla::dom::GeoPositionOptions> options(new mozilla::dom::GeoPositionOptions());
+  jsval null = JS::NullValue();
+  options->Init(nullptr, &null);
+  geosvc->WatchPosition(this, nullptr, options.forget(), &mGeolocationWatchID);
   return true;
 }
 

dom/src/geolocation/nsGeoPositionIPCSerialiser.h

@@ -9,15 +9,14 @@
 #include "nsGeoPosition.h"
 #include "nsIDOMGeoPosition.h"
 
-typedef nsGeoPositionCoords       *GeoPositionCoords;
-typedef nsIDOMGeoPosition         *GeoPosition;
+typedef nsIDOMGeoPosition* GeoPosition;
 
 namespace IPC {
 
 template <>
-struct ParamTraits<GeoPositionCoords>
+struct ParamTraits<nsIDOMGeoPositionCoords*>
 {
-  typedef GeoPositionCoords paramType;
+  typedef nsIDOMGeoPositionCoords* paramType;
 
   // Function to serialize a geoposition
   static void Write(Message *aMsg, const paramType& aParam)
@@ -96,9 +95,9 @@ struct ParamTraits<GeoPositionCoords>
 };
 
 template <>
-struct ParamTraits<GeoPosition>
+struct ParamTraits<nsIDOMGeoPosition*>
 {
-  typedef GeoPosition paramType;
+  typedef nsIDOMGeoPosition* paramType;
 
   // Function to serialize a geoposition
   static void Write(Message *aMsg, const paramType& aParam)
@@ -114,8 +113,7 @@ struct ParamTraits<GeoPosition>
 
     nsCOMPtr<nsIDOMGeoPositionCoords> coords;
     aParam->GetCoords(getter_AddRefs(coords));
-    GeoPositionCoords simpleCoords = static_cast<GeoPositionCoords>(coords.get());
-    WriteParam(aMsg, simpleCoords);
+    WriteParam(aMsg, coords.get());
   }
 
   // Function to de-serialize a geoposition
@@ -131,16 +129,14 @@ struct ParamTraits<GeoPosition>
     }
 
     DOMTimeStamp timeStamp;
-    GeoPositionCoords coords = nullptr;
+    nsIDOMGeoPositionCoords* coords = nullptr;
 
     // It's not important to us where it fails, but rather if it fails
-    if (!(   ReadParam(aMsg, aIter, &timeStamp)
-          && ReadParam(aMsg, aIter, &coords   ))) {
-          // note it is fine to do "delete nullptr" in case coords hasn't
-          // been allocated
-          delete coords;
-          return false;
-      }
+    if (!ReadParam(aMsg, aIter, &timeStamp) ||
+        !ReadParam(aMsg, aIter, &coords)) {
+      nsCOMPtr<nsIDOMGeoPositionCoords> tmpcoords = coords;
+      return false;
+    }
 
     *aResult = new nsGeoPosition(coords, timeStamp);
 

dom/src/geolocation/nsGeolocation.cpp

@@ -285,6 +285,7 @@ nsDOMGeoPositionError::NotifyCallback(nsIDOMGeoPositionErrorCallback* aCallback)
 nsGeolocationRequest::nsGeolocationRequest(nsGeolocation* aLocator,
                                            nsIDOMGeoPositionCallback* aCallback,
                                            nsIDOMGeoPositionErrorCallback* aErrorCallback,
+                                           mozilla::dom::GeoPositionOptions* aOptions,
                                            bool aWatchPositionRequest,
                                            int32_t aWatchId)
   : mAllowed(false),
@@ -293,6 +294,7 @@ nsGeolocationRequest::nsGeolocationRequest(nsGeolocation* aLocator,
     mIsWatchPositionRequest(aWatchPositionRequest),
     mCallback(aCallback),
     mErrorCallback(aErrorCallback),
+    mOptions(aOptions),
     mLocator(aLocator),
     mWatchId(aWatchId)
 {
@@ -302,15 +304,21 @@ nsGeolocationRequest::~nsGeolocationRequest()
 {
 }
 
-nsresult
-nsGeolocationRequest::Init(JSContext* aCx, const jsval& aOptions)
+
+static mozilla::dom::GeoPositionOptions*
+OptionsFromJSOptions(JSContext* aCx, const jsval& aOptions, nsresult* aRv)
 {
+  *aRv = NS_OK;
+  nsAutoPtr<mozilla::dom::GeoPositionOptions> options(nullptr);
   if (aCx && !JSVAL_IS_VOID(aOptions) && !JSVAL_IS_NULL(aOptions)) {
-    mOptions = new mozilla::dom::GeoPositionOptions();
-    nsresult rv = mOptions->Init(aCx, &aOptions);
-    NS_ENSURE_SUCCESS(rv, rv);
+    options = new mozilla::dom::GeoPositionOptions();
+    nsresult rv = options->Init(aCx, &aOptions);
+    if (NS_FAILED(rv)) {
+      *aRv = rv;
+      return nullptr;
+    }
   }
-  return NS_OK;
+  return options.forget();
 }
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsGeolocationRequest)
@@ -865,10 +873,6 @@ nsGeolocationService::IsBetterPosition(nsIDOMGeoPosition *aSomewhere)
     return false;
   }
 
-  if (XRE_GetProcessType() == GeckoProcessType_Content) {
-    return true;
-  }
-
   if (mProviders.Count() == 1 || !mLastPosition) {
     return true;
   }
@@ -1242,8 +1246,20 @@ nsGeolocation::Update(nsIDOMGeoPosition *aSomewhere, bool aIsBetter)
 NS_IMETHODIMP
 nsGeolocation::GetCurrentPosition(nsIDOMGeoPositionCallback *callback,
                                   nsIDOMGeoPositionErrorCallback *errorCallback,
-                                  const jsval& options,
+                                  const jsval& jsoptions,
                                   JSContext* cx)
+{
+  nsresult rv;
+  nsAutoPtr<mozilla::dom::GeoPositionOptions> options(
+      OptionsFromJSOptions(cx, jsoptions, &rv));
+  NS_ENSURE_SUCCESS(rv, rv);
+  return GetCurrentPosition(callback, errorCallback, options.forget());
+}
+
+nsresult
+nsGeolocation::GetCurrentPosition(nsIDOMGeoPositionCallback *callback,
+                                  nsIDOMGeoPositionErrorCallback *errorCallback,
+                                  mozilla::dom::GeoPositionOptions *options)
 {
   NS_ENSURE_ARG_POINTER(callback);
 
@@ -1254,13 +1270,8 @@ nsGeolocation::GetCurrentPosition(nsIDOMGeoPositionCallback *callback,
   nsRefPtr<nsGeolocationRequest> request = new nsGeolocationRequest(this,
                                                                     callback,
                                                                     errorCallback,
+                                                                    options,
                                                                     false);
-  if (!request) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  nsresult rv = request->Init(cx, options);
-  NS_ENSURE_SUCCESS(rv, rv);
 
   if (!sGeoEnabled) {
     nsCOMPtr<nsIRunnable> ev = new RequestAllowEvent(false, request);
@@ -1307,9 +1318,22 @@ nsGeolocation::GetCurrentPositionReady(nsGeolocationRequest* aRequest)
 NS_IMETHODIMP
 nsGeolocation::WatchPosition(nsIDOMGeoPositionCallback *callback,
                              nsIDOMGeoPositionErrorCallback *errorCallback,
-                             const jsval& options,
+                             const jsval& jsoptions,
                              JSContext* cx,
                              int32_t *_retval)
+{
+  nsresult rv;
+  nsAutoPtr<mozilla::dom::GeoPositionOptions> options(
+      OptionsFromJSOptions(cx, jsoptions, &rv));
+  NS_ENSURE_SUCCESS(rv, rv);
+  return WatchPosition(callback, errorCallback, options.forget(), _retval);
+}
+
+nsresult
+nsGeolocation::WatchPosition(nsIDOMGeoPositionCallback *callback,
+                             nsIDOMGeoPositionErrorCallback *errorCallback,
+                             mozilla::dom::GeoPositionOptions *options,
+                             int32_t *_retval)
 {
   NS_ENSURE_ARG_POINTER(callback);
 
@@ -1323,14 +1347,9 @@ nsGeolocation::WatchPosition(nsIDOMGeoPositionCallback *callback,
   nsRefPtr<nsGeolocationRequest> request = new nsGeolocationRequest(this,
                                                                     callback,
                                                                     errorCallback,
+                                                                    options,
                                                                     true,
                                                                     *_retval);
-  if (!request) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  nsresult rv = request->Init(cx, options);
-  NS_ENSURE_SUCCESS(rv, rv);
 
   if (!sGeoEnabled) {
     nsCOMPtr<nsIRunnable> ev = new RequestAllowEvent(false, request);

dom/src/geolocation/nsGeolocation.h

@@ -53,9 +53,9 @@ class nsGeolocationRequest
   nsGeolocationRequest(nsGeolocation* locator,
                        nsIDOMGeoPositionCallback* callback,
                        nsIDOMGeoPositionErrorCallback* errorCallback,
+                       mozilla::dom::GeoPositionOptions* aOptions,
                        bool watchPositionRequest = false,
                        int32_t watchId = 0);
-  nsresult Init(JSContext* aCx, const jsval& aOptions);
   void Shutdown();
 
   // Called by the geolocation device to notify that a location has changed.
@@ -75,7 +75,6 @@ class nsGeolocationRequest
   void IPDLRelease() { Release(); }
 
   int32_t WatchId() { return mWatchId; }
-
  private:
 
   void NotifyError(int16_t errorCode);
@@ -202,6 +201,14 @@ public:
   // Notification from the service:
   void ServiceReady();
 
+  // Versions of the DOM APIs that don't require JS option values
+  nsresult WatchPosition(nsIDOMGeoPositionCallback *callback,
+                         nsIDOMGeoPositionErrorCallback *errorCallback,
+                         mozilla::dom::GeoPositionOptions *options,
+                         int32_t *_retval);
+  nsresult GetCurrentPosition(nsIDOMGeoPositionCallback *callback,
+                              nsIDOMGeoPositionErrorCallback *errorCallback,
+                              mozilla::dom::GeoPositionOptions *options);
 private:
 
   ~nsGeolocation();

dom/system/NetworkGeolocationProvider.js

@@ -15,7 +15,7 @@ const Cc = Components.classes;
 
 let gLoggingEnabled = false;
 let gTestingEnabled = false;
-let gDesist = false;
+let gUseScanning = true;
 
 let gPrivateAccessToken = '';
 let gPrivateAccessTime = 0;
@@ -76,6 +76,10 @@ function WifiGeoPositionProvider() {
     gTestingEnabled = Services.prefs.getBoolPref("geo.wifi.testing");
   } catch (e) {}
 
+  try {
+    gUseScanning = Services.prefs.getBoolPref("geo.wifi.scan");
+  } catch (e) {}
+
   this.wifiService = null;
   this.timer = null;
   this.hasSeenWiFi = false;
@@ -111,20 +115,20 @@ WifiGeoPositionProvider.prototype = {
   watch: function(c, requestPrivate) {
     LOG("watch called");
 
-    let useScanning = true;
-    try {
-      useScanning = Services.prefs.getBoolPref("geo.wifi.scan");
-    } catch (e) {}
-
-    if (!this.wifiService && useScanning) {
+    if (!this.wifiService && gUseScanning) {
       this.wifiService = Cc["@mozilla.org/wifi/monitor;1"].getService(Components.interfaces.nsIWifiMonitor);
       this.wifiService.startWatching(this);
       this.lastRequestPrivate = requestPrivate;
     }
     if (this.hasSeenWiFi) {
       this.hasSeenWiFi = false;
-      this.wifiService.stopWatching(this);
-      this.wifiService.startWatching(this);
+      if (gUseScanning) {
+        this.wifiService.stopWatching(this);
+        this.wifiService.startWatching(this);
+      } else {
+        // For testing situations, ensure that we always trigger an update.
+        this.timer.initWithCallback(this, 5000, this.timer.TYPE_ONE_SHOT);
+      }
       this.lastRequestPrivate = requestPrivate;
     }
   },
@@ -314,7 +318,7 @@ WifiGeoPositionProvider.prototype = {
   },
 
   notify: function (timer) {
-    if (gTestingEnabled) {
+    if (gTestingEnabled || !gUseScanning) {
       // if we are testing, timer is repeating
       this.onChange(null);
     }

dom/tests/unit/test_multiple_geo_listeners.js

@@ -0,0 +1,69 @@
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+const Cu = Components.utils;
+const Cr = Components.results;
+
+Cu.import("resource://testing-common/httpd.js");
+
+var httpserver = null;
+var geolocation = null;
+var success = false;
+var watchId = -1;
+
+let gAccuracy = 42;
+function geoHandler(metadata, response)
+{
+  var georesponse = {
+      status: "OK",
+      location: {
+          lat: 0.45,
+          lng: 0.45,
+      },
+      accuracy: gAccuracy,
+  };
+  var position = JSON.stringify(georesponse);
+  response.setStatusLine("1.0", 200, "OK");
+  response.setHeader("Cache-Control", "no-cache", false);
+  response.setHeader("Content-Type", "aplication/x-javascript", false);
+  response.write(position);
+}
+
+function errorCallback() {
+  do_check_true(false);
+  do_test_finished();
+}
+
+function run_test()
+{
+  do_test_pending();
+
+  httpserver = new HttpServer();
+  httpserver.registerPathHandler("/geo", geoHandler);
+  httpserver.start(4444);
+
+  if (Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
+        .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
+    var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
+    prefs.setBoolPref("geo.wifi.scan", false);
+    prefs.setCharPref("geo.wifi.uri", "http://localhost:4444/geo");
+  }
+
+  let timesCalled = 0;
+  geolocation = Cc["@mozilla.org/geolocation;1"].createInstance(Ci.nsIDOMGeoGeolocation);
+  geolocation.watchPosition(function(pos) {
+    do_check_eq(++timesCalled, 1);
+    do_check_eq(pos.coords.accuracy, gAccuracy);
+
+    gAccuracy = 420;
+    geolocation2 = Cc["@mozilla.org/geolocation;1"].createInstance(Ci.nsIDOMGeoGeolocation);
+    geolocation2.getCurrentPosition(function(pos) {
+      do_check_eq(pos.coords.accuracy, gAccuracy);
+
+      gAccuracy = 400;
+      geolocation2.getCurrentPosition(function(pos) {
+        do_check_eq(pos.coords.accuracy, 42);
+        do_test_finished();
+      }, errorCallback);
+    }, errorCallback, {maximumAge: 0});
+  }, errorCallback, {maximumAge: 0});
+}

dom/tests/unit/test_multiple_geo_listeners_wrap.js

@@ -0,0 +1,9 @@
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+
+function run_test() {
+  var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
+  prefs.setBoolPref("geo.wifi.scan", false);
+  prefs.setCharPref("geo.wifi.uri", "http://localhost:4444/geo");
+  run_test_in_child("test_multiple_geo_listeners.js");
+}

dom/tests/unit/xpcshell.ini

@@ -5,8 +5,11 @@ tail =
 [test_bug319968.js]
 [test_bug465752.js]
 [test_geolocation_provider.js]
+# Bug 684962: test hangs consistently on Android
+skip-if = os == "android"
 [test_geolocation_timeout.js]
 [test_geolocation_timeout_wrap.js]
 skip-if = os == "mac"
-# Bug 684962: test hangs consistently on Android
-skip-if = os == "android"
+[test_multiple_geo_listeners.js]
+[test_multiple_geo_listeners_wrap.js]
+skip-if = os == "mac"