Closed
Bug 996998
Opened 11 years ago
Closed 11 years ago
Geolocation jumping
Categories
(Core :: DOM: Geolocation, defect)
Tracking
()
People
(Reporter: gerard-majax, Assigned: garvan)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Assignee: nobody → dougt
Attachment #8408462 -
Flags: review?(dougt)
Comment 2•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8409671 -
Flags: review?(dougt) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 4•11 years ago
|
||
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
Updated•11 years ago
|
Assignee: dougt → gkeeley
Comment 5•11 years ago
|
||
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
Comment 6•11 years ago
|
||
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 8•11 years ago
|
||
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?
Updated•11 years ago
|
blocking-b2g: 2.0? → 2.0+
Comment 9•11 years ago
|
||
I'm adding qawanted to see if we can reproduce this on Tarako 1.3T.
Keywords: qawanted
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Updated•11 years ago
|
QA Contact: mclemmons
Comment 11•11 years ago
|
||
(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
Tarako doesn't have a GPS chip if I'm not mistaken.
Flags: needinfo?(gkeeley)
Assignee | ||
Comment 14•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
garvan, does this compile on all platforms (you pushed to try?)
Comment 17•11 years ago
|
||
(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)
Assignee | ||
Comment 18•11 years ago
|
||
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)
Assignee | ||
Comment 19•11 years ago
|
||
(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
Comment 20•11 years ago
|
||
Can you open a separate bug for the Tarako-specific issue here?
Flags: needinfo?(mclemmons)
Comment 21•11 years ago
|
||
(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)
Comment 22•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8413730 -
Flags: review?(dougt) → review+
Reporter | ||
Comment 23•11 years ago
|
||
What are we waiting for to land this ? It's r+ for one week.
blocking-b2g: --- → 2.0?
Keywords: checkin-needed
Comment 24•11 years ago
|
||
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•11 years ago
|
blocking-b2g: 2.0? → 2.0+
Updated•11 years ago
|
status-b2g-v2.0:
--- → fixed
Comment 26•10 years ago
|
||
status-b2g-v1.4:
--- → fixed
Updated•10 years ago
|
Blocks: geo-b2g-2.0
You need to log in
before you can comment on or make changes to this bug.
Description
•