Closed Bug 732923 Opened 8 years ago Closed 6 years ago

Timeout is always being triggered on watchPosition (geolocation)

Categories

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

x86
Windows XP
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla29

People

(Reporter: rafael.coutinho, Assigned: jdm)

References

Details

Attachments

(7 files, 2 obsolete files)

Attached file 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.
Severity: normal → major
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
Component: Untriaged → Geolocation
Product: Firefox → Core
QA Contact: untriaged → geolocation
Version: 9 Branch → Trunk
Attached file Testcase (obsolete) —
This is not good. Thanks for the clear steps to reproduce!
Assignee: nobody → josh
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file Testcase
Attachment #611222 - Attachment is obsolete: true
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.
Is there any update on this issue? I can still replicate the testcase on the latest release 21.0
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.
STR
* Open testcase
* Share location when prompted
* Wait ~3 min
Attachment #763090 - Attachment mime type: text/plain → text/html
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.
Attachment #8355679 - Flags: feedback?(josh)
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.
Attachment #8355679 - Flags: feedback?(josh) → feedback+
Attachment #8355680 - Flags: review+
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.
Attachment #8355679 - Attachment is obsolete: true
Attachment #8357106 - Flags: review?(josh)
Attachment #8357106 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/1a13c7459626
https://hg.mozilla.org/mozilla-central/rev/44ad28c2159e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
29.0a1 (2014-01-13), Win 7 x64, cable connection
Open the testcase -> SUCCESS
Reload -> ERROR

Thoughts?
Flags: needinfo?
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.
Flags: needinfo?
(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?
No change for me. Yes, you should create it.
Attached file console log.txt
reloading couple of times the first testcase
>*** 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.
Probably because this computer doesn't have wifi. Couldn't be this related to https://bugzilla.mozilla.org/show_bug.cgi?id=923618#c16 ?
It's possible, yes.
Confirmed it's not repro with wifi on.
Does it worth to file a new bug?
Sure.
Blocks: 959987
Filed bug 959987.
Marking this one as verified fixed FF 29.0a1 (2014-01-14).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.