If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make GonkGPSGeolocation less hungry

RESOLVED FIXED in 2.1 S8 (7Nov)

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gerard, Assigned: gerard)

Tracking

Trunk
2.1 S8 (7Nov)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 affected)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
+++ 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.
(Assignee)

Comment 1

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

Comment 2

3 years ago
I'm wondering if we are not leaking observers ...
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

3 years ago
Created attachment 8512041 [details] [diff] [review]
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.
Flags: needinfo?(dbaron)

Comment 7

3 years ago
Good catch, observing the setting before that mStarted check is wrong
Flags: needinfo?(gkeeley)
Blocks: 1081780
No longer depends on: 1081780
(Assignee)

Comment 8

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

Updated

3 years ago
Depends on: 1056857
(Assignee)

Comment 9

3 years ago
Created attachment 8512615 [details] [diff] [review]
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().
(Assignee)

Comment 10

3 years ago
Created attachment 8512617 [details] [diff] [review]
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().
(Assignee)

Updated

3 years ago
Attachment #8512615 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8512617 - Flags: review?(dougt)
(Assignee)

Updated

3 years ago
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S8 (7Nov)
(Assignee)

Comment 11

3 years ago
Pending try: https://tbpl.mozilla.org/?tree=Try&rev=7b43ffed7f4c
(Assignee)

Updated

3 years ago
blocking-b2g: --- → 2.2?
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → affected
(Assignee)

Comment 12

3 years ago
Nominating because it makes dogfooder's life miserable.
(Assignee)

Updated

3 years ago
Attachment #8512617 - Attachment is obsolete: true
Attachment #8512617 - Flags: review?(dougt)
(Assignee)

Comment 13

3 years ago
Created attachment 8512812 [details] [diff] [review]
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().
(Assignee)

Comment 14

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

Updated

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

Comment 16

3 years ago
Created attachment 8514102 [details] [diff] [review]
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().
(Assignee)

Comment 17

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

Updated

3 years ago
Attachment #8512812 - Attachment is obsolete: true
(Assignee)

Comment 18

3 years ago
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/6797040a2034
Keywords: checkin-needed
sorry had to back this out for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3432380&repo=mozilla-inbound
(Assignee)

Comment 21

3 years ago
Reproduced locally, I should have a fix soon.
(Assignee)

Comment 22

3 years ago
Created attachment 8514284 [details] [diff] [review]
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().
(Assignee)

Updated

3 years ago
Attachment #8514102 - Attachment is obsolete: true
(Assignee)

Comment 23

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

Comment 24

3 years ago
(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/integration/b2g-inbound/rev/7d4b9c10b746
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7d4b9c10b746
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1072155
(Assignee)

Updated

3 years ago
Blocks: 1064718
No longer depends on: 1064718
(Assignee)

Comment 27

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