824432 - navigator.geolocation.watchPosition does not report time out error periodically

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

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

Attachment: Patch to test_timerRestartWatch.html

According to the spec: "In case of a watchPosition(), the errorCallback could be invoked repeatedly: the first timeout is relative to the moment watchPosition() was called or the moment the user's permission was obtained, if that was necessary. 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."

Comment 1

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

The spec uses "could" which is not a RFC2119 keyword. I'm not sure whether this is required or not.

Comment 2

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

It's clear that our impl is incorrect: "If the attempt fails, the errorCallback must be invoked with a new PositionError object, reflecting the reason for the failure. The watch operation then MUST continue to monitor the position of the device and invoke the appropriate callback every time this position changes. The watch operation must continue until the clearWatch method is called with the corresponding identifier."

Comment 3

[Josh Matthews [:jdm]]

This sounds like the problem I identified in bug 732923.

Comment 4

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

It seems not. The problem in bug 732923 is that a watch request always ends in a timeout. The problem here is that the timeout is only fired once and we clear the timer.

Comment 5

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

Attachment: Don't cancel request if it's a watch request

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 695693 [details] [diff] [review]
Don't cancel request if it's a watch request

>+  NotifyError(nsIDOMGeoPositionError::TIMEOUT);
>+  if (mIsWatchPositionRequest) {
>+    SetTimeoutTimer();
>+  } else {
>+    mLocator->RemoveRequest(this);

The request should be removed before signalling the error, or else a getCurrentPosition request could see a real geolocation notification while inside NotifyError (such as if the error callback performed a sync XHR).

Comment 7

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

Attachment: Don't cancel request if it's a watch request. v1.1

Address the comment. Josh, could you review this?

Comment 8

[Mozilla RelEng Bot]

Try run for 72a3c0b991a0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=72a3c0b991a0
Results (out of 8 total builds):
    success: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-72a3c0b991a0

Comment 9

[Mozilla RelEng Bot]

Try run for 72a3c0b991a0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=72a3c0b991a0
Results (out of 9 total builds):
    success: 8
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-72a3c0b991a0

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4aeb2bd7b186

Comment 11

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/4aeb2bd7b186

Comment 12

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

Comment on attachment 695701 [details] [diff] [review]
Don't cancel request if it's a watch request. v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: incorrect timeout behavior might break Maps app.
Testing completed: m-c
Risk to taking this patch (and alternatives if risky): none, this is covered by tests
String or UUID changes made by this patch:

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 695701 [details] [diff] [review]
Don't cancel request if it's a watch request. v1.1

Given where we are in the release, we should only be fixing known issues. This represents only the possibility of regression, not an actual regression.

Comment 14

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

This makes our implementation non-confirming. And I believe it will cause unexpected behavior because GPS is likely to timeout in indoor. Renominating for bb+.

Comment 15

[Lucas Adamski [:ladamski]]

Don't think this needs to go on aurora but this is a fix we should take on b2g18.  Currently GPS behavior is very unreliable especially when initiated indoors.

Comment 16

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

It will affect the Maps app.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/aabed455127e