489813 - fire timeout error if provider fails to response after initial response

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

In case of a watchPosition(), the errorCallback could be invoked repeatedly: the first timeout is relative to the moment watchPosition() was called, while subsequent timeouts are relative to the moment when the implementation determines that the position of the hosting device has changed and a new Position object must be acquired.

Currently we are not firing any timeout errors after the initial response from the geolocation provider.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 3

[Doug Turner (:dougt)]

in order to properly test geolocation, i made the following changes:

1) no longer caching geo.wifi.uri.  This is so that test cases can change the provider URL on the fly without having to shutdown the geolocation service.

2) increase the timeout from 200->750.  There really was not reason that it needs to be faster than this as so long as we are faster than 1s.

3) in the notify() method, we need to always call onChange() while in testing mode.

4) the provider needs to send different positions or our code will ignore the new input from be provider.


The rest of the patch, which address this bug, basically resets the timeout timer when we post a new position to the js context.

Comment 4

[Doug Turner (:dougt)]

we should consider this for 3.6/fennec.  it improves our testings and helps us better conform to the specification (wrt timeouts after a watch() call)

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Would take a reviewed patch, don't think we need to block on it.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 415071 [details] [diff] [review]
patch v.1

 >-  ok (location.coords.latitude  == 37.41857, "lat matches known value");
>-  ok (location.coords.longitude == -122.08769, "lon matches known value");
>-  ok(location.coords.altitude == 42, "alt matches known value");
>-  ok(location.coords.altitudeAccuracy == 42, "alt acc matches known value");
>+  //  ok (location.coords.latitude  == 37.41857, "lat matches known value");
>+  //  ok (location.coords.longitude == -122.08769, "lon matches known value");
>+  //  ok(location.coords.altitude == 42, "alt matches known value");
>+  //  ok(location.coords.altitudeAccuracy == 42, "alt acc matches known value");

So why you comment out these tests? And you don't even add a comment why.

Comment 7

[Doug Turner (:dougt)]

We should just remove these tests.

This change will change the values of these so that we can better test the flow from the network location provider to web content:

+  // We must force a random location change to avoid any filtering that
+  // the platform does.
+
+  coords.latitude  += Math.floor(Math.random()*11);
+  coords.longitude += Math.floor(Math.random()*11);
+  coords.accuracy  += Math.floor(Math.random()*11);

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 415071 [details] [diff] [review]
patch v.1

>+  // We must force a random location change to avoid any filtering that
>+  // the platform does.
>+
>+  coords.latitude  += Math.floor(Math.random()*11);
>+  coords.longitude += Math.floor(Math.random()*11);
>+  coords.accuracy  += Math.floor(Math.random()*11);
>+  

Is this guaranteed to not cause any random test failures?

Comment 9

[Doug Turner (:dougt)]

hmm. ollli, it may.  What I think we have to do is to ensure that there is a significant change such that the platform does fire the "on change" events.  It probably wouldn't be too hard to get the same values over a period of time such that a test run will not see any location change.

What I will have to do is modify the backend to honor some sort of pref that indicates that we should not try to filter any of these location changes.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.2

this ensures that while testing we do not filter the updates.  testing scripts can toggle this feature to test filter if they so desired.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 424036 [details] [diff] [review]
patch v.2

I'm not really fan of adding for-testing-only prefs, but if you can't think of anything better...

Comment 12

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/0542680c6f74

Comment 13

[Phil Ringnalda (:philor)]

You're turning the tree a rather bright shade of orange.

Comment 14

[Doug Turner (:dougt)]

backed out.

Comment 15

[Doug Turner (:dougt)]

looking into this... I removed the change to the timeout value and everything passed try.  I will try pushing this again to m-c soon.

Comment 16

[Doug Turner (:dougt)]

never mind... thought everything was in....

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.3

Short notes:  same patch as before plus a few fixes and a rewrite of the tests.

0) the tests were really broken and they didn't test what they should have been.

1) the location providers can have their startup() called multiple times.  Our network provider implementation didn't realize this.

2) I added some code to avoid calling out to the application prompt/notification.  I did this because it was difficult to predict when the notification box would be available.  Some of the test failures I saw in geo were related to not being able to find the box.  Note -- I didn't disable testing the prompt in the basic cases.  I stopped using the application prompt in test cases where I wanted to hammer on the guts of geolocation.

3) In some cases, the GetCurrentPosition and the WatchPosition APIs could be cause recusions.  For example, if a user clicked "remember this setting", a webpage could call GetCurrentPosition dozens of times and we wouldn't unwind from that.  You could also see this problem any time you use the newly added preference to avoid the application prompt/notification.  I added some event that allow us to avoid this problem.

Pushing to try now....

jst, do you mind reviewing?  Also, please let me know if this is something we want to consider for a 1.9.2 fix (because of the recursion issue).  If if it easier to review I will break this patch apart, but i would rather not take these fixes piecemeal.

Comment 18

[Doug Turner (:dougt)]

(i probably want to change "nsRefPtr<nsGeolocationRequest>&" to "nsGeolocationRequest*" in the event classes.

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 449576 [details] [diff] [review]
patch v.3

chrome xul test is failing.  it looks like an unreferenced variable being used.  Will clean up and repost a new patch.

Comment 20

[Doug Turner (:dougt)]

Attachment: clean up the test error

Comment 21

[Doug Turner (:dougt)]

Attachment: More cleanup.

Olli, you reviewed this a few weeks ago.  I further cleaned up the patch.

One thing I disabled was tests that required us to block for a long period of time.  I hope to re-enable them soon.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 456792 [details] [diff] [review]
More cleanup.

>Bug 489813 - Fire timeout error if provider fails to response after initial response.  Also cleans up test. r=
>
>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -1,8 +1,9 @@
>+
No need for this




>+
>+class RequestPromptEvent : public nsRunnable {
{ should be in the next line


>+  NS_IMETHOD Run() {
ditto

>+    nsCOMPtr<nsIGeolocationPrompt> prompt = do_GetService(NS_GEOLOCATION_PROMPT_CONTRACTID);
>+    NS_ASSERTION(prompt, "null geolocation prompt");
Useless assertion, since you handle the !prompt case anyway

>+class RequestAllowEvent : public nsRunnable {
{ should be in the next line


>+
>+  NS_IMETHOD Run() {
Same here.


>+class RequestSendLocationEvent : public nsRunnable {
ditto

>+public:
>+  // a bit funky.  if locator is passed, that means this
>+  // event should remove the request from it.  If we ever
>+  // have to do more, then we can change this around.
>+  RequestSendLocationEvent(nsIDOMGeoPosition* position, nsGeolocationRequest* request, nsGeolocation* locator=nsnull)
Space before and after =
And parameters should be in form aFoo


>+  NS_IMETHOD Run() {
{ should be in the next line


> nsGeolocationService::IsBetterPosition(nsIDOMGeoPosition *aSomewhere)
>-{
>+{  
Don't add extra spaces

>+  printf("-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= nsGeolocation::Update called and there are %d / %d  pending\n", mPendingCallbacks.Length(), mWatchingCallbacks.Length());
?

 
>+  printf("-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= nsGeolocation::Update DONE\n");
?
>+  if (nsContentUtils::GetBoolPref("geo.prompt.testing", PR_FALSE))
>+  {
{ should be in the previous line


>+    printf("************** Geolocation Prompt is in test mode.\n");
?


I need to still review the tests.
But since the patch has printfs, it does seem like a bit WIP patch. So r- for now.

Comment 23

[Doug Turner (:dougt)]

Attachment: More cleanup.

merged to truck. addresses olli's comments.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 459117 [details] [diff] [review]
More cleanup.

>+class RequestPromptEvent : public nsRunnable
>+{
>+public:
>+  RequestPromptEvent(nsGeolocationRequest* request)
>+    : mRequest(request)
>+  {
>+  }
>+
>+  NS_IMETHOD Run() {
{ should be in the next line.
Same problem also elsewhere.

>+  if (nsContentUtils::GetBoolPref("geo.prompt.testing", PR_FALSE))
>+  {
>+    printf("************** Geolocation Prompt is in test mode.\n");
Could you perhaps put the printf inside #ifdef DEBUG


> function start_sending_garbage()
> {
>   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>   var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
>   prefs.setCharPref("geo.wifi.uri", "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs?action=respond-garbage");
>+
>+  // we need to be sure that all location data has been purged/set.
>+  sleep(1000);
> }
> 
> function stop_sending_garbage()
> {
>   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>   var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
>   prefs.setCharPref("geo.wifi.uri", "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs");
>+
>+  // we need to be sure that all location data has been purged/set.
>+  sleep(1000);
> }
> 
> function stop_geolocationProvider()
> {
>   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>   var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
>   prefs.setCharPref("geo.wifi.uri", "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs?action=stop-responding");
>+
>+  // we need to be sure that all location data has been purged/set.
>+  sleep(1000);
> }
Could you explain the sleep. Is there any better way? This looks pretty error prone. 1s isn't that long time.


> function successCallback(position) {
>-  ok(hasAccepted, "Ensure that accept was pressed");
>+
>+  dump("aaaaaaaaaaaaaa!!!\n");
?

> function successCallback(pos){
>   ok(false, "success should have never been called.");
>+  dump("success callback sent: " + pos + "\n");
?

 
> function successCallback(position) {
>-  check_geolocation(position);
>-  completeCount--;
>-  if (completeCount > 0)   
>-    navigator.geolocation.getCurrentPosition(successCallback, null, null);
>+  dump("----------> "+ keepGoing +"\n");
?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 459117 [details] [diff] [review]
More cleanup.

Minusing until the sleep() thingie is fixed or explained.

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 459117 [details] [diff] [review]
More cleanup.

so.... the sleep() are basically a way for the tests to wait until the platform has enough time to purge any caching it has done of other locations.  It needs to be called when we tell the location provider to change the type/quality of the locations it provides.

Right now, the platform gets a new location every 250ms.  This pause is long enough for us to ensure that when we switch location types, we will get what we expect.

I plan to make this better (and enable more tests).  i can remove these calls when I push if you like, or I can leave them in and follow up.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #26)
> Right now, the platform gets a new location every 250ms.  This pause is long
> enough for us to ensure that when we switch location types, we will get what we
> expect.
Well, sleep(1000) waits for 1 second. What if garbage collection happens at that
time and it takes more than a second? (It very well can take that long
on a slow machine/debug build)

Comment 28

[Doug Turner (:dougt)]

sleep() isn't used right now.  the tests are disabled.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Can you think of any better way to test what you want to test?
We really don't want untested code.
(Sorry, I'm going to require more tests the closer we get to FF4 release)

Comment 30

[Doug Turner (:dougt)]

the tests were broken before.  this patch makes us test much more accurately.  I disabled two tests:

1) garbageWatch -- this verifies that if we get bad data from a location provider, we don't do bad things.  the risk is pretty low.  through code inspection, you can verify that right now, we are okay

2) timeoutWatch -- this verifies that we do get timeout errors when the geolocation provider doesn't send us data (assuming web content asked for such an error).  this is really the more important of the two.


I'd like to get this larger patch in, then worry about following up for (1), and (2) as it fixes the tests, reduces oranges, and actually fixes the behavior of how initial responses are handled.

As to how we fix the broke/disabled tests, I would imagine that I would add more code to the core which allows us to purge all cached values only in the testing case.  This should fix the problem once and for all.  I will do this in a follow up patch.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 459117 [details] [diff] [review]
More cleanup.

Ok

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This landed but was backed out because of

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280960468.1280965400.28903.gz&fulltext=1

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You're also spewing a lot of crap into the logs.

Comment 34

[Doug Turner (:dougt)]

ffffuu.. wrong patch pushed.

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

FTR:

http://hg.mozilla.org/mozilla-central/rev/1d8d2deb3644
http://hg.mozilla.org/mozilla-central/rev/ad122c354040

Comment 36

[Doug Turner (:dougt)]

Comment on attachment 459117 [details] [diff] [review]
More cleanup.

i'll also remove the extra dump+printf stuff.

Comment 37

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: More cleanup (updated for bitrot)

updated for bitrot, no changes other than putting the code in the same spot which had some extra lines before it to confuse hg.

Marking r+ as a carryover, please change if this needs to be reviewed again.

Comment 38

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/0d8d6369655d

Comment 39

[Rafael Coutinho]

This is impacting our product, so I see it's planned for mozilla2.0b7, how do I map that milestone to FF version?

Comment 40

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Rafael Coutinho from comment #39)
> This is impacting our product, so I see it's planned for mozilla2.0b7, how
> do I map that milestone to FF version?

This fix shipped in Firefox 4.