Closed Bug 867895 Opened 7 years ago Closed 7 years ago

High Accuracy state is not updated to low in concurrent use case

Categories

(Core :: DOM: Geolocation, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

()

RESOLVED FIXED
1.1 QE3 (26jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- wontfix
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: stephenl, Assigned: jdm)

References

Details

(Whiteboard: c=)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130409194949

Steps to reproduce:

Test with the two changes below:

https://bugzilla.mozilla.org/show_bug.cgi?id=866893
https://bugzilla.mozilla.org/show_bug.cgi?id=866913

request geolocation with one high accuracy and one low, then let high accuracy request time out or be served.


Actual results:

provider is not being notified to change its state to low


Expected results:

provider should be notified to change its state to low
Severity: normal → major
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Severity: major → critical
I am increasing the importance of the issue due to it causes more power consumption.
(leo+, due to power impact on a long-term background app running low accuracy fixes)
Status: UNCONFIRMED → NEW
blocking-b2g: --- → leo+
Ever confirmed: true
Severity: critical → major
Assignee: nobody → doug.turner
Whiteboard: c=
I think the str are:

Start a high accuracy application.
Start the low accuracy application.
Kill the high accuracy application.

The state of geo should be in low power mode.

Note, I don't think our gonk geolocation provider does anything with high accuracy at all right now.  Keep in mind that QC and other may replace our provider with their fancy commercial one.
-> jdm
Assignee: doug.turner → josh
Attachment #758822 - Flags: review?(doug.turner)
Comment on attachment 758822 [details] [diff] [review]
Ensure the geolocation provider's high accuracy flag is reset when all requests that require it are satisfied.

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

::: dom/src/geolocation/nsGeolocation.cpp
@@ +515,5 @@
> +
> +  // Attempt to save power when possible
> +  if (WantsHighAccuracy()) {
> +    nsRefPtr<nsGeolocationService> gs = nsGeolocationService::GetGeolocationService();
> +    gs->SetHigherAccuracy(false);

Suppose we have 3 watch requests.  Watch 1 and 2 requested high, watch 3 requested low.

if 1 is cleared, doesn't that screw request 2?

::: dom/tests/unit/test_highaccuracy.js
@@ +1,2 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

same issues as in _wrap

::: dom/tests/unit/test_highaccuracy_wrap.js
@@ +4,5 @@
> +const providerCID = Components.ID("{14aa4b81-e266-45cb-88f8-89595dece114}");
> +const providerContract = "@mozilla.org/geolocation/provider;1";
> +
> +var provider = {
> +  QueryInterface: function eventsink_qi(iid) {

eventsink_ --> something else.

@@ +29,5 @@
> +  },
> +  shutdown: function() {
> +  },
> +  setHighAccuracy: function(enable) {
> +    this._status = enable;

_status --> _highAccuracy
(In reply to Doug Turner (:dougt) from comment #6) 
> ::: dom/src/geolocation/nsGeolocation.cpp
> @@ +515,5 @@
> > +
> > +  // Attempt to save power when possible
> > +  if (WantsHighAccuracy()) {
> > +    nsRefPtr<nsGeolocationService> gs = nsGeolocationService::GetGeolocationService();
> > +    gs->SetHigherAccuracy(false);
> 
> Suppose we have 3 watch requests.  Watch 1 and 2 requested high, watch 3
> requested low.
> 
> if 1 is cleared, doesn't that screw request 2?

Nope. SetHigherAccuracy is no longer descriptive of what it does; if any existing requests require high accuracy it ignores the attempt to turn it off.
When can we expect this to be fixed?
Stephen, have you tried the patch?  Does it work in testing?  I can review this week.
Comment on attachment 758822 [details] [diff] [review]
Ensure the geolocation provider's high accuracy flag is reset when all requests that require it are satisfied.

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

::: dom/tests/unit/test_highaccuracy_wrap.js
@@ +1,1 @@
> +const Cc = Components.classes;

We should probalby move this provider business into a separate file and include it.
Attachment #758822 - Flags: review?(doug.turner) → review+
This patch has been tested. Can you mainline this change? Need it to land here by this Friday.
Target Milestone: --- → 1.1 QE3 (24jun)
Keywords: checkin-needed
I guess I'll file a new bug to port this to mozilla-central after bug 874996 lands.
Blocks: 884385
Backed out:
1.) Wasn't supposed to land on birch/m-c.
2.) Broke xpcshell (the manifest changes are broken)

https://hg.mozilla.org/projects/birch/rev/7a2a6d227a71
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.