Closed Bug 1089619 Opened 7 years ago Closed 7 years ago

Make GonkGPSGeolocation less hungry

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
2.1 S8 (7Nov)
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- affected

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 5 obsolete files)

+++ 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.
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
I'm wondering if we are not leaking observers ...
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.
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 ?
Flags: needinfo?(gkeeley)
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"
Attached patch bug1089619.diff (obsolete) — Splinter Review
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.
Flags: needinfo?(dbaron)
Good catch, observing the setting before that mStarted check is wrong
Flags: needinfo?(gkeeley)
Blocks: 1081780
No longer depends on: 1081780
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.
Depends on: 1056857
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().
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().
Attachment #8512615 - Attachment is obsolete: true
Attachment #8512617 - Flags: review?(dougt)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S8 (7Nov)
Nominating because it makes dogfooder's life miserable.
Attachment #8512617 - Attachment is obsolete: true
Attachment #8512617 - Flags: review?(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 on attachment 8512812 [details] [diff] [review]
Stop abusing the Settings API from Gonk GPS r=jdm

Switching reviewers as suggested by garvan.
Attachment #8512812 - Flags: review?(josh)
Depends on: 1064718
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
Attachment #8512812 - Flags: review?(josh) → review+
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 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.
Attachment #8514102 - Flags: review+
Attachment #8512812 - Attachment is obsolete: true
(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Pending try: https://tbpl.mozilla.org/?tree=Try&rev=7b43ffed7f4c

With a green try.
Flags: needinfo?(dbaron)
Keywords: checkin-needed
Reproduced locally, I should have a fix soon.
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().
Attachment #8514102 - Attachment is obsolete: true
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.
Attachment #8514284 - Flags: review+
(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 :)
Keywords: checkin-needed
Attachment #8512041 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7d4b9c10b746
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 1072155
Blocks: 1064718
No longer depends on: 1064718
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.
Flags: needinfo?(dbaron)
Already on master.
blocking-b2g: 2.2? → ---
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.
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.
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.