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

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Stephen Li]

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

Comment 1

[Stephen Li]

I am increasing the importance of the issue due to it causes more power consumption.

Comment 2

[Michael Vines [:m1] [:evilmachines]]

(leo+, due to power impact on a long-term background app running low accuracy fixes)

Comment 3

[Doug Turner (:dougt)]

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

[Doug Turner (:dougt)]

-> jdm

Comment 5

[Josh Matthews [:jdm]]

Attachment: Ensure the geolocation provider's high accuracy flag is reset when all requests that require it are satisfied.

b2g18 patch

Comment 6

[Doug Turner (:dougt)]

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

Comment 7

[Josh Matthews [:jdm]]

(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.

Comment 8

[Stephen Li]

When can we expect this to be fixed?

Comment 9

[Doug Turner (:dougt)]

Stephen, have you tried the patch?  Does it work in testing?  I can review this week.

Comment 10

[Doug Turner (:dougt)]

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.

Comment 11

[Stephen Li]

This patch has been tested. Can you mainline this change? Need it to land here by this Friday.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/9915f8dae4e5

Comment 13

[Josh Matthews [:jdm]]

I guess I'll file a new bug to port this to mozilla-central after bug 874996 lands.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/08cb0fc1e205

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/08cb0fc1e205