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

RESOLVED FIXED in Firefox OS v1.1hd

Status

()

--
major
RESOLVED FIXED
6 years ago
5 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

Updated

6 years ago
Assignee: nobody → doug.turner
Whiteboard: c=

Comment 3

5 years ago
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.

Comment 4

5 years ago
-> jdm
Assignee: doug.turner → josh
(Assignee)

Comment 5

5 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

5 years ago
Attachment #758822 - Flags: review?(doug.turner)

Comment 6

5 years ago
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

5 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

5 years ago
When can we expect this to be fixed?

Comment 9

5 years ago
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

5 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)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/9915f8dae4e5
Flags: in-testsuite+
Keywords: checkin-needed
(Assignee)

Comment 13

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

Updated

5 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

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 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.