Closed Bug 824432 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
blocking-b2g -
Tracking Status
firefox20 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: kanru, Assigned: kanru)

Details

Attachments

(1 file, 2 obsolete files)

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."
The spec uses "could" which is not a RFC2119 keyword. I'm not sure whether this is required or not.
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."
This sounds like the problem I identified in bug 732923.
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.
Assignee: nobody → kchen
Attachment #695405 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #695693 - Flags: review?(doug.turner)
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).
Address the comment. Josh, could you review this?
Attachment #695693 - Attachment is obsolete: true
Attachment #695693 - Flags: review?(doug.turner)
Attachment #695701 - Flags: review?(josh)
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/kchen@mozilla.com-72a3c0b991a0
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/kchen@mozilla.com-72a3c0b991a0
Attachment #695701 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/4aeb2bd7b186
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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:
Attachment #695701 - Flags: approval-mozilla-b2g18?
Attachment #695701 - Flags: approval-mozilla-aurora?
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.
Attachment #695701 - Flags: approval-mozilla-b2g18?
Attachment #695701 - Flags: approval-mozilla-b2g18-
Attachment #695701 - Flags: approval-mozilla-aurora?
Attachment #695701 - Flags: approval-mozilla-aurora-
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+.
blocking-basecamp: --- → ?
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.
blocking-basecamp: ? → -
tracking-b2g18: --- → +
It will affect the Maps app.
blocking-b2g: --- → tef?
blocking-basecamp: - → ---
tracking-b2g18: + → ---
You need to log in before you can comment on or make changes to this bug.