716127 - Always sharing Geolocation fails to determine location on every other reload

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Jason Smith [:jsmith]]

Litmus test: [geolocation] always allow sharing location

Steps:

1. Go to http://mozqa.com/data/firefox/geolocation/position.html
2. When prompted to share location, select "always"
3. Reload the page

Expected:

The latitude and longitude for your current geographic location is shown.

Actual:

undefined is outputted to the web page

Additional notes:

Continuous reloads of the page after output will show the geolocation determined every other time. On other times, undefined is outputted.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Avoid shutting down geolocation providers if one-shot updates are pending.

Comment 2

[Josh Matthews [:jdm]]

Thank you for filing this bug, Jason! The testcase does not actually show any incorrect behaviour, it turns out - the geolocation request includes a timeout, and the "undefined" appears when the request times out and attempts to display an undefined error message (I don't believe the message property is actually in the spec). However, if you remove the timeout, the behaviour is still seen - it turns out that the geolocation service shuts down after 6 seconds unless there are outstanding requests, but it only considers watchPosition requests when making that decision.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 586871 [details] [diff] [review]
Avoid shutting down geolocation providers if one-shot updates are pending.

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

This makes sense.  bonus points for adding a mochitest.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Test for overlapping geolocation provider shutdown and unfulfilled request.

Good call. I did a bit of cleaning up in the geolocation service to make this testable.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 586894 [details] [diff] [review]
Test for overlapping geolocation provider shutdown and unfulfilled request.

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

::: dom/src/geolocation/nsGeolocation.cpp
@@ +512,4 @@
>  
>  nsresult nsGeolocationService::Init()
>  {
> +  Preferences::AddIntVarCache(&sProviderTimeout, "geo.timeout", 6000);

use sProviderTimeout instead of 6000.

Comment 6

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e8544d859847
http://hg.mozilla.org/integration/mozilla-inbound/rev/4998e366dd09

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e8544d859847
https://hg.mozilla.org/mozilla-central/rev/4998e366dd09

Comment 9

[Jason Smith [:jsmith]]

Currently broken on Firefox 12 and Nightly 13 with the following test case:

Steps:

1. Go to 1. Go to http://mozqa.com/data/firefox/geolocation/position.html
2. When prompted to share location, select "share"
3. Reload the page
4. When prompted to share location, select "share"

Expected:

The coordinates of my position should be shown.

Actual:

undefined is returned.

See attached log.

Comment 10

[Jason Smith [:jsmith]]

Attachment: Console Log Wifi Geo Share Location Twice Quickly

Comment 11

[Josh Matthews [:jdm]]

Oh, harumph. The automated test works because we just hammer at the URL without waiting for access point data. In real life, however, the patch here did not change the status quo (much). The problem is that we now don't shutdown the service, but we also don't trigger any further network requests until new access point data is received. If the timeout is removed from the testcase, the correct location is eventually displayed, but we should find a better solution.

Comment 12

[Josh Matthews [:jdm]]

Attachment: When the network geolocation provider is told to watch for wifi changes, force a change to occur to avoid indefinite waiting.

This works for me, and makes sense. If we've previously seen a result, there's no reason to expect another one in the near future.

Comment 13

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e485c2c74c4d

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e485c2c74c4d