Closed Bug 489813 Opened 11 years ago Closed 9 years ago

fire timeout error if provider fails to response after initial response

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Duplicate of this bug: 526326
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #415071 - Flags: review?
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.
Attachment #415071 - Flags: review? → review?(jst)
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)
Flags: blocking1.9.2?
Would take a reviewed patch, don't think we need to block on it.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Attachment #415071 - Flags: review?(jst) → review?(Olli.Pettay)
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.
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 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?
Attachment #415071 - Flags: review?(Olli.Pettay) → review+
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.
Attached patch patch v.2 (obsolete) — Splinter Review
this ensures that while testing we do not filter the updates.  testing scripts can toggle this feature to test filter if they so desired.
Attachment #415071 - Attachment is obsolete: true
Attachment #424036 - Flags: review?(Olli.Pettay)
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...
Attachment #424036 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/0542680c6f74
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You're turning the tree a rather bright shade of orange.
backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
never mind... thought everything was in....
Attached patch patch v.3 (obsolete) — Splinter Review
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.
Attachment #424036 - Attachment is obsolete: true
Attachment #449576 - Flags: review?(jst)
(i probably want to change "nsRefPtr<nsGeolocationRequest>&" to "nsGeolocationRequest*" in the event classes.
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.
Attachment #449576 - Attachment is obsolete: true
Attachment #449576 - Flags: review?(jst)
Attached patch clean up the test error (obsolete) — Splinter Review
Attachment #452952 - Flags: review?(jst)
Attached patch More cleanup. (obsolete) — Splinter Review
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.
Attachment #452952 - Attachment is obsolete: true
Attachment #456792 - Flags: review?(Olli.Pettay)
Attachment #452952 - Flags: review?(jst)
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.
Attachment #456792 - Flags: review?(Olli.Pettay) → review-
Attached patch More cleanup. (obsolete) — Splinter Review
merged to truck. addresses olli's comments.
Attachment #456792 - Attachment is obsolete: true
Attachment #459117 - Flags: review?(Olli.Pettay)
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 on attachment 459117 [details] [diff] [review]
More cleanup.

Minusing until the sleep() thingie is fixed or explained.
Attachment #459117 - Flags: review?(Olli.Pettay) → review-
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.
Attachment #459117 - Flags: review- → review?(Olli.Pettay)
(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)
sleep() isn't used right now.  the tests are disabled.
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)
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 on attachment 459117 [details] [diff] [review]
More cleanup.

Ok
Attachment #459117 - Flags: review?(Olli.Pettay) → review+
You're also spewing a lot of crap into the logs.
ffffuu.. wrong patch pushed.
blocking2.0: --- → ?
blocking2.0: ? → -
blocking2.0: - → ?
blocking2.0: ? → ---
Comment on attachment 459117 [details] [diff] [review]
More cleanup.

i'll also remove the extra dump+printf stuff.
Attachment #459117 - Flags: approval2.0?
Attachment #459117 - Flags: approval2.0? → approval2.0+
Blocks: 495076
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.
Attachment #459117 - Attachment is obsolete: true
Attachment #470434 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/0d8d6369655d
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Flags: wanted-fennec1.0?
Flags: wanted-fennec1.0?
Target Milestone: --- → mozilla2.0b7
This is impacting our product, so I see it's planned for mozilla2.0b7, how do I map that milestone to FF version?
(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.
You need to log in before you can comment on or make changes to this bug.