820683 - geolocation.watchPosition timeout when device is stationary

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Michael Vines [:m1] [:evilmachines]]

STR:
1)	Run a GPS app that uses geolocation.watchPosition.  Ensure the app is running OOP
2)	After a couple fixes, don’t move!
3)	If subsequent fixes from the GPS HAL don’t increase in accuracy or change position, we see nsIDOMGeoPositionError:TIMEOUT emitted to the app after some time.

Expected behavior is that while we may not get new fixes a timeout error would not be received by the app as fixes are still coming in regularly.  

Analysis:
IsBetterPosition() is determining that the new fixes are not better (we're reporting the same position/accuracy repeatedly) and returns false.  This causes the new fix to not be sent to the app and eventually we hit a timeout.

Why doesn’t this happen when not running OOP?  |mProviders.Count() == 1| so we return true early [1].  mProviders.Count() is 0 in the chrome process, and we recompute isBetter there as well.

We could (should?) probably additionally avoid calling nsGeolocationService::IsBetterPosition() in the content process.  This computation has already occurred in the system process and the determination that the fix /isBetter/ has already been made, otherwise the content process would not have received the fix. 

[1] http://mxr.mozilla.org/mozilla-beta/source/dom/src/geolocation/nsGeolocation.cpp#830

Comment 1

[Michael Vines [:m1] [:evilmachines]]

(In reply to Michael Vines [:m1] from comment #0)
>  mProviders.Count() is 0 in the chrome process

s/chrome/content/

Comment 2

[Al Tsai [:atsai]]

Add Askeing for providing QA's help

Comment 3

[Dietrich Ayala (:dietrich)]

Al, what's the QA ask for here? Kan-ru, do you need any more info to fix this?

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Restart the timeout timer if position is not better

This patch implements the watchPosition algorithm 5.2.1

  Cancel the pending timer. Note that the timer must be restarted once this algorithm jumps back to the beginning of the acquisition steps.

and

  In step 5.2.2 of the watch process, the successCallback is only invoked when a new position is obtained and this position differs significantly from the previously reported position.

It also skips the IsBetterPosition check in content processes.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 694271 [details] [diff] [review]
Restart the timeout timer if position is not better

Review of attachment 694271 [details] [diff] [review]:
-----------------------------------------------------------------

as always, great stuff.  r+ w/ the nits fixed.


I have a minor question about how this may changes the spec.  We can discuss after we checkin (since the change isn't really defined by the spec)

::: dom/src/geolocation/nsGeolocation.cpp
@@ +204,5 @@
> +  {
> +  }
> +
> +  NS_IMETHOD Run() {
> +    mRequest->SetTimeoutTimer();

Doesn't the timeout geolocation option only apply to the initial acquisition?  For example, suppose:

1) the call to geolocation specifies a timeout of 1s.
2) we quickly hand back a location
3) then suppose we get a series of updates where mIsFirstUpdate == false and aIsBetter == false
4) we re-arm the timeout timer.
5) then we don't get any updates from the geo device

In this case, won't we fire an timeout error?  I am not sure that this is what we want to do?  Thoughts?

"""
The timeout attribute only applies to the location acquisition operation. 
"""

Maybe we're okay.

@@ +554,5 @@
>                                                               mIsWatchPositionRequest ? nullptr : mLocator);
>      NS_DispatchToMainThread(ev);
> +  } else {
> +    nsCOMPtr<nsIRunnable> ev = new RequestRestartTimerEvent(this);
> +    NS_DispatchToMainThread(ev);

move the NS_DispatchToMainThread(ev); to outside of the if/else stmt.

nsCOMPtr<nsIRunnable> ev;
if (xxxx) {
  ev = 
} else {
  ev = 
}
NS_DispatchToMainThread(ev);

@@ +847,5 @@
>  {
>    if (!aSomewhere) {
>      return false;
>    }
>    

might as remove the trailing whitespace here.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Doug Turner (:dougt) from comment #5)
> Comment on attachment 694271 [details] [diff] [review]
> Restart the timeout timer if position is not better
> 
> Review of attachment 694271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> as always, great stuff.  r+ w/ the nits fixed.
> 
> 
> I have a minor question about how this may changes the spec.  We can discuss
> after we checkin (since the change isn't really defined by the spec)
> 
> ::: dom/src/geolocation/nsGeolocation.cpp
> @@ +204,5 @@
> > +  {
> > +  }
> > +
> > +  NS_IMETHOD Run() {
> > +    mRequest->SetTimeoutTimer();
> 
> Doesn't the timeout geolocation option only apply to the initial
> acquisition?  For example, suppose:

It applies to all acquisition operations:

 "The watch operation then must continue to monitor the position of the device and invoke the appropriate callback every time this position changes."
 
> 1) the call to geolocation specifies a timeout of 1s.
> 2) we quickly hand back a location
> 3) then suppose we get a series of updates where mIsFirstUpdate == false and
> aIsBetter == false
> 4) we re-arm the timeout timer.
> 5) then we don't get any updates from the geo device
> 
> In this case, won't we fire an timeout error?  I am not sure that this is
> what we want to do?  Thoughts?

We should continue to fire timeout errors until clearWatch is called. I thought we already handle this. But from looking at the code, it seems we cancel the request when we report a timeout error. This should be fixed in another bug.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Restart the timeout timer if position is not better. v1.1

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a71e8b545dc

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 695389 [details] [diff] [review]
Restart the timeout timer if position is not better. v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 805965
User impact if declined: Incorrect timeout if we have multiple providers enabled.
Testing completed: inbound
Risk to taking this patch (and alternatives if risky): little or none.
String or UUID changes made by this patch: N/A

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a71e8b545dc

Comment 11

[Doug Turner (:dougt)]

kanru - with blocking-basecamp+, you can uplift.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 695389 [details] [diff] [review]
Restart the timeout timer if position is not better. v1.1

What Doug said.

https://hg.mozilla.org/releases/mozilla-aurora/rev/404509707b15
https://hg.mozilla.org/releases/mozilla-b2g18/rev/cf7f6e8cbc2e