Closed Bug 914888 Opened 6 years ago Closed 6 years ago

geolocation provider accuracy not being reset properly

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: stephenl, Assigned: mjh563)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 4 obsolete files)

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.
Summary: Issue with accuracy not getting properly → Issue with accuracy not resetting properly
blocking-b2g: --- → koi+
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.
Summary: Issue with accuracy not resetting properly → geolocation provider accuracy not being reset properly
Changed to title to reflect the issue better.
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)
dhuseby, is this something you can pick up?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(doug.turner)
Assignee: nobody → mjh563
Attached patch patch (obsolete) — Splinter Review
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 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+
What is pending on this issue ?
By when can we get the fix ?
Attached patch patch (including test) (obsolete) — Splinter Review
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)
(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+
Attached patch patch (including test) (obsolete) — Splinter Review
(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)
Attachment #816950 - Flags: review?(josh) → review+
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
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
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
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: mozilla27 → ---
Attached patch patch for try server (obsolete) — Splinter Review
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)?
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+
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
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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)
(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)
Whiteboard: [needs-aurora-patch]
Attached patch patch for auroraSplinter Review
Whiteboard: [needs-aurora-patch]
Depends on: 931930
Keywords: verifyme
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)
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)
Keywords: verifyme
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.