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

RESOLVED FIXED in 1.1 QE3 (26jun)

Status

()

--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: stephenl, Assigned: jdm)

Tracking

(Blocks: 1 bug)

Trunk
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 wontfix, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: c=)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Severity: normal → major
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(Reporter)

Updated

6 years ago
Severity: major → critical
(Reporter)

Comment 1

6 years ago
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
(Assignee)

Comment 5

6 years ago
Created attachment 758822 [details] [diff] [review]
Ensure the geolocation provider's high accuracy flag is reset when all requests that require it are satisfied.

b2g18 patch
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 7

6 years ago
(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.
(Reporter)

Comment 8

6 years ago
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+
(Reporter)

Comment 11

6 years ago
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
https://hg.mozilla.org/projects/birch/rev/9915f8dae4e5
Flags: in-testsuite+
Keywords: checkin-needed
(Assignee)

Comment 13

6 years ago
I guess I'll file a new bug to port this to mozilla-central after bug 874996 lands.
(Assignee)

Updated

6 years ago
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
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
You need to log in before you can comment on or make changes to this bug.