732923 - Timeout is always being triggered on watchPosition (geolocation)

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect, major)

Description

[Rafael Coutinho]

Attachment: FFWatchIssue.txt

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

I'm trying to watch user location with a timeout.



Actual results:

Although it returns the correct user location, it will always fail on timeout. 


Expected results:

Timeout the watch request if it was trying to retrieve the user location and takes too long.

Comment 1

[brewster]

after a successful position is returned, the FF implementation of the geolocation spec should watch for position changes but instead fires a TIMEOUT. it seems as though the the timer is not being cancelled and reset after a successful position acquisition so it times out (and may/or may not be halting system events to watch for further changes in position).

pastie link is to a JS sample to reproduce the issue.  it returns SUCCESS upon finding the initial position, and then 10 seconds later (as specified by the timeout option) returns ERROR. no further attempts for a position are made (that i saw).

http://pastie.org/3695489

Comment 2

[Josh Matthews [:jdm]]

Attachment: Testcase

This is not good. Thanks for the clear steps to reproduce!

Comment 3

[Josh Matthews [:jdm]]

Attachment: Testcase

Comment 4

[Josh Matthews [:jdm]]

The problem is that we simply reset the timer as soon as we deliver a new position, instead of following the spec: "Cancel the pending timer. Note that the timer must be restarted once this algorithm jumps back to the beginning of the acquisition steps." The acquisition steps in this case refer to the steps that are taken after an has been received from the system indicating that new position data is available.

Comment 5

[mathias]

Is there any update on this issue? I can still replicate the testcase on the latest release 21.0

Comment 6

[Daniel]

It is also reproducible in current Nightly (24.0a1) and on Firefox for Android with GPS disabled.

I have encountered some more odd behavior. The success callback function is periodically called (~4 times i a row) with multiple timeout errors in-between.

Comment 7

[Daniel]

Attachment: Testcase success in intervals

STR
* Open testcase
* Share location when prompted
* Wait ~3 min

Comment 8

[Daniel]

Attachment: Example result for attachment 763090

Comment 9

[Guilherme Gonçalves [:ggp]]

Attachment: 2 - Make watchPosition timeouts adhere to spec (WIP).

Tentative patch. This adds a Changed() method to nsIGeolocationUpdate that
should get called by the provider whenever its state changes, indicating that
a new position might be available. We can then use this to set the timeout.

For the Wifi provider, it looks easy enough to just call Change() whenever the
set of visible access points change. I'm not so sure what to do with the other
providers, though: CoreLocation and Android don't seem to expose the kind of
information we need, but Gonk might be promising.

Thus the way the patch currently stands, the Wifi provider can cause timeouts
if the server takes too long to reply, but other providers will never timeout,
only provide new positions when available.

All tests pass locally with one single issue so far: on optimized builds only,
test_windowClose.html fails if it's run more than once.

Comment 10

[Guilherme Gonçalves [:ggp]]

Attachment: 1 - CoreLocation provider shouldn't implement nsIGeolocationUpdate

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 8355679 [details] [diff] [review]
2 - Make watchPosition timeouts adhere to spec (WIP).

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

This looks pretty good to me. I don't have a problem with some geolocation providers never causing a timeout after the initial position search.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +466,5 @@
>  
> +    if (!mIsWatchPositionRequest) {
> +      SetTimeoutTimer();
> +    }
> +  }

This doesn't look right. I think we do want the first timeout to be relative to the initial request, regardless of whether we're watching or not.

::: xpcom/system/nsIGeolocationProvider.idl
@@ +31,5 @@
> +   * Notify the geolocation service that the location has
> +   * potentially changed, and thus a new position is in the
> +   * process of being acquired.
> +   */
> +  void changed();

I would call this locationUpdatePending instead.

Comment 12

[Guilherme Gonçalves [:ggp]]

Attachment: 2 - Make watchPosition timeouts adhere to spec.

This also disables a couple of tests that depend on the network
provider on Android. These tests were passing by accident before
this patch (and started failing afterwards), and were never meant
to run on Android anyway.

Comment 13

[Guilherme Gonçalves [:ggp]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=fb1e0c63c012

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a13c7459626
https://hg.mozilla.org/integration/mozilla-inbound/rev/44ad28c2159e

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a13c7459626
https://hg.mozilla.org/mozilla-central/rev/44ad28c2159e

Comment 16

[Paul Silaghi, QA [:pauly]]

29.0a1 (2014-01-13), Win 7 x64, cable connection
Open the testcase -> SUCCESS
Reload -> ERROR

Thoughts?

Comment 17

[Josh Matthews [:jdm]]

I can't reproduce that result. I'd be interested in seeing your browser console output when you have the geo.wifi.logging.enabled pref turned on.

Comment 18

[Paul Silaghi, QA [:pauly]]

(In reply to Josh Matthews [:jdm] from comment #17)
> I can't reproduce that result. 
How about if you open both testcases in 2 tabs and reload them?

> I'd be interested in seeing your browser
> console output when you have the geo.wifi.logging.enabled pref turned on.
I don't have that pref. Should I create it manually?

Comment 19

[Josh Matthews [:jdm]]

No change for me. Yes, you should create it.

Comment 20

[Paul Silaghi, QA [:pauly]]

Attachment: console log.txt

reloading couple of times the first testcase

Comment 21

[Josh Matthews [:jdm]]

>*** WIFI GEO: wifi error: 2147746065

You have some problem with the code that watches for wifi networks, and this intentionally propagates to the content geolocation error handler. Nothing we can do here.

Comment 22

[Paul Silaghi, QA [:pauly]]

Probably because this computer doesn't have wifi. Couldn't be this related to https://bugzilla.mozilla.org/show_bug.cgi?id=923618#c16 ?

Comment 23

[Josh Matthews [:jdm]]

It's possible, yes.

Comment 24

[Paul Silaghi, QA [:pauly]]

Confirmed it's not repro with wifi on.
Does it worth to file a new bug?

Comment 25

[Josh Matthews [:jdm]]

Sure.

Comment 26

[Paul Silaghi, QA [:pauly]]

Filed bug 959987.
Marking this one as verified fixed FF 29.0a1 (2014-01-14).