AppWindow could leak through its submodule BrowserContextMenu and then BrowserCotextMenu's submodule PinPageSystemDialog to SystemDialogManager.runningDialogs.

PinPageSystemDialog is not a base module, which has only destroy() but stop().

I'm not sure what's the general way to stop a base module's sub module instance without stop method. Is it OK if I call PinPageSystemDisalog.destroy() in BrowserContextMenu._stop()? Thanks.

If the BrowserContextMenu is meant to be restarted at some point (and so the submodules), destroying the PinPageSytemDialog seems fine. We should probably unregister the dialog from the SystemDialogManager when destroying, though. We need to add a listener to the 'destroy' event in the SystemDialogManager, and unregister in that case. Let me know if you want me to work on it. Thanks!

Blocking a 2.5+ bug.

Comment on attachment 8677294 [details] [gaia] janus926:bug-1217289 > mozilla-b2g:master The code looks good but, let's add some unit tests that check: - AppWindow stops submodules, if the method exists - The PinPageSystemDialog gets destroyed when the BrowserContextMenu gets stopped - The SytemDialogManager unregisters the dialog when receiving a destroyed event Thanks!

(In reply to Alberto Pastor [:albertopq] from comment #6) > The code looks good but, let's add some unit tests that check: I'm new this, where can I find similiar tests? There're many folders under gaia/tests.

Just found tests in apps/system/test/unit, I'll try make thme.

Let me know if you need any help with that in IRC

Comment on attachment 8677294 [details] [gaia] janus926:bug-1217289 > mozilla-b2g:master Tests added.

:ting, it seems that some tests are failing. Could you please rebase and push again?

Rebased, but there come more fails. I checked and don't seem relate to the patch.

(In reply to Ting-Yu Chou [:ting] from comment #12) > Rebased, but there come more fails. I checked and don't seem relate to the > patch. Just pinged :danhuang about this.

Rebased again as bug 1218211 just landed, still failed Gij16 and 20, investigating.

It seems PinPageSystemDialog is a singelton created by the first AppWindow (Homescreen), then I am not sure when should it be destroyed: - when the AppWindow creates it is destroyed, or - whenever an AppWindow is destroyed Both don't sound right since there could be other AppWindow reference it. Did I misunderstand anything?

You are right. I think we need to move the PinPageSystemDialog to the AppWindowManager (which only have 1 instance) instead of each app. What do you think?

I am not sure. Seems BrowserContextMenu relies on PinPageSystemDialog.onHide() to hide properly, won't move PinPageSystemDialog to AppWindowManager break it?

We probably need to use events for this. Do you want me to take this bug? It seems is more a design problem from the beginning. Thanks!

Sure, that'd be better, thank you.

Comment on attachment 8678942 [details] [gaia] albertopq:1217289-leak-pin-dialog > mozilla-b2g:master :ting, how does this look? Thanks!

Comment on attachment 8677294 [details] [gaia] janus926:bug-1217289 > mozilla-b2g:master Clearing

Comment on attachment 8678942 [details] [gaia] albertopq:1217289-leak-pin-dialog > mozilla-b2g:master Will there be actionmenustarted/actionmenustopped event? ActionMenu is not listed as a SUB_MODULE like SimLockSystemDialog or PinPageSystemDialog.

Comment on attachment 8678942 [details] [gaia] albertopq:1217289-leak-pin-dialog > mozilla-b2g:master Michael, would you take a look? Thanks!

Comment on attachment 8678942 [details] [gaia] albertopq:1217289-leak-pin-dialog > mozilla-b2g:master There are two thing that I think are weird here: 1.) First browser content menu is created and destroyed along with the app window, but pin dialog lives in app window manager. It feels like this two components should live on the same "level" so to speak. Did you try simply not making PinPageSystemDialog a singleton anymore and allowing it to be created and destroyed along with the app window. 2.) In app window, we start and stop submodules in the install/uninstallSubComponents functions. This logic seems to already have started before your patch, but I really wonder what this is for and why we continue it. I think you should also address ting's comment about actionmenu lifecycle events. Overall though, the patch seems to work and if Ting says this fixes the leak lets just go with it and fix the archictectural stuff going forward.

It depends on how you look at the PinDialog. If you think is an element that can be shown in multiple windows at the same time, I agree that we would need to have 1 on each window (and move the dom accordingly) but if we think about it as a common dialog that lives in the dialog-overlay, then we need to find a common place that only loads it once. Regarding the actionmenu, :ting was commenting that because I changed 'actionmenucreated' by 'actionmenustarted'. When I saw the comment, I left it as it was in :ting's patch. Not sure what should we do!?

(In reply to Alberto Pastor [:albertopq] from comment #26) > It depends on how you look at the PinDialog. If you think is an element that > can be shown in multiple windows at the same time, I agree that we would > need to have 1 on each window (and move the dom accordingly) but if we think > about it as a common dialog that lives in the dialog-overlay, then we need > to find a common place that only loads it once. Makes sense. I guess in my mind both context menu and pin-dialog were common elements across all browser windows. But now looking at the code I see that's not true since the context menu is tied to only the current app window (eg. I can do edge gestures and it sticks to the window). > Regarding the actionmenu, :ting was commenting that because I changed > 'actionmenucreated' by 'actionmenustarted'. When I saw the comment, I left > it as it was in :ting's patch. Oh, ok gotcha. I was mostly commenting on the fact we are adding an 'actionmenudestroyed' event listener, but since ActionMenu does not use the module system for instantiation, I wasn't sure how we could ever get that event, so I'm not sure it does anything. In any case, it's a small detail.

master: https://github.com/mozilla-b2g/gaia/commit/717ee46c64fde7d69a1146f3c54e95b2ef20e706

Alberto, could you help to make sure this patch in v2.5 branch? Thanks.

Yes, it's there.

Got it. Thanks.

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion