1089619 - Make GonkGPSGeolocation less hungry

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[:gerard-majax]

+++ This bug was initially created as a clone of Bug #1081780 +++

So in GonkGPSGeolocationProvider, a listener for settings changes is added at: http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#823

This will be handled by the Observe() method at http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#993 and that code does a request to a local RequestSettingValue() method.

This will be handled at http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#395:
 - we get the settings service
 - we create a new lock, and request the value

This means that on each observed geolocation setting change triggered, we will create two new locks to read those values, when the observer already has the proper value when the observer gets triggered.

I'm not sure yet if this could explain the issue we are tracking there, but this looks much more than suboptimal already.

Comment 1

[:gerard-majax]

I need to check also if:
 - we could detect those bad uses case directly from SettingsRequestManager
 - current use could lead to improper results, like looping promises

Comment 2

[:gerard-majax]

I'm wondering if we are not leaking observers ...

Comment 3

[:gerard-majax]

So I added some debugging around:
 - when we add observers, http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#823
 - when we remove observers, http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#882

What I observe is:
 - Upon HERE Maps starting,
> 10-27 17:21:45.458   268   268 I Gecko   : geo: GonkGPSGeolocationProvider::Startup: observerService->AddObserver
> 10-27 17:21:45.458   268   268 I Gecko   : geo: GonkGPSGeolocationProvider::Startup: observerService->AddObserver
 - Upon HERE Maps put in task switcher,
> 10-27 17:22:07.049   268   268 I Gecko   : geo: GonkGPSGeolocationProvider::Startup: observerService->AddObserver
 - Upon killing HERE Maps from task switcher,
> 10-27 17:22:40.372   268   268 I Gecko   : geo: GonkGPSGeolocationProvider::Startup: observerService->AddObserver
> 10-27 17:22:46.388   268   268 I Gecko   : geo: GonkGPSGeolocationProvider::Shutdown: obs->RemoveObserver

So the final balance of AddObserver()/RemoveObserver() calls is clearly not 0.

Comment 4

[:gerard-majax]

There is a mStarted boolean that is supposed to guard some things. However, we check it AFTER doing the AddObserver() calls.

This has been changed back in bug 1056857, which landed in early september: this could be consistent with the failures we have been observing since.

Garvan, is there any rationale for adding the observers before checking mStarted ?

Comment 5

[:gerard-majax]

Moving mStarted checking before the settings, I see a proper balance in the logs. That seems not to break the features of geolocation debugging/ignoring. And I don't see the bunch of logcat messages we were seeing previously, i.e., "GeckoConsole: geo: Debug: GPS ignored 0"

Comment 6

[:gerard-majax]

Attachment: bug1089619.diff

This patches fixes:
 - checking mStarted earlier
 - avoid re-doing createLock().get() on mozsettings-changed, and directly read the value from there.

David, do you mind giving a try to applying this patch and see if you still feel the slow phone issue ?

I'm going to dogfood this patch too, but I'm getting quite confident this could be our root issue.

Comment 7

[:garvan]

Good catch, observing the setting before that mStarted check is wrong

Comment 8

[:gerard-majax]

So far, I have been stressing this since yesterday mainly using FxStumbler, and with the patch provided in attachment 8512041 [details] [diff] [review] then I don't have anymore issue after several hours.

Comment 9

[:gerard-majax]

Attachment: Stop abusing the Settings API from Gonk GPS

GPS handling for Gonk abuses the Settings API in two ways. First, on
mozsettings-changed event, it will trigger two new requests to read
values (debug enabled and GPS locations ignore). This is useless since
the event already contains the key that has changed and the new value,
so there is no need to do a createLock().get() call. Then, in startup
code, the Init() method is supposed to check itself whether it is
already running. This is done through the mStarted boolean. The same
Init() method is responsible for adding the mozsettings-changed
observer, which is removed by the Shutdown() method. Investigation on
device by using the Geolocation API has proven that we were leaking some
observers. This is because checking mStarted boolean is performed after
we request settings values and we install the mozsettings-changed
observer. So any time the Init() gets called, we install it but we just
remove it once in Shutdown().

Comment 10

[:gerard-majax]

Attachment: Stop abusing the Settings API from Gonk GPS r=dougt

GPS handling for Gonk abuses the Settings API in two ways. First, on
mozsettings-changed event, it will trigger two new requests to read
values (debug enabled and GPS locations ignore). This is useless since
the event already contains the key that has changed and the new value,
so there is no need to do a createLock().get() call. Then, in startup
code, the Init() method is supposed to check itself whether it is
already running. This is done through the mStarted boolean. The same
Init() method is responsible for adding the mozsettings-changed
observer, which is removed by the Shutdown() method. Investigation on
device by using the Geolocation API has proven that we were leaking some
observers. This is because checking mStarted boolean is performed after
we request settings values and we install the mozsettings-changed
observer. So any time the Init() gets called, we install it but we just
remove it once in Shutdown().

Comment 11

[:gerard-majax]

Pending try: https://tbpl.mozilla.org/?tree=Try&rev=7b43ffed7f4c

Comment 12

[:gerard-majax]

Nominating because it makes dogfooder's life miserable.

Comment 13

[:gerard-majax]

Attachment: Stop abusing the Settings API from Gonk GPS r=jdm

GPS handling for Gonk abuses the Settings API in two ways. First, on
mozsettings-changed event, it will trigger two new requests to read
values (debug enabled and GPS locations ignore). This is useless since
the event already contains the key that has changed and the new value,
so there is no need to do a createLock().get() call. Then, in startup
code, the Init() method is supposed to check itself whether it is
already running. This is done through the mStarted boolean. The same
Init() method is responsible for adding the mozsettings-changed
observer, which is removed by the Shutdown() method. Investigation on
device by using the Geolocation API has proven that we were leaking some
observers. This is because checking mStarted boolean is performed after
we request settings values and we install the mozsettings-changed
observer. So any time the Init() gets called, we install it but we just
remove it once in Shutdown().

Comment 14

[:gerard-majax]

Comment on attachment 8512812 [details] [diff] [review]
Stop abusing the Settings API from Gonk GPS r=jdm

Switching reviewers as suggested by garvan.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8512812 [details] [diff] [review]
Stop abusing the Settings API from Gonk GPS r=jdm

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

LGTM

Comment 16

[:gerard-majax]

Attachment: Stop abusing the Settings API from Gonk GPS r=kanru

GPS handling for Gonk abuses the Settings API in two ways. First, on
mozsettings-changed event, it will trigger two new requests to read
values (debug enabled and GPS locations ignore). This is useless since
the event already contains the key that has changed and the new value,
so there is no need to do a createLock().get() call. Then, in startup
code, the Init() method is supposed to check itself whether it is
already running. This is done through the mStarted boolean. The same
Init() method is responsible for adding the mozsettings-changed
observer, which is removed by the Shutdown() method. Investigation on
device by using the Geolocation API has proven that we were leaking some
observers. This is because checking mStarted boolean is performed after
we request settings values and we install the mozsettings-changed
observer. So any time the Init() gets called, we install it but we just
remove it once in Shutdown().

Comment 17

[:gerard-majax]

Comment on attachment 8514102 [details] [diff] [review]
Stop abusing the Settings API from Gonk GPS r=kanru

Carrying r+ from :kanru, just updating the commit message.

Comment 18

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Pending try: https://tbpl.mozilla.org/?tree=Try&rev=7b43ffed7f4c

With a green try.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6797040a2034

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3432380&repo=mozilla-inbound

Comment 21

[:gerard-majax]

Reproduced locally, I should have a fix soon.

Comment 22

[:gerard-majax]

Attachment: Stop abusing the Settings API from Gonk GPS r=kanru

GPS handling for Gonk abuses the Settings API in two ways. First, on
mozsettings-changed event, it will trigger two new requests to read
values (debug enabled and GPS locations ignore). This is useless since
the event already contains the key that has changed and the new value,
so there is no need to do a createLock().get() call. Then, in startup
code, the Init() method is supposed to check itself whether it is
already running. This is done through the mStarted boolean. The same
Init() method is responsible for adding the mozsettings-changed
observer, which is removed by the Shutdown() method. Investigation on
device by using the Geolocation API has proven that we were leaking some
observers. This is because checking mStarted boolean is performed after
we request settings values and we install the mozsettings-changed
observer. So any time the Init() gets called, we install it but we just
remove it once in Shutdown().

Comment 23

[:gerard-majax]

Comment on attachment 8514284 [details] [diff] [review]
Stop abusing the Settings API from Gonk GPS r=kanru

Carrying r+ from :kanru.

Fixing build with non unified builds: adding some missing includes and using mozilla::dom namespace.

Comment 24

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #23)
> Comment on attachment 8514284 [details] [diff] [review]
> Stop abusing the Settings API from Gonk GPS r=kanru
> 
> Carrying r+ from :kanru.
> 
> Fixing build with non unified builds: adding some missing includes and using
> mozilla::dom namespace.

That builds on both non unified and unified builds locally :)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7d4b9c10b746

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d4b9c10b746

Comment 27

[:gerard-majax]

David, this has landed on master. We should track if we still see bug 1064718, bug 1072155 and bug 1081780. Current status after one week of using this patch on one of my devices is that this is fixing all those three issues, but I'd like confirmation from someone else.

Comment 28

[Gregor Wagner [:gwagner]]

Already on master.

Comment 29

[David Baron :dbaron:]

bug 1105511 makes me concerned that some symptoms are fixed but the underlying problem is not.  But it's still early; let's see how the discussion there goes.

Comment 30

[David Baron :dbaron:]

I haven't seen the Settings API problems for a while; they do seem to be fixed now.  My impression was that it was bug 1105511 that really fixed things.