Closed
Bug 732923
Opened 13 years ago
Closed 11 years ago
Timeout is always being triggered on watchPosition (geolocation)
Categories
(Core :: DOM: Geolocation, defect)
Tracking
()
VERIFIED
FIXED
mozilla29
People
(Reporter: rafael.coutinho, Assigned: jdm)
References
Details
Attachments
(7 files, 2 obsolete files)
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.
Reporter | ||
Updated•13 years ago
|
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
Updated•13 years ago
|
Component: Untriaged → Geolocation
Product: Firefox → Core
QA Contact: untriaged → geolocation
Version: 9 Branch → Trunk
Assignee | ||
Comment 2•13 years ago
|
||
This is not good. Thanks for the clear steps to reproduce!
Assignee: nobody → josh
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #611222 -
Attachment is obsolete: true
Assignee | ||
Comment 4•13 years ago
|
||
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.
Attachment #763090 -
Attachment mime type: text/plain → text/html
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
Assignee | ||
Comment 11•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Attachment #8355680 -
Flags: review+
Comment 12•12 years ago
|
||
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)
Comment 13•12 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8357106 -
Flags: review?(josh) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a13c7459626
https://hg.mozilla.org/integration/mozilla-inbound/rev/44ad28c2159e
Keywords: checkin-needed
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1a13c7459626
https://hg.mozilla.org/mozilla-central/rev/44ad28c2159e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 16•11 years ago
|
||
29.0a1 (2014-01-13), Win 7 x64, cable connection
Open the testcase -> SUCCESS
Reload -> ERROR
Thoughts?
Flags: needinfo?
Assignee | ||
Comment 17•11 years ago
|
||
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?
Comment 18•11 years ago
|
||
(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?
Assignee | ||
Comment 19•11 years ago
|
||
No change for me. Yes, you should create it.
Comment 20•11 years ago
|
||
reloading couple of times the first testcase
Assignee | ||
Comment 21•11 years ago
|
||
>*** 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•11 years ago
|
||
Probably because this computer doesn't have wifi. Couldn't be this related to https://bugzilla.mozilla.org/show_bug.cgi?id=923618#c16 ?
Assignee | ||
Comment 23•11 years ago
|
||
It's possible, yes.
Comment 24•11 years ago
|
||
Confirmed it's not repro with wifi on.
Does it worth to file a new bug?
Assignee | ||
Comment 25•11 years ago
|
||
Sure.
Comment 26•11 years ago
|
||
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.
Description
•