1035044 - nsGeolocation: For checking if location is cached in JS, support timestamp check *not* object-equality

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[:garvan]

Attachment: test_cachedPosition.html

+++ This bug was initially created as a clone of Bug #1013012 +++

Remove unnecessary additional cached location from nsGeolocation class -it is not a true cached location, that would be on the service level only, nsGeolocation has page lifespan, the service has a longer lifespan.

This function was there to satisfy a particular interpretation of the W3C geolocation spec. [*** See below ***]
My interpretation of this spec is that if a user wants to check if a consecutive position is a cached position, they would compare the timestamp of the previous to the current position. If they are equal, the position is cached. 
The alternate interpretation, which the code was written to, is that to check if a consecutive position is cached, is to compare the Position object by address-of equality. I don't think the spec means to say that, but more importantly, I don't think users of the JS geolocation API are going to expect or rely on that behaviour.

*** "If a cached Position object, whose age is no greater than the value of the maximumAge variable, is available, invoke the successCallback with the cached Position object as a parameter"

A useful browser test is this file: test_cachedPosition.html (attached)

This is a test of the W3C spec. Check if a position is cached by comparing it to a previous position, using address-of equality and timestamp equality.

Results on Chrome Version 35.0.1916.153 and Safari 7.0.4:
- address-of equality not supported
- timestamp equality not supported

Neither approach seems supported ATM :).

Comment 1

[:garvan]

Attachment: bug1035044.patch

1) Remove the extra cached JS wrapper position from nsGeolocation.
2) Update the mochitest to test for timestamp, not object equality.

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 8451607 [details] [diff] [review]
bug1035044.patch

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

::: dom/src/geolocation/nsGeolocation.cpp
@@ +505,5 @@
>      // Ignore SendLocationEvents issued before we were cleared.
>      return;
>    }
>  
> +  if (!aPosition) {

Cut this block and put the next one in an if (aPosition) instead, please.

::: dom/tests/mochitest/geolocation/test_cachedPosition.html
@@ +51,5 @@
> +      cached = pos;
> +
> +      navigator.geolocation.getCurrentPosition(function(pos) {
> +        ok(areTimesEqual(pos, cached), "position should be equal to cached position");
> +        set_network_request_cache_enabled(false, function() {

Why is this necessary? If the request cache doesn't respect maximumAge, that's a serious problem.

Comment 3

[:garvan]

Attachment: bug1035044-mochitest.patch

Updating per josh's review. More to follow.

Also, I am splitting between mochitest and cpp changes, for my own convenience, please advise if I shouldn't do that.

Comment 4

[:garvan]

> > +      navigator.geolocation.getCurrentPosition(function(pos) {
> > +        ok(areTimesEqual(pos, cached), "position should be equal to cached position");
> > +        set_network_request_cache_enabled(false, function() {
> 
> Why is this necessary? If the request cache doesn't respect maximumAge,
> that's a serious problem.

The PositionOptions maximumAge is a flag as to whether the GeolocationService uses its cache, or queries its provider. The providers should never need the PositionOptions, they are a black box to the GeolocationService level. (Maybe that is too strong, QC wants the high accuracy flag passed in to their GPS provider as they can set the gps chip accordingly.)

I will add comments to the mochitest for the reader, updating now.

More thoughts on the network cache introduced in 1018383:
The word 'cache' is heavily used in our code, and is confusing. The network request cache is like an MLS proxy. A plain network request cache would just hash the request string and lookup previous matching requests (and consider staleness also). This network request cache differs from that model in that it stores only one cached object, has fuzzy lookup logic instead of a hash on the URI (+POST args), AND it has some MLS logic in regards to accuracy based on the amount of info in the request. 

I am open to name change suggestions :).

Comment 5

[:garvan]

Attachment: bug1035044-mochitest.patch

Added comments to explain why the network request cache gets shut off.

Comment 6

[Josh Matthews [:jdm]]

(In reply to Garvan Keeley [:garvank] from comment #4)
> > > +      navigator.geolocation.getCurrentPosition(function(pos) {
> > > +        ok(areTimesEqual(pos, cached), "position should be equal to cached position");
> > > +        set_network_request_cache_enabled(false, function() {
> > 
> > Why is this necessary? If the request cache doesn't respect maximumAge,
> > that's a serious problem.
> 
> The PositionOptions maximumAge is a flag as to whether the
> GeolocationService uses its cache, or queries its provider. The providers
> should never need the PositionOptions, they are a black box to the
> GeolocationService level. (Maybe that is too strong, QC wants the high
> accuracy flag passed in to their GPS provider as they can set the gps chip
> accordingly.)
> 
> I will add comments to the mochitest for the reader, updating now.
> 
> More thoughts on the network cache introduced in 1018383:
> The word 'cache' is heavily used in our code, and is confusing. The network
> request cache is like an MLS proxy. A plain network request cache would just
> hash the request string and lookup previous matching requests (and consider
> staleness also). This network request cache differs from that model in that
> it stores only one cached object, has fuzzy lookup logic instead of a hash
> on the URI (+POST args), AND it has some MLS logic in regards to accuracy
> based on the amount of info in the request. 
> 
> I am open to name change suggestions :).

My point is that if we get different behaviour with the request_cache enabled and disabled when maximumAge is 0, that sounds like a bug to me. maximumAge specifies that the result returned must be no older than the provided age; in particular, the spec calls out providing a maximumAge of 0 as the way to force an up to date position.

Comment 7

[:garvan]

> My point is that if we get different behaviour with the request_cache
> enabled and disabled when maximumAge is 0, that sounds like a bug to me.
> maximumAge specifies that the result returned must be no older than the
> provided age; in particular, the spec calls out providing a maximumAge of 0
> as the way to force an up to date position.

Another approach to explaining the problem in the mochitest. A test of cached position should look like this:

mochitest -> get location
GeolocationService -> call NetworkGeolocationProvider
NetworkGeolocationProvider -> geoip scan
mochtest -> gets location, saves it

NetworkGeolocationProvider -> detailed scan (cell for example)

mochitest -> get location, maximumAge 2 weeks
GeolocationService -> does not call NetworkGeolocationProvider
mochitest -> get location, same as previous

mochitest -> get location, maximumAge zero
GeolocationService -> call NetworkGeolocationProvider
NetworkGeolocationProvider -> detailed scan, or no scan, doesn't matter it will give new location
mochtest -> gets location, not same as previous

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 8453894 [details] [diff] [review]
bug1035044-mochitest.patch

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

This looks fine after further reflection and discussion.

Comment 9

[:garvan]

Attachment: bug1035044_cpp.patch

Cleaned up the if-statement as per Josh's review.

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8453961 [details] [diff] [review]
bug1035044_cpp.patch

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

Please clean up the commit message here.

Comment 11

[:garvan]

note to self, when bug 1018383 lands in m-c, patch this on top, do a try run, mark check-in needed.

Comment 12

[:garvan]

https://tbpl.mozilla.org/?tree=Try&rev=1ec06c85a537
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1ec06c85a537

leaving the needinfo so I remember to check back when the try build is done

Comment 13

[:garvan]

try green on the geo mochitests.

Sheriff: Ignore the test_cachedPosition.html

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f168b149300
https://hg.mozilla.org/integration/mozilla-inbound/rev/308a26647b35

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f168b149300
https://hg.mozilla.org/integration/mozilla-inbound/rev/308a26647b35

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f168b149300
https://hg.mozilla.org/mozilla-central/rev/308a26647b35