Ting-Yu Chou [:ting] (away) Reporter
	Description • 9 years ago

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

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 1 • 9 years ago

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

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 2 • 9 years ago

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.

	Alberto Pastor [:albertopq] Assignee
	Comment 4 • 9 years ago

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!

	Ken Chang[:kenkai]
	Comment 5 • 9 years ago

Blocking a 2.5+ bug.

	Alberto Pastor [:albertopq] Assignee
	Comment 6 • 9 years ago

Comment on attachment 8677294 [details] [review]
[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!

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 7 • 9 years ago

(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.

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 8 • 9 years ago

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

	Alberto Pastor [:albertopq] Assignee
	Comment 9 • 9 years ago

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

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 10 • 9 years ago

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

Tests added.

	Alberto Pastor [:albertopq] Assignee
	Comment 11 • 9 years ago

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

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 12 • 9 years ago

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

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 13 • 9 years ago

(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.

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 14 • 9 years ago

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

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 15 • 9 years ago

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?

	Alberto Pastor [:albertopq] Assignee
	Comment 16 • 9 years ago

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?

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 17 • 9 years ago

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

	Alberto Pastor [:albertopq] Assignee
	Comment 18 • 9 years ago

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!

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 19 • 9 years ago

Sure, that'd be better, thank you.

	Alberto Pastor [:albertopq] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8678942 [details] [review]
[gaia] albertopq:1217289-leak-pin-dialog > mozilla-b2g:master

:ting, how does this look? Thanks!

	Alberto Pastor [:albertopq] Assignee
	Comment 22 • 9 years ago

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

Clearing

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 23 • 9 years ago

Comment on attachment 8678942 [details] [review]
[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.

	Alberto Pastor [:albertopq] Assignee
	Comment 24 • 9 years ago

Comment on attachment 8678942 [details] [review]
[gaia] albertopq:1217289-leak-pin-dialog > mozilla-b2g:master

Michael, would you take a look? Thanks!

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 25 • 9 years ago

Comment on attachment 8678942 [details] [review]
[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.

	Alberto Pastor [:albertopq] Assignee
	Comment 26 • 9 years ago

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!?

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 27 • 9 years ago

(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.

	Alberto Pastor [:albertopq] Assignee
	Comment 28 • 9 years ago

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

	Bobby Chien
	Comment 29 • 9 years ago

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

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 30 • 9 years ago

Yes, it's there.

	Bobby Chien
	Comment 31 • 9 years ago

Got it. Thanks.

	Carsten Book [:Tomcat]
	Comment 32 • 9 years ago

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