Closed Bug 996998 Opened 6 years ago Closed 6 years ago

Geolocation jumping

Categories

(Core :: DOM: Geolocation, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 2.0+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: gerard-majax, Assigned: garvan)

References

Details

Attachments

(1 file, 2 obsolete files)

With current gecko/gaia master running on some devices (Nexus S especially), I noticed some jumping in the geolocation.

STR:
 0. Start HERE Maps
 1. Allow Geolocation
 2. GPS starts
 3. A first geolocation is gathered from Mozilla location services, via GeoIP, that locates me in Paris, with accuraccy of 40km
 4. Several seconds after, GPS gets a fix with accuraccy around 20m and correctly located
 5. Fix stays for a couple of seconds
 6. Once in a while but constanly, location will jump between to the one we got in (3) and (4)

In my case, the jump is around 250km.
Assignee: nobody → dougt
Attachment #8408462 - Flags: review?(dougt)
Comment on attachment 8408462 [details] [diff] [review]
bug996998_mls_overriding_gps.diff

Review of attachment 8408462 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +675,5 @@
>    coords->GetLongitude(&lon);
>    coords->GetAccuracy(&acc);
> +  
> +  double oldLat, oldLon;
> +  mLastMLSPosition->GetLatitude(&oldLat);

The first time through this code, mLastMLSPosition will be null and crash.  Right?

::: dom/system/gonk/GonkGPSGeolocationProvider.h
@@ +115,5 @@
>    nsCOMPtr<nsIThread> mInitThread;
>    nsCOMPtr<nsIGeolocationProvider> mNetworkLocationProvider;
>  
> +  nsCOMPtr<nsIDOMGeoPositionCoords> mLastMLSPosition;
> +  

nit: delete all whitespace on new lines.
Attachment #8408462 - Flags: review?(dougt) → review-
Thanks Doug, indeed the nsCOMPtr mLastMLSPosition would have been null first time through. [Headsmack]. Fixed whitespace, removed whitespace on blank lines.
Attachment #8409671 - Flags: review?(dougt)
Attachment #8409671 - Flags: review?(dougt) → review+
Keywords: checkin-needed
Comment on attachment 8408462 [details] [diff] [review]
bug996998_mls_overriding_gps.diff

># HG changeset patch
># Parent c55dfb01a02757b15d5c5d1f2bfec4310d0232fc
># User Garvan Keeley <gkeeley@mozilla.com>
>Bug 996998 - GonkGPSGeolocationProvider::NetworkLocationUpdate::Update() MLS location can override GPS location, even through the MLS location is unchanged
>
>
>diff --git a/dom/system/gonk/GonkGPSGeolocationProvider.cpp b/dom/system/gonk/GonkGPSGeolocationProvider.cpp
>--- a/dom/system/gonk/GonkGPSGeolocationProvider.cpp
>+++ b/dom/system/gonk/GonkGPSGeolocationProvider.cpp
>@@ -664,28 +664,54 @@ GonkGPSGeolocationProvider::NetworkLocat
>   nsRefPtr<GonkGPSGeolocationProvider> provider =
>     GonkGPSGeolocationProvider::GetSingleton();
> 
>   nsCOMPtr<nsIDOMGeoPositionCoords> coords;
>   position->GetCoords(getter_AddRefs(coords));
>   if (!coords) {
>     return NS_ERROR_FAILURE;
>   }
>-
>-  // if we haven't seen anything from the GPS device for 1s,
>-  // use this network derived location.
>-  int64_t diff = PR_Now() - provider->mLastGPSDerivedLocationTime;
>-  if (provider->mLocationCallback && diff > kDefaultPeriod) {
>-    provider->mLocationCallback->Update(position);
>-  }
>-
>+  
>   double lat, lon, acc;
>   coords->GetLatitude(&lat);
>   coords->GetLongitude(&lon);
>   coords->GetAccuracy(&acc);
>+  
>+  double oldLat, oldLon;
>+  mLastMLSPosition->GetLatitude(&oldLat);
>+  mLastMLSPosition->GetLongitude(&oldLon);
>+  
>+  // Use spherical law of cosines to calculate difference
>+  // Not quite as correct as the Haversine but simpler and cheaper
>+  // Should the following be a utility function? Others might need this calc.
>+  double radsInDeg = 3.14159265 / 180.0;
>+  double rNewLat = newLat * radsInDeg;
>+  double rNewLon = newLon * radsInDeg;
>+  double rOldLat = oldLat * radsInDeg;
>+  double rOldLon = oldLon * radsInDeg;
>+  // WGS84 equatorial radius of earth = 6378137m
>+  double delta = acos( (sin(rNewLat) * sin(rOldLat)) +
>+                       (cos(rNewLat) * cos(rOldLat) * cos(rOldLon - rNewLon)) )
>+                      * 6378137;
>+  
>+  // if the MLS coord change is smaller than this arbitrarily small value
>+  // assume the MLS coord is unchanged, and stick with the GPS location
>+  const double kMinMLSCoordChangeInMeters = 10;
>+  
>+  // if we haven't seen anything from the GPS device for 1s,
>+  // use this network derived location.
>+  int64_t diff = PR_Now() - provider->mLastGPSDerivedLocationTime;
>+  if (provider->mLocationCallback && diff > kDefaultPeriod
>+      && delta > kMinMLSCoordChangeInMeters)
>+  {
>+    provider->mLocationCallback->Update(position);
>+  }
>+
>+  mLastMLSPosition = position;
>+  
>   provider->InjectLocation(lat, lon, acc);
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> GonkGPSGeolocationProvider::NetworkLocationUpdate::LocationUpdatePending()
> {
>   return NS_OK;
>diff --git a/dom/system/gonk/GonkGPSGeolocationProvider.h b/dom/system/gonk/GonkGPSGeolocationProvider.h
>--- a/dom/system/gonk/GonkGPSGeolocationProvider.h
>+++ b/dom/system/gonk/GonkGPSGeolocationProvider.h
>@@ -110,16 +110,18 @@ private:
>   const AGpsRilInterface* mAGpsRilInterface;
>   nsCOMPtr<nsIRadioInterface> mRadioInterface;
> #endif
>   nsCOMPtr<nsIGeolocationUpdate> mLocationCallback;
>   PRTime mLastGPSDerivedLocationTime;
>   nsCOMPtr<nsIThread> mInitThread;
>   nsCOMPtr<nsIGeolocationProvider> mNetworkLocationProvider;
> 
>+  nsCOMPtr<nsIDOMGeoPositionCoords> mLastMLSPosition;
>+  
>   class NetworkLocationUpdate : public nsIGeolocationUpdate
>   {
>     public:
>       NS_DECL_ISUPPORTS
>       NS_DECL_NSIGEOLOCATIONUPDATE
> 
>       NetworkLocationUpdate() {}
>
Attachment #8408462 - Attachment is obsolete: true
Assignee: dougt → gkeeley
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa0d64e544e

Just a friendly reminder that your commit message should be summarizing what the patch is doing, not restating the problem it's fixing. Thanks!
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
Yikes my first patch has not gone smoothly, thanks for your patience guys.

GonkGPSGeolocationProvider.o built ok now:
https://tbpl.mozilla.org/php/getParsedLog.php?id=38609612&tree=Try&full=1
Attachment #8409671 - Attachment is obsolete: true
Attachment #8413730 - Flags: review?(dougt)
Comment on attachment 8413730 [details] [diff] [review]
bug996998_mls_overriding_gps.diff

Garvan, does this handle the case where the GPS stop responding after a bit of time and we need to switch back to MLS?
blocking-b2g: 2.0? → 2.0+
I'm adding qawanted to see if we can reproduce this on Tarako 1.3T.
Keywords: qawanted
(In reply to Doug Turner (:dougt) from comment #8)
I am going to wait to comment further, I'll be getting a device any day now to look at this bug hands-on, and I'd like to validate the original bug, and your question.

comment #9, as long as that device has a GPS chip.
QA Contact: mclemmons
(In reply to Jason Smith [:jsmith] from comment #9)
> I'm adding qawanted to see if we can reproduce this on Tarako 1.3T.

Following the STR from Comment 0 on the latest 1.3T build, user is able to witness Geolocation jumping. The jumps are roughly 5 miles apart (8km) with the device at the Kirkland, WA (USA) location. Repro Rate = 5/5

Device: Tarako 1.3T MOZ
BuildID: 20140507014006
Gaia: 25a17d9d7143977ea9a81ed310098e326609d248
Gecko: 68a2f24960d2
Version: 28.1
Firmware Version: sp6821a-gonk4.0-4-29
Keywords: qawanted
Sounds like this we need this for Tarako then.
blocking-b2g: 2.0+ → 1.3T?
Tarako doesn't have a GPS chip if I'm not mistaken.
Flags: needinfo?(gkeeley)
Tarako devices have no GPS, unless that has been recently introduced. 

Which would mean the bug is either not in this block of code (which as written would have had a problem of the network derived location overriding the GPS location), or there are 2 bugs.

I'll soon have the devices I need in-hand to debug this properly.
Flags: needinfo?(gkeeley)
this isn't needed for the tarako.
blocking-b2g: 1.3T? → ---
garvan, does this compile on all platforms (you pushed to try?)
(In reply to Doug Turner (:dougt) from comment #15)
> this isn't needed for the tarako.

Then why are we seeing the same bug on tarako then?
Flags: needinfo?(dougt)
Geolocation jumping can be the result of more than one thing. There is the original case in this bug, where the user correlated the jump with his GPS location and the network location, and we could see in the code that these can indeed clash.

There are other possibilities, one such possibility is the hardware producing different data for WiFi signals and cell strengths on consecutive scans, then MLS returning different location values. 

I agree with Doug that the Tarako bug should be opened as a separate issue.
Flags: needinfo?(dougt)
(In reply to Doug Turner (:dougt) from comment #16)
> garvan, does this compile on all platforms (you pushed to try?)

Yes. I was unnecessarily selective in my previous try results in comment #7, on all platforms I have:

https://tbpl.mozilla.org/?tree=Try&rev=55ca35b70697
Once completed, builds and logs will be available at:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/gkeeley@mozilla.com-55ca35b70697
Can you open a separate bug for the Tarako-specific issue here?
Flags: needinfo?(mclemmons)
(In reply to Jason Smith [:jsmith] from comment #20)
> Can you open a separate bug for the Tarako-specific issue here?

Please see Bug 1007953
Flags: needinfo?(mclemmons)
I can reproduce this on the Flame ,running master/m-c.  I've repro'd this to Garvan today, and he confirms his patch will hopefully fix this when it lands on m-c.

for reference STRs:
1) install m-c/master nightly on Flame

Gaia      15ac34804eb8b3c9b2582d7cf754c57e23182df6
Gecko     https://hg.mozilla.org/mozilla-central/rev/cf89b5d018f8
BuildID   20140509040202
Version   32.0a1
ro.build.version.incremental=87
ro.build.date=Mon May  5 20:19:07 CST 2014

2) turn on wifi, but not data
3) launch a browser, and goto html5demos.com/geo
4) at first, it retrieves my location to be about 5k away from where i am
5) hit refresh on the browser
6) second time, it geolocates me to be exactly where i am.

So the 5k jumping can be reproduced 100% with this test site.  unfortunately, i have no way of providing logging on device since setting "geo.wifi.logging.enabled=true" does not show anything in logcat except:

05-09 11:14:06.729: I/Gecko(2277): *** WIFI GEO: startup called.
Attachment #8413730 - Flags: review?(dougt) → review+
What are we waiting for to land this ? It's r+ for one week.
blocking-b2g: --- → 2.0?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5923d4b52fb9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
blocking-b2g: 2.0? → 2.0+
Blocks: 1026115
You need to log in before you can comment on or make changes to this bug.