Closed
Bug 1283563
Opened 8 years ago
Closed 8 years ago
Both success and error callbacks called if timeout is shorter than few seconds
Categories
(Core :: DOM: Geolocation, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: radek.svitil, Assigned: kanru)
References
Details
(Keywords: regression)
Attachments
(4 files, 3 obsolete files)
1.00 KB,
text/html
|
Details | |
5.68 KB,
patch
|
kanru
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
850 bytes,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
1022 bytes,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36 Steps to reproduce: See code at https://jsfiddle.net/Nadradouch/fbqa8fuh/5/ and run it at Firefox 47 or 47.0.1 (tested on Windows 7 and Android). Actual results: Both success and error callbacks are called. If there is no timeout set or it is set to ca 5-10 seconds and more, only success is called. Expected results: Only one callback should be called.
Comment 2•8 years ago
|
||
I haven't been able to reproduce this yet, no matter what value between 4000 and 10000 the timeout was set to.
Comment 3•8 years ago
|
||
To be clear, I saw some cases where the error callback was called, and some where the success callback was called. I never saw both.
Comment 4•8 years ago
|
||
I can't reproduce, either. Michelangelo, any ideas?
More details on how to reproduce this bug: 1. You MUST use Firefox 47.0.1. It seems earlier versions do not have this problem. 2. You MUST make successful geolocation in your Firefox browser earlies. If there is no earlier position, this script displayes error (it is correct result in that case). If you find geolocate your position e. g. using https://www.google.cz/maps and then go to the https://jsfiddle.net/Nadradouch/fbqa8fuh/5/ , you should at first see SUCCESS and after few seconds ERROR.
Comment 7•8 years ago
|
||
Confirmed on 47 on Win32, Win64, OS X, Linux. Does *NOT* repro on 48 onward. Thanks for letting us know Radek; interestingly enough, this actually happens only on 47. Just to be on the safe side, would you please try to reproduce this with beta? You can download it from here: https://beta.mozilla.org/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(michelangelo) → needinfo?(radek.svitil)
Michelangelo, I am sorry to have a bad news: this error DOES reproduce on 48.0b for me... I also tried our real application, where the problem was originally spotted, and it occurs there too.
STR: 1) Open http://www.w3schools.com/html/html5_geolocation.asp and share location 2) Open https://jsfiddle.net/Nadradouch/fbqa8fuh/5/ Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=953741f757abdb9b12719aba565d82209a46ab7b&tochange=9e99bde8d25469407cb1f86965ca7beacde95737 Doug Turner — Bug 1253159 - Remove locationUpdatePending and restore request timeout. r=jdm I found the issue is only present with e10s disabled, if e10s is on, I can't reproduce it. Doug, what do you think?
Blocks: 1253159
Flags: needinfo?(dougt)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Comment 10•8 years ago
|
||
I found/reproduced this (or something like this) in Firefox v47.0 and in (updated today) v48.0 (running on Windows 10 64-bit). My API call looks like this: navigator.geolocation.getCurrentPosition(success, error, { timeout: 10000, enableHighAccuracy: true }); I call this API repeatedly about once every 4 or 5 seconds (i.e. after it invokes the success or error callback, I call it again about 4 seconds later). Approximately half the times I call it, it fails: the error.code is 3 and the error.message is "Position acquisition timed out". NB that when it fails, it fails immediately, and not after a 10-second timeout. So it looks like it's reporting a earlier (now-expired) timeout which happened after an earlier successful call succeeded.
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
Comment 11•8 years ago
|
||
I found something that looks very strange. I use the following as a test page (and using a PC not a mobile device, no GPS). <!DOCTYPE html> <html> <head> <title>Test Geolocation</title> <meta charset="utf-8" /> </head> <body> <h1>Results</h1> <ul id="list"></ul> <script> var $ul = document.getElementById('list'); function show(text, count) { var $li = document.createElement('li'); $li.textContent = '('+count+') '+text; $ul.appendChild($li); } function testAgain(count) { show('Testing', count) function success() { show('OK', count); testAgain(count + 1); } function error(err) { show('Error ' + err.code + ': ' + err.message, count); testAgain(count + 1); } setTimeout(function () { navigator.geolocation.getCurrentPosition(success, error, { timeout: 10000, enableHighAccuracy: true }); }, 4000); } testAgain(1); </script> </body> </html> Then I get results which are e.g. as follows: Results (1) Testing (1) OK (2) Testing (2) OK (3) Testing (3) OK (4) Testing (4) OK (5) Testing (3) Error 3: Position acquisition timed out (4) Testing (5) OK (6) Testing (4) Error 3: Position acquisition timed out (5) Testing (6) OK (7) Testing (4) OK (5) Testing (5) Error 3: Position acquisition timed out (6) Testing The strange thing is that invocation #3 returned twice: after "(3) Testing" it returned "(3) OK" fairly promptly; after which it started "(4) Testing", and "(5) Testing"; but sometime after that, there was an error return from the #3 invocation i.e. "(3) Error 3: Position acquisition timed out". I was expecting either a success callback or a failure callback; never both, and for sure not a failure callback after a success callback.
Comment 12•8 years ago
|
||
Comment 13•8 years ago
|
||
I tested your testcase and I got the same regression range as in comment #9. Waiting for dougt's answer.
Comment 14•8 years ago
|
||
47 shipped with it & too late for 48.
Assignee | ||
Comment 16•8 years ago
|
||
I cannot reproduce it locally but I suspect it's due to the location cache. I'll take a look.
Assignee: nobody → kchen
Flags: needinfo?(kchen)
Flags: needinfo?(doug.turner)
Comment 17•8 years ago
|
||
I encountered this in 48.0.2, windows 10 64, no GPS hardware. I can reproduce reliably with the test code below: Click the "test" button, wait 5 seconds, then click share in the geolocation permission prompt. The success message appears, followed some time later by the error. I'm not completely sure if time spent with the prompt open makes a difference. <!DOCTYPE html> <html> <head> <title>Geolocation test</title> <script type="text/javascript"> function addStatus(str) { var e=document.getElementById('status'); e.innerHTML = e.innerHTML + '<div>'+str+'</div>'; } function onSuccess(pos) { addStatus("success "+pos.coords.latitude+" "+pos.coords.longitude); } function onError(err) { addStatus("error "+err.code+" "+err.message); } function doTest() { navigator.geolocation.getCurrentPosition(onSuccess,onError,{timeout:5000}); } </script> </head> <body> <button onclick="doTest()">Test</button> <div id="status"></div> </body> </html>
Hi Kan-Ru, could you please take a look at the test case provided in the last comment? Does this help reproduce this locally?
Flags: needinfo?(kchen)
Assignee | ||
Comment 19•8 years ago
|
||
I can't reproduce with a release build. But with a debug build I saw this assertion Assertion failure: !mShutdown (timeout after shutdown), at /home/kanru/mozilla/gecko/dom/geolocation/nsGeolocation.cpp:394 Which means the error callback was called after the success callback and after shutdown.
Flags: needinfo?(kchen)
Assignee | ||
Comment 20•8 years ago
|
||
Okay, patches on the way. But I need to add a test.
Assignee | ||
Comment 21•8 years ago
|
||
The test test_handlerSpinsEventLoop.html should have caught this but it's broken. geo.wifi.timeToWaitBeforeSending was set to 2000 for tests so errorCallback was always called first. In this patch I set geo.wifi.timeToWaitBeforeSending to 10 and timeout to 500 to ensure that successCallback is called before errorCallback. And in successCallback I use sleep(1000) to make sure the next spinEventLoop will run the errorCallback if it's in the event queue.
Attachment #8794151 -
Flags: review?(josh)
Assignee | ||
Comment 22•8 years ago
|
||
Also make sure all timers are consumed.
Attachment #8794151 -
Attachment is obsolete: true
Attachment #8794151 -
Flags: review?(josh)
Attachment #8794152 -
Flags: review?(josh)
Comment 23•8 years ago
|
||
Comment on attachment 8794152 [details] [diff] [review] Don't reset timeout for oneshot geolocation request. v1.1 Review of attachment 8794152 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/geolocation/test_handlerSpinsEventLoop.html @@ +37,4 @@ > successCallbackCalled = true; > + check_geolocation(position); > + sleep(1000); > + while (timeoutPassed) { Shouldn't this be `while (!timeoutPassed)`?
Attachment #8794152 -
Flags: review?(josh) → review+
Assignee | ||
Comment 24•8 years ago
|
||
Argh. Timeout won't fire during sync xhr. I need a better idea to test this.
Comment 25•8 years ago
|
||
What if you use an nsITimer directly, rather than setTimeout?
Assignee | ||
Comment 26•8 years ago
|
||
I changed the test to a mochitest-chrome test in order to use nsITimer directly. If I use mochitest-plain I get a "Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnamedClass" error when timer callback is called.
Attachment #8794152 -
Attachment is obsolete: true
Attachment #8794722 -
Flags: review?(josh)
Updated•8 years ago
|
Attachment #8794722 -
Flags: review?(josh) → review+
Hi Kan-Ru, this seems a recent regression, I'd be happy to take an uplift to Beta50. Is this going to land in Nightly52 soon?
Flags: needinfo?(kchen)
Assignee | ||
Comment 28•8 years ago
|
||
I fixed one more assertion. Now try looks green https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf51e3b441c
Flags: needinfo?(kchen)
Comment 29•8 years ago
|
||
Pushed by kchen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f54d698d7de7 Don't reset timeout for oneshot geolocation request. r=jdm
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f54d698d7de7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 31•8 years ago
|
||
committed version.
Attachment #8794722 -
Attachment is obsolete: true
Attachment #8796493 -
Flags: review+
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8796493 [details] [diff] [review] Don't reset timeout for oneshot geolocation request. v2.1 Approval Request Comment [Feature/regressing bug #]: bug 1253159 [User impact if declined]: unexpected location request timeout [Describe test coverage new/current, TreeHerder]: landed on m-c with tests [Risks and why]: if the patch was wrong, it might break watch request. But the chance is pretty low. [String/UUID change made/needed]:
Attachment #8796493 -
Flags: approval-mozilla-aurora?
Comment 33•8 years ago
|
||
Comment on attachment 8796493 [details] [diff] [review] Don't reset timeout for oneshot geolocation request. v2.1 Fix a regression related to geolocation. Take it in 51 aurora.
Attachment #8796493 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 34•8 years ago
|
||
Hi :kanru, Since this bug also affects 50, do you think it's worth uplifting to 50?
Flags: needinfo?(kchen)
Comment 35•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b684d597878
Flags: in-testsuite+
Comment 36•8 years ago
|
||
Backed out for test_handlerSpinsEventLoop.html timeouts. Does this patch depend on bug 1218627 by chance? https://hg.mozilla.org/releases/mozilla-aurora/rev/9522156a6cda https://treeherder.mozilla.org/logviewer.html#?job_id=3715233&repo=mozilla-aurora
Assignee | ||
Comment 37•8 years ago
|
||
It turns out there are two errors here. First, mochitest/geolocation/chrome.ini was removed from moz.build in bug 1218627 so the test wasn't actually run on nightly. Second, mochitest-chrome test needs a different URL for network_geolocation.sjs, that's why the test timeouts on aurora.
Flags: needinfo?(kchen)
Assignee | ||
Comment 38•8 years ago
|
||
Attachment #8797109 -
Flags: review?(josh)
Assignee | ||
Comment 39•8 years ago
|
||
Attachment #8797110 -
Flags: review?(josh)
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•8 years ago
|
Attachment #8797109 -
Flags: review?(josh) → review+
Comment 40•8 years ago
|
||
Comment on attachment 8797110 [details] [diff] [review] Follow up. Change geo.wifi.url BASE_URL based on test type Review of attachment 8797110 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/geolocation/geolocation_common.js @@ +3,5 @@ > +case "chrome": > + BASE_URL = "http://mochi.test:8888/chrome/dom/tests/mochitest/geolocation/network_geolocation.sjs"; > + break; > +default: > + BASE_URL = "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs"; Let's do var harness = SimpleTest.harnessParameters.testRoot == "chrome" ? "chrome" : "test"; var BASE_URL = "http://mochi.test:8888/" + harness + "/dom/tests/mochitest/geolocation/network_geolocation.sjs";
Attachment #8797110 -
Flags: review?(josh) → review+
Comment 41•8 years ago
|
||
Pushed by kchen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d11cbf8bfd Add mochitest/geolocation/chrome.ini back. r=jdm https://hg.mozilla.org/integration/mozilla-inbound/rev/032113ce311c Change geo.wifi.url BASE_URL based on test type. r=jdm
Assignee | ||
Comment 42•8 years ago
|
||
I'll prepare a patch for Aurora.
Comment 43•8 years ago
|
||
Pushed by kchen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/af72e3cf941f Followup - properly finish test in case timeout is really flaky. r=me
Comment 44•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3d11cbf8bfd https://hg.mozilla.org/mozilla-central/rev/032113ce311c https://hg.mozilla.org/mozilla-central/rev/af72e3cf941f
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 45•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8a93d7f0a67cfef9e0e021ddb8d6eeed2114fd5 https://hg.mozilla.org/releases/mozilla-aurora/rev/1960bbbac430921ccd4178075f974477ae7f707e https://hg.mozilla.org/releases/mozilla-aurora/rev/f161a85e85e45fd2dcc56abbcd4e43c70d177bb5
Comment 48•7 years ago
|
||
pls reopen. still present with v50.0 see url from 1312685 - https://bilgiprog.github.io/jsders/test/geotest.html A working fine in 53.0a1 (2016-11-17) (64-bit)
Comment 49•7 years ago
|
||
(In reply to Peter Nemsak from comment #48) > pls reopen. > > still present with v50.0 > see url from 1312685 - https://bilgiprog.github.io/jsders/test/geotest.html A > > working fine in 53.0a1 (2016-11-17) (64-bit) Read the flags on the right, it's NOT fixed in 50: status-firefox48: wontfix status-firefox49: wontfix status-firefox50: affected status-firefox51: fixed status-firefox52: fixed
Comment 50•7 years ago
|
||
sry, didn't know that it overrides the main bug status - RESOLVED FIXED that is - even for current stable. thanks for notifying.
Too late to fix in 50.1.0 release
Comment 52•5 years ago
|
||
This issue is still reproducible with Beta 67.0b15 on Mac 10.12, but not on 10.13 or 10.14.
You need to log in
before you can comment on or make changes to this bug.
Description
•