Closed Bug 820683 Opened 7 years ago Closed 7 years ago

geolocation.watchPosition timeout when device is stationary

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: m1, Assigned: kanru)

References

Details

Attachments

(1 file, 1 obsolete file)

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
(In reply to Michael Vines [:m1] from comment #0)
>  mProviders.Count() is 0 in the chrome process

s/chrome/content/
Assignee: nobody → kchen
blocking-basecamp: ? → +
Priority: -- → P2
Target Milestone: --- → B2G C3 (12dec-1jan)
Add Askeing for providing QA's help
QA Contact: fyen
Al, what's the QA ask for here? Kan-ru, do you need any more info to fix this?
Priority: P2 → P1
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.
Attachment #694271 - Flags: review?(doug.turner)
Whiteboard: [has patch, needs review 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.
Attachment #694271 - Flags: review?(doug.turner) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a71e8b545dc
Whiteboard: [has patch, needs review dougt]
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
Attachment #695389 - Flags: approval-mozilla-b2g18?
Attachment #695389 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8a71e8b545dc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
kanru - with blocking-basecamp+, you can uplift.
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
Attachment #695389 - Flags: approval-mozilla-b2g18?
Attachment #695389 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.