1201481 - WARNING: MORE THAN 10 OBSERVERS FOR copypaste.enabled: 101

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Gregor Wagner [:gwagner]]

09-03 14:03:42.315  3161  3161 I Gecko   : -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR copypaste.enabled: 101 FROMaddObserver@jar:file:///system/b2g/omni.ja!/components/SettingsManager.js:382:48
09-03 14:03:42.315  3161  3161 I Gecko   : sl_observe@app://system.gaiamobile.org/shared/js/settings_listener.js:58:5
09-03 14:03:42.315  3161  3161 I Gecko   : AppTextSelectionDialog@app://system.gaiamobile.org/js/app_text_selection_dialog.js:56:1
09-03 14:03:42.315  3161  3161 I Gecko   : aw_installSubComponents@app://system.gaiamobile.org/js/app_window.js:854:1
09-03 14:03:42.315  3161  3161 I Gecko   : aw_render@app://system.gaiamobile.org/js/app_window.js:702:5
09-03 14:03:42.315  3161  3161 I Gecko   : AppWindow@app://system.gaiamobile.org/js/app_window.js:65:5
09-03 14:03:42.315  3161  3161 I Gecko   : AppWindowFactory.prototype.trackLauchingWindow@app://system.gaiamobile.org/js/app_window_factory.js:230:17
09-03 14:03:42.315  3161  3161 I Gecko   : awf_launch/launchApp@app://system.gaiamobile.org/js/app_window_factory.js:216:11
09-03 14:03:42.315  3161  3161 I Gecko   : awf_launch@app://system.gaiamobile.org/js/app_window_factory.js:224:11
09-03 14:03:42.315  3161  3161 I Gecko   : awf_handleEvent@app://system.gaiamobile.org/js/app_window_factory.js:125:11
09-03 14:03:42.315  3161  3161 I Gecko   : AppWindowFactory.prototype.preHandleEvent@app://system.gaiamobile.org/js/app_window_factory.js:96:9
09-03 14:03:42.315  3161  3161 I Gecko   : shell_sendEvent@chrome://b2g/content/shell.js:593:5
09-03 14:03:42.315  3161  3161 I Gecko   : shell_sendCu

Comment 1

[Gregor Wagner [:gwagner]]

Seems like we are leaking copyandpaste.enabled observers. Phone feels very slow.

Comment 2

[Gregor Wagner [:gwagner]]

Build ID               20150903125820
Gaia Revision          d7385b79e68d4ad662cacf810506e9ee53345d23
Gaia Date              2015-09-03 10:40:46
Gecko Revision         n/a
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150826.122901
Firmware Date          Wed Aug 26 12:29:09 UTC 2015
Bootloader             s1

Comment 3

[Gregor Wagner [:gwagner]]

With a debug-gecko. Haven't tried with opt build.

Comment 4

[:gerard-majax]

This is something that should have been fixed by bug 1189078.

Comment 5

[:gerard-majax]

Did it regressed ?

Comment 6

[:gerard-majax]

Might be after bug 1194042 landed.

Comment 7

[Jeremy Chen ([e-mail])]

It's not a regression. We're now using app-based text-selection-dialog, which means each app would have its own observer for copypaste.enabled. If you open more than 10 apps, you'll see more than 10 observers. Once you kill these apps, the observers would be gone. So, it's not a leak problem.

I guess maybe we could find a way to just use one observer for all app-text-selection-dialogs.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jeremy Chen [:jeremychen] from comment #7)
> It's not a regression. We're now using app-based text-selection-dialog,
> which means each app would have its own observer for copypaste.enabled. If
> you open more than 10 apps, you'll see more than 10 observers. Once you kill
> these apps, the observers would be gone. So, it's not a leak problem.
> 

Jeremy, comment 0 shows there are 101 observers -- we are not likely to be have 101 apps launched at the same time. It might worth to invest in some effort figure out the life cycle of AppWindow -- maybe the instance don't get destroyed as long as it is listed on the card view?

> I guess maybe we could find a way to just use one observer for all
> app-text-selection-dialogs.

That's another way to fix this, maybe like what Ting-Yu did in bug 1194042?

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #8)
> (In reply to Jeremy Chen [:jeremychen] from comment #7)
> > It's not a regression. We're now using app-based text-selection-dialog,
> > which means each app would have its own observer for copypaste.enabled. If
> > you open more than 10 apps, you'll see more than 10 observers. Once you kill
> > these apps, the observers would be gone. So, it's not a leak problem.
> > 
> 
> Jeremy, comment 0 shows there are 101 observers -- we are not likely to be
> have 101 apps launched at the same time. It might worth to invest in some
> effort figure out the life cycle of AppWindow -- maybe the instance don't
> get destroyed as long as it is listed on the card view?
> 

Etienne, do you know the lifetime of AppWindow? Why are we ended up having 101 observers when we did removed the observer like bug 1189078 did?

Comment 10

[Jeremy Chen ([e-mail])]

Just discussed with Tim offline. Working on a patch which only use one global observer for all apps.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] chenpighead:1201481 > mozilla-b2g:master

Comment 12

[Jeremy Chen ([e-mail])]

Comment on attachment 8657060 [details] [review]
[gaia] chenpighead:1201481 > mozilla-b2g:master

SettingsListener.observe is moved into shared global states. I maintain an AppTextSelectionDialog array in global states to keep all AppTextSelectionDialog instances from launched apps. The state of each AppTextSelectionDialog would be updated whenever copypaste.enable pref is changed.

Tim, could you help review this? Thanks.

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #9)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #8)
> > (In reply to Jeremy Chen [:jeremychen] from comment #7)
> > > It's not a regression. We're now using app-based text-selection-dialog,
> > > which means each app would have its own observer for copypaste.enabled. If
> > > you open more than 10 apps, you'll see more than 10 observers. Once you kill
> > > these apps, the observers would be gone. So, it's not a leak problem.
> > > 
> > 
> > Jeremy, comment 0 shows there are 101 observers -- we are not likely to be
> > have 101 apps launched at the same time. It might worth to invest in some
> > effort figure out the life cycle of AppWindow -- maybe the instance don't
> > get destroyed as long as it is listed on the card view?
> > 
> 
> Etienne, do you know the lifetime of AppWindow? Why are we ended up having
> 101 observers when we did removed the observer like bug 1189078 did?

The fix looks promising, but let's make sure we're not hiding another issue.

The .destroy() method should definitely get called on each subComponent when the user closes the app (we keep some stuff around for OOM kills). And for all BaseUIs, .destroy() should call _unregisterEvents.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8657060 [details] [review]
[gaia] chenpighead:1201481 > mozilla-b2g:master

Beside updates the array to Set, this also need unit test too!

Comment 15

[Jeremy Chen ([e-mail])]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #14)
> Comment on attachment 8657060 [details] [review]
> [gaia] chenpighead:1201481 > mozilla-b2g:master
> 
> Beside updates the array to Set, this also need unit test too!

Thank you for the guide. I'll send review request again after addressing the feedback comments and adding unit test.

Comment 16

[Jeremy Chen ([e-mail])]

Comment on attachment 8657060 [details] [review]
[gaia] chenpighead:1201481 > mozilla-b2g:master

1. Use Set instead of Array.
2. Update app_text_selection_dialog_test.js.
2-1 update resetAllStates.
2-2 add unit test for maintaining _appTSDs in global states.
2-3 update test for switching settings value of copypaste.enabled.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8657060 [details] [review]
[gaia] chenpighead:1201481 > mozilla-b2g:master

Superb!

Comment 18

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/641d40343b6aacc55071a56acb5bd48367d6b2c0