Closed
Bug 914888
Opened 11 years ago
Closed 11 years ago
geolocation provider accuracy not being reset properly
Categories
(Core :: DOM: Geolocation, defect)
Core
DOM: Geolocation
Tracking
()
People
(Reporter: stephenl, Assigned: mjh563)
References
Details
(Whiteboard: [qa-])
Attachments
(2 files, 4 obsolete files)
12.72 KB,
patch
|
mjh563
:
review+
|
Details | Diff | Splinter Review |
12.72 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
Summary: Issue with accuracy not getting properly → Issue with accuracy not resetting properly
Updated•11 years ago
|
blocking-b2g: --- → koi+
Reporter | ||
Comment 1•11 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.
Comment 2•11 years ago
|
||
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•11 years ago
|
Summary: Issue with accuracy not resetting properly → geolocation provider accuracy not being reset properly
Reporter | ||
Comment 3•11 years ago
|
||
Changed to title to reflect the issue better.
Updated•11 years ago
|
Component: General → Geolocation
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Comment 4•11 years ago
|
||
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•11 years ago
|
||
dhuseby, is this something you can pick up?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(doug.turner)
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•11 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•11 years ago
|
||
What is pending on this issue ? By when can we get the fix ?
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•11 years ago
|
||
(The blocking-b2g flag got reset with my last comment for some reason.)
Comment 12•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
Attachment #816950 -
Flags: review?(josh) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/403bbc5e127f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
Comment 16•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc0306c09d59
status-b2g-v1.2:
--- → fixed
status-firefox25:
--- → wontfix
status-firefox26:
--- → fixed
status-firefox27:
--- → fixed
Comment 17•11 years ago
|
||
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
Comment 18•11 years ago
|
||
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 → ---
Assignee | ||
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9cfc78684e2d
Assignee | ||
Comment 21•11 years ago
|
||
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
Comment 22•11 years ago
|
||
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.
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/37139015b5cd
Keywords: checkin-needed
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/37139015b5cd
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Updated•11 years ago
|
Comment 25•11 years ago
|
||
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
Assignee | ||
Comment 27•11 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•11 years ago
|
Whiteboard: [needs-aurora-patch]
Assignee | ||
Comment 28•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [needs-aurora-patch]
Updated•11 years ago
|
Comment 30•11 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•11 years ago
|
||
Any updates on the above comment?
Comment 32•11 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•