1203999 - Pin the Web is leaking setting observers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P2)

Description

[vnicolas]

Happens to me just opening/closing app while hacking.

I/Gecko   ( 9975): -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR dev.gaia.pinning_the_web: 17 FROMaddObserver@jar:file:///system/b2g/omni.ja!/components/SettingsManager.js:382:48
I/Gecko   ( 9975): sl_observe@app://system.gaiamobile.org/shared/js/settings_listener.js:58:5
I/Gecko   ( 9975): BrowserContextMenu@app://system.gaiamobile.org/js/browser_context_menu.js:21:1
I/Gecko   ( 9975): BaseModule.instantiate/constructor@app://system.gaiamobile.org/js/base_module.js:471:9
I/Gecko   ( 9975): BaseModule.instantiate@app://system.gaiamobile.org/js/base_module.js:474:1
I/Gecko   ( 9975): aw_installSubComponents@app://system.gaiamobile.org/js/app_window.js:863:1
I/Gecko   ( 9975): aw_render@app://system.gaiamobile.org/js/app_window.js:702:5
I/Gecko   ( 9975): AppWindow@app://system.gaiamobile.org/js/app_window.js:65:5
I/Gecko   ( 9975): AppWindowFactory.prototype.trackLauchingWindow@app://system.gaiamobile.org/js/app_window_factory.js:230:17
I/Gecko   ( 9975): awf_launch/launchApp@app://system.gaiamobile.org/js/app_window_factory.js:216:11
I/Gecko   ( 9975): awf_launch@app://system.gaiamobile.org/js/app_window_factory.js:224:11
I/Gecko   ( 9975): awf_handleEvent@app://system.gaiamobile.org/js/app_window_factory.js:125:11

Comment 1

[Sam Foster [:sfoster] (he/him)]

What in this stack trace makes you finger Pin the Web as the source of this leak? Is this coming from related changes in AppChrome?

Comment 2

[Sam Foster [:sfoster] (he/him)]

> I/Gecko   ( 9975): -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR
> dev.gaia.pinning_the_web: 17

Doh, nm :)

Comment 3

[Martijn Wargers (dead)]

Is this fixed by bug 1193051?

Comment 4

[Alberto Pastor [:albertopq]]

Nope. I'll take this one.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> Is this fixed by bug 1193051?

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

Comment 6

[:gerard-majax]

Comment on attachment 8660405 [details] [review]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

Applied locally, it seems to do the job as expected :)

Comment 7

[vnicolas]

You may want to double check that once the BrowserContextMenu.show method has been called and the app is killed by a memory pressure the BrowserContextMenu.hide method is really called, which is unclear looking at the code (but maybe there is an abstraction that does that).

Otherwise we may still leak in some cases.

Comment 8

[Alberto Pastor [:albertopq]]

Comment on attachment 8660405 [details] [review]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

Vivien, could you please review this? Thanks

Comment 9

[vnicolas]

Comment on attachment 8660405 [details] [review]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

f+ but I'm not sure the call to hideBrowserContextMenu covers all the cases here.
For example does it covers app killed by the user from the card view ? If yes consider my f+ to be a r+ :)

Comment 10

[Alberto Pastor [:albertopq]]

Comment on attachment 8660405 [details] [review]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

You are totally right. I assumed the context menu was being hidden when entering in the cards view.

Comment 11

[vnicolas]

Comment on attachment 8660405 [details] [review]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

Sorry for all this back and forth. I realized that this solution may be a bit racy if the user quickly open the contextmenu and click on something and also I would prefer and explicit call to this.contextmenu.destroy() in the destroy() method of AppWindow.
It makes it clearer that the ContextMenu may have some stuff to clear, instead of a call to this.contextmenu.hide() which is a bit implicit and may let someone change the code in the future to avoid calling this code if the contextmenu is not visible.

So I think you are close to finish the patch and this should be the last iteration!

Comment 12

[Alberto Pastor [:albertopq]]

After thinking again about it, I believe is safer just getting the preference (and unobserve) every time the context menu items list is generated. Creating the observer in the constructor would result on the initial bug, as the context menu is instantiated (without showing) many times.

Comment 13

[Alberto Pastor [:albertopq]]

Comment on attachment 8660405 [details] [review]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

Sorry for the rush, but is blocking some bugs... Etienne, could you please take a look? Thanks!

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8660405 [details] [review]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master

r=me with the last commit addressed :) Thanks!

Comment 15

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/27a86ef0e13fa7d02417a5094bb2712a67a623ab