Closed Bug 645325 Opened 9 years ago Closed Last year

Geolocation api returns NaN for speed value and probably 0 instead of null

Categories

(Core :: DOM: Geolocation, defect, P3, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: mail, Assigned: cpeterson)

References

()

Details

(Whiteboard: [W3C GEO SPEC])

Attachments

(7 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16

Firefox's geolocation api returns NaN as speed value. According to http://www.w3.org/TR/geolocation-API/#speed this should be null or a non-negative real number.

Furthermore, altitude and altitudeAccuracy have a value of 0, although probably there is no altitude information available, and the value should thus be null.

Also heading seems to return NaN instead of null (NaN should be returned if the device is stationary), see specs.

Reproducible: Always

Steps to Reproduce:
use navigator.geolocation.watchPosition(...) and look at the result sent to the callback function, especially at
location.coords.speed,
location.coords.heading,
location.coords.altitude and
location.coords.altitudeAccuracy
(assuming location is the callback functions parameter).
Actual Results:  
location.coords.speed = NaN
location.coords.heading = NaN
location.coords.altitude = 0
location.coords.altitudeAccuracy = 0

Expected Results:  
location.coords.speed = null
location.coords.heading = null
location.coords.altitude = null
location.coords.altitudeAccuracy = null
Component: General → Geolocation
Product: Firefox → Core
QA Contact: general → geolocation
Are you sure you're not traveling in hyperspace? :)
Could you please attach or link to a test case which displays this bug?
This is still valid, we don't massage the coords values in any way. See also #544877 which wants to keep speed in between the 0-360 number range.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Duplicate of this bug: 857659
Whiteboard: [W3C GEO SPEC]
See Also: → 833025
Blocks: mgga
Firefox still has this bug. I made a spreadsheet comparing the coords returned from different browsers and operating systems:

https://docs.google.com/spreadsheets/d/1Zp0Jwrv2rotJGsnfFUQtgAezi4UxXZFZFvX3hujmeXk/edit#gid=0

Chrome and Safari both appear to sanitize the results returned from their location providers, whereas Firefox returns NaN, -1, and suspicious 0 values where the spec says it shouldn't. dougt closed bug 544877 saying he no longer thinks we should attempt to sanitize the location provider's results, but he didn't explain the rationale. Firefox is returning invalid values, forcing web developers to deal with platform inconsistencies.

The Firefox results from the spreadsheet:

Browser                 OS              accuracy    altitude    altitudeAccuracy    heading speed
=======                 ==              ========    ========    ================    ======= =====
Firefox 58 (Release)    Windows 10      21          0           0                   NaN     NaN
Firefox 58 (Release)    macOS 10.13.3   33          0           0                   NaN     NaN
Firefox 60 (Nightly)    Windows 10      5923        0           0                   0       0
Firefox 60 (Nightly)    macOS 10.13.3   65          36.60520554 10                  -1      -1

To test, I ran the following JS snippet in the devtools console:

navigator.geolocation.getCurrentPosition(pos => { console.log(pos.coords); }, error => { console.log(error); });
Depends on: 544877
Summary: geolocation api returns NaN for speed value and probably 0 instead of null → Geolocation api returns NaN for speed value and probably 0 instead of null
One interesting finding: Firefox Nightly, Safari on macOS, and Safari on iOS all use Apple's CoreLocation API, which supports altitude and altitudeAccuracy but the navigator.geolocation API on Safari on macOS returns null altitude and altitudeAccuracy.

I don't know if this is intentional or an oversight. WebKit has separate GeolocationPosition classes for macOS [1] and iOS [2] (added in WebKit bug [3]) and the only difference between these classes is that the iOS one initializes altitude, altitudeAccuracy, heading, speed, and (non-standard) floorLevel. CoreLocation's floor level property is not available on macOS.

[1] https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/geolocation/GeolocationPosition.h
[2] https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/geolocation/ios/GeolocationPositionIOS.mm
[3] https://bugs.webkit.org/show_bug.cgi?id=178173
Assignee: nobody → cpeterson
Priority: -- → P3
Garvan, Josh Matthews, the current geolocation module owner, is not accepting reviews while he is away. In the meantime, do you mind doing a first review pass, since you were the last person to touch a lot of this code? :)
Flags: needinfo?(gkeeley)
I don't mind, skimming it now, your patch is very nicely broken up it should be easy enough to review.
Flags: needinfo?(gkeeley)
Comment on attachment 8966845 [details]
Bug 645325 - Part 7: Always check location results in geolocation tests' success callbacks.

https://reviewboard.mozilla.org/r/235524/#review244684

::: dom/tests/mochitest/geolocation/test_clearWatch.html:51
(Diff revision 1)
>      SimpleTest.executeSoon(clearWatch);
>      firstCallback = false;
>    }
>  }
>  
> +let watchID;

Please verify this should be here.
Attachment #8966845 - Flags: review+
Comment on attachment 8966844 [details]
Bug 645325 - Part 6: Disable geolocation request cache when testing different location providers.

https://reviewboard.mozilla.org/r/235522/#review244688

::: dom/tests/mochitest/geolocation/test_worseAccuracyDoesNotBlockCallback.html:37
(Diff revision 1)
>    SimpleTest.finish();
>  }
>  
>  function successCallback1(position) {
>    check_geolocation(position);
> +  is(position.coords.accuracy, 42, "high accuracy");

Consider making these constants part of geolocation_common (or comment how they originate from the .sjs). [line 31 above also]
Attachment #8966844 - Flags: review+
Comment on attachment 8966843 [details]
Bug 645325 - Part 5: Add stricter type and range checks for JavaScript Coordinates properties.

https://reviewboard.mozilla.org/r/235520/#review244690
Attachment #8966843 - Flags: review+
Comment on attachment 8966841 [details]
Bug 645325 - Part 3: Remove nsGeoPosition's unused `long long aTimestamp` constructor.

https://reviewboard.mozilla.org/r/235516/#review244694
Attachment #8966841 - Flags: review+
Comment on attachment 8966839 [details]
Bug 645325 - Part 1: Use NaN to indicate unset optional coordinate values returned from the location providers.

https://reviewboard.mozilla.org/r/235512/#review244702

::: dom/geolocation/nsGeoPosition.cpp:44
(Diff revision 1)
>    , mAlt(aAlt)
> -  , mHError(aHError)
> -  , mVError(aVError)
> -  , mHeading(aHeading)
> -  , mSpeed(aSpeed)
> -{
> +  , mHError((aHError >= 0) ? aHError : 0)
> +    // altitudeAccuracy without an altitude doesn't make any sense.
> +  , mVError((aVError >= 0 && !IsNaN(aAlt)) ? aVError : NaN())
> +    // If the hosting device is stationary (i.e. the value of the speed attribute
> +    // is 0), then the value of the heading attribute must be NaN (or null).

Nice! This will ensure some future geolocation provider isn't abusing heading for providing stationary orientation.
Attachment #8966839 - Flags: review+
Comment on attachment 8966842 [details]
Bug 645325 - Part 4: Handle null listener when xhr callbacks are called after WifiGeoPositionProvider shutdown.

https://reviewboard.mozilla.org/r/235518/#review244692

Great that you caught (and fixed) this case!
Attachment #8966842 - Flags: review+
Comment on attachment 8966840 [details]
Bug 645325 - Part 2: Remove the two-second startup delay for the MLS fallback provider on Windows and macOS.

Marking for review so I remember to come back to this. This patch requires me reading a bit more of the old code to recall the details of how it works.
Attachment #8966840 - Flags: review?(gkeeley)
(In reply to :garvan from comment #16)
> > +let watchID;
> 
> Please verify this should be here.

Yes. Declaring the watchID variable was necessary after I added "use strict" to this test script. Previously watchID was used without a declaration and thus became a property of the JS global object. That worked fine in this case, but it's bad style.
Comment on attachment 8966840 [details]
Bug 645325 - Part 2: Remove the two-second startup delay for the MLS fallback provider on Windows and macOS.

https://reviewboard.mozilla.org/r/235514/#review244796

LGTM! Good that you ensured changes were non-breaking to GPSD provider code (when I read the code without the commit comment at first I was wondering why we can't just remove the fallback delay entirely).
Attachment #8966840 - Flags: review?(gkeeley) → review+
Comment on attachment 8966845 [details]
Bug 645325 - Part 7: Always check location results in geolocation tests' success callbacks.

https://reviewboard.mozilla.org/r/235524/#review253618
Attachment #8966845 - Flags: review?(josh) → review+
Comment on attachment 8966844 [details]
Bug 645325 - Part 6: Disable geolocation request cache when testing different location providers.

https://reviewboard.mozilla.org/r/235522/#review253616
Attachment #8966844 - Flags: review?(josh) → review+
Comment on attachment 8966843 [details]
Bug 645325 - Part 5: Add stricter type and range checks for JavaScript Coordinates properties.

https://reviewboard.mozilla.org/r/235520/#review253612

::: dom/tests/mochitest/geolocation/geolocation_common.js:74
(Diff revision 2)
> +function check_geolocation(location)
> +{
>    ok(location, "Check to see if this location is non-null");
>  
> -  ok("timestamp" in location, "Check to see if there is a timestamp");
> +  const timestamp = location.timestamp;
> +  dump(`timestamp=$timestamp}\n`);

Let's remove this dump call.

::: dom/tests/mochitest/geolocation/geolocation_common.js:91
(Diff revision 2)
> +  dump(`longitude=${longitude}\n`);
> +  dump(`accuracy=${accuracy}\n`);
> +  dump(`altitude=${altitude}\n`);
> +  dump(`altitudeAccuracy=${altitudeAccuracy}\n`);
> +  dump(`speed=${speed}\n`);
> +  dump(`heading=${heading}\n`);

And these calls.
Attachment #8966843 - Flags: review?(josh) → review+
Comment on attachment 8966842 [details]
Bug 645325 - Part 4: Handle null listener when xhr callbacks are called after WifiGeoPositionProvider shutdown.

https://reviewboard.mozilla.org/r/235518/#review253610
Attachment #8966842 - Flags: review?(josh) → review+
Comment on attachment 8966840 [details]
Bug 645325 - Part 2: Remove the two-second startup delay for the MLS fallback provider on Windows and macOS.

https://reviewboard.mozilla.org/r/235514/#review253606
Attachment #8966840 - Flags: review?(josh) → review+
Comment on attachment 8966839 [details]
Bug 645325 - Part 1: Use NaN to indicate unset optional coordinate values returned from the location providers.

https://reviewboard.mozilla.org/r/235512/#review253604
Attachment #8966839 - Flags: review?(josh) → review+
Comment on attachment 8966841 [details]
Bug 645325 - Part 3: Remove nsGeoPosition's unused `long long aTimestamp` constructor.

https://reviewboard.mozilla.org/r/235516/#review253608
Attachment #8966841 - Flags: review?(josh) → review+
(In reply to Josh Matthews [:jdm] from comment #34)
> > -  ok("timestamp" in location, "Check to see if there is a timestamp");
> > +  const timestamp = location.timestamp;
> > +  dump(`timestamp=$timestamp}\n`);
> 
> Let's remove this dump call.
> 
> ::: dom/tests/mochitest/geolocation/geolocation_common.js:91
> (Diff revision 2)
> > +  dump(`longitude=${longitude}\n`);
> > +  dump(`accuracy=${accuracy}\n`);
> > +  dump(`altitude=${altitude}\n`);
> > +  dump(`altitudeAccuracy=${altitudeAccuracy}\n`);
> > +  dump(`speed=${speed}\n`);
> > +  dump(`heading=${heading}\n`);
> 
> And these calls.

OK. My thinking was that the dumped details would be useful for debugging why a location provider failed on the test machine, since the failures might be hard to reproduce. Then again, these tests are unlikely to fail because the test location provider uses hard coded values.
That's a reasonable reason for keeping them.
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5472e604fa
Part 1: Use NaN to indicate unset optional coordinate values returned from the location providers. r=garvank r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/814c7e1c16e5
Part 2: Remove the two-second startup delay for the MLS fallback provider on Windows and macOS. r=garvank r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e4da56b11b
Part 3: Remove nsGeoPosition's unused `long long aTimestamp` constructor. r=garvank r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/7482eb372d94
Part 4: Handle null listener when xhr callbacks are called after WifiGeoPositionProvider shutdown. r=garvank r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0cd7366b1a2
Part 5: Add stricter type and range checks for JavaScript Coordinates properties. r=garvank r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/5759d2ab032b
Part 6: Disable geolocation request cache when testing different location providers. r=garvank r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8a87c6a518
Part 7: Always check location results in geolocation tests' success callbacks. r=garvank r=jdm
Sheriff: these patches touch some tests, so the TV test verification job runs. But one of theses tests has a preexisting intermittent failure (bug 1439042) that reproduces during the TV job. It's not my fault! :)
You need to log in before you can comment on or make changes to this bug.