bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

geolocation provider accuracy not being reset properly

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
Geolocation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Stephen Li, Assigned: mjh563)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.66 Safari/537.36

Steps to reproduce:

1. Run a high and low accuracy tracking session in parallel
2. ClearWatch the high accuracy session



Actual results:

The low accuracy session continues to run with high accuracy fixes


Expected results:

The provider should have got a reset setHighAccuracy to false.
(Reporter)

Updated

5 years ago
Summary: Issue with accuracy not getting properly → Issue with accuracy not resetting properly
blocking-b2g: --- → koi+
(Reporter)

Comment 1

5 years ago
This fix is important because if all high accuracy requests have exited,  the provider should be informed to switch to low accuracy so the GPS engine can be turned off. This happens only with 2 tracking sessions, one high and one low.
The bug title needs to be better defined here to be actionable - are you talking about geolocation here? A reduced test case would help make this more actionable.
(Reporter)

Updated

5 years ago
Summary: Issue with accuracy not resetting properly → geolocation provider accuracy not being reset properly
(Reporter)

Comment 3

5 years ago
Changed to title to reflect the issue better.

Updated

5 years ago
Component: General → Geolocation
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Release triage:

Doug - This has been made a 1.2 CS blocker. Can you help find an assignee for this bug?
Flags: needinfo?(doug.turner)

Comment 5

5 years ago
dhuseby, is this something you can pick up?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(doug.turner)
(Assignee)

Updated

5 years ago
Assignee: nobody → mjh563
(Assignee)

Comment 6

5 years ago
Created attachment 812450 [details] [diff] [review]
patch

This happens because the call to nsGeolocationService::SetHigherAccuracy(false) in nsGeolocationRequest::Shutdown has no effect. The request being shut down is still in the list of callbacks at this point, so nsGeolocationService::HighAccuracyRequested() returns true even when this is the only high accuracy request. So the provider is never notified of a change.

The request has already been shut down though (mShutdown = true), so the simplest solution is to make nsGeolocationRequest::WantsHighAccuracy return false for requests that have been shut down.

I've also made a few other changes, not sure if they're wanted but I can remove them if not:

(1) rename SetHigherAccuracy to better reflect what it does
(2) add some additional logging to NetworkGeolocationProvider.js
(3) also update the accuracy when a Geolocation object is shut down, this catches the case where the high accuracy request is ended due to the tab being closed
Attachment #812450 - Flags: feedback?(josh)

Comment 7

5 years ago
Comment on attachment 812450 [details] [diff] [review]
patch

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

Your changes are sensible like usual :) Thank you for figuring this out!

::: dom/src/geolocation/nsGeolocation.cpp
@@ +465,5 @@
>      if (mOptions->mMaximumAge >= 0) {
>        maximumAge = mOptions->mMaximumAge;
>      }
>    }
> +  gs->UpdateAccuracy(mOptions && mOptions->mEnableHighAccuracy);

Just pass WantsHighAccuracy() here instead.

::: dom/system/NetworkGeolocationProvider.js
@@ +72,5 @@
>    this.wifiService = null;
>    this.timer = null;
>    this.hasSeenWiFi = false;
>    this.started = false;
> +  this.highAccuracy = false;

Add a comment explaining that this isn't used, but is useful for debugging interactions with the geolocation service.
Attachment #812450 - Flags: feedback?(josh) → feedback+
(Reporter)

Comment 8

5 years ago
What is pending on this issue ?
By when can we get the fix ?
(Assignee)

Comment 9

5 years ago
Created attachment 816291 [details] [diff] [review]
patch (including test)

Updated the patch to include the requested changes plus a test.
Attachment #812450 - Attachment is obsolete: true
blocking-b2g: koi+ → ---
Attachment #816291 - Flags: review?(josh)
(Assignee)

Comment 10

5 years ago
(The blocking-b2g flag got reset with my last comment for some reason.)
(ftfy)
blocking-b2g: --- → koi+
Comment on attachment 816291 [details] [diff] [review]
patch (including test)

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

This looks good; we just need to make sure we annotate xpcshell.ini correctly. Can you make the changes and reattach?

::: dom/tests/unit/xpcshell.ini
@@ +12,5 @@
>  skip-if = os == "android"
>  [test_geolocation_timeout_wrap.js]
>  skip-if = os == "mac" || os == "android"
> +[test_geolocation_reset_accuracy.js]
> +[test_geolocation_reset_accuracy_wrap.js]

Add the same annotations for these two tests as the other _wrap and non-_wrap ones.
Attachment #816291 - Flags: review?(josh) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 816950 [details] [diff] [review]
patch (including test)

(In reply to Josh Matthews [:jdm] from comment #12)
> Add the same annotations for these two tests as the other _wrap and
> non-_wrap ones.

Done.
Attachment #816291 - Attachment is obsolete: true
Attachment #816950 - Flags: review?(josh)

Updated

5 years ago
Attachment #816950 - Flags: review?(josh) → review+

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/403bbc5e127f
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/403bbc5e127f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc0306c09d59
status-b2g-v1.2: --- → fixed
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
status-firefox27: --- → fixed
Two test failures in the first 4 pushes isn't encouraging. Backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7b18d2da3414

https://tbpl.mozilla.org/php/getParsedLog.php?id=29196410&tree=Mozilla-Aurora
status-b2g-v1.2: fixed → affected
status-firefox26: fixed → affected
And actually yes, this is failing on trunk too. Backed out there as well.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1850054aca10

https://tbpl.mozilla.org/php/getParsedLog.php?id=29207938&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=29200784&tree=Mozilla-Inbound
Status: RESOLVED → REOPENED
status-firefox27: fixed → affected
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: mozilla27 → ---
(Assignee)

Comment 19

5 years ago
Created attachment 819403 [details] [diff] [review]
patch for try server

Hmm, maybe the test failures are due to the timeout between setting and clearing the watch being too short?

This patch increases the timeout and adds some extra logging so we can see what's going on. Would someone be able to submit it to the try server (I don't have access myself)?
(Assignee)

Comment 21

5 years ago
Created attachment 819435 [details] [diff] [review]
patch for checkin

Well the try run is green, so maybe this one will avoid the test failures.

I've carried forward the r+, because the only change from the previous patch is to increase the timeout used in the test. Hope that's OK.
Attachment #816950 - Attachment is obsolete: true
Attachment #819403 - Attachment is obsolete: true
Attachment #819435 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
The timeouts worry me, since their presence is usually a symptom of tests that will intermittently fail, but we don't have a good way of simulating something like cross-process yield right now. Let's land this and file another bug for cleaning this up by using runnables in-process and finding a way to communicate cross-process.
https://hg.mozilla.org/mozilla-central/rev/37139015b5cd
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

5 years ago
status-firefox27: affected → fixed
For aurora/koi uplift this needs an updated patch:

[/c/src-gecko/aurora]$ transplant 37139015b5cd
searching for changes
applying 37139015b5cd
patching file dom/src/geolocation/nsGeolocation.cpp
Hunk #2 FAILED at 439
1 out of 5 hunks FAILED -- saving rejects to file dom/src/geolocation/nsGeolocation.cpp.rej
patching file dom/tests/unit/xpcshell.ini
Hunk #1 FAILED at 6
1 out of 1 hunks FAILED -- saving rejects to file dom/tests/unit/xpcshell.ini.rej
patch failed to apply
Michael, do you have time to produce that?
Flags: needinfo?(mjh563)
(Assignee)

Comment 27

5 years ago
(In reply to Josh Matthews [:jdm] from comment #26)
> Michael, do you have time to produce that?

Yes, I'll attach an updated patch for aurora/koi.
Flags: needinfo?(mjh563)

Updated

5 years ago
Whiteboard: [needs-aurora-patch]
(Assignee)

Comment 28

5 years ago
Created attachment 822792 [details] [diff] [review]
patch for aurora

Updated

5 years ago
Whiteboard: [needs-aurora-patch]
https://hg.mozilla.org/releases/mozilla-aurora/rev/4947996681ed
status-firefox26: affected → fixed
status-b2g-v1.2: affected → fixed
Depends on: 931930

Updated

5 years ago
Keywords: verifyme

Comment 30

5 years ago
Is there any help needed for manual testing here? If yes can you please provide some steps in order to reproduce/verify this?
Flags: needinfo?(mjh563)

Comment 31

5 years ago
Any updates on the above comment?
Comment 0 describes the problem situation, but I don't think it's possible to verify the internal state of the geolocation provider outside of the engine.
Flags: needinfo?(mjh563)

Updated

5 years ago
Keywords: verifyme
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.