Leak browser iframe through BrowserContextMenu and PinPageSystemDialog

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ting, Assigned: apastor)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.5+, b2g-master fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
AppWindow could leak through its submodule BrowserContextMenu and then BrowserCotextMenu's submodule PinPageSystemDialog to SystemDialogManager.runningDialogs.
(Reporter)

Comment 1

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

Comment 2

3 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.
Flags: needinfo?(apastor)
Whiteboard: [systemsfe]
Created attachment 8677294 [details] [review]
[gaia] janus926:bug-1217289 > mozilla-b2g:master
(Assignee)

Comment 4

3 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!
Flags: needinfo?(apastor)

Comment 5

3 years ago
Blocking a 2.5+ bug.
blocking-b2g: --- → 2.5+
Priority: -- → P1
(Reporter)

Updated

3 years ago
Attachment #8677294 - Flags: review?(apastor)
(Assignee)

Updated

3 years ago
Assignee: nobody → janus926
(Assignee)

Comment 6

3 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!
Attachment #8677294 - Flags: review?(apastor)
(Reporter)

Comment 7

3 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.
(Reporter)

Comment 8

3 years ago
Just found tests in apps/system/test/unit, I'll try make thme.
(Assignee)

Comment 9

3 years ago
Let me know if you need any help with that in IRC
(Reporter)

Comment 10

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

Tests added.
Attachment #8677294 - Flags: review?(apastor)
(Assignee)

Comment 11

3 years ago
:ting, it seems that some tests are failing. Could you please rebase and push again?
Flags: needinfo?(janus926)
(Reporter)

Comment 12

3 years ago
Rebased, but there come more fails. I checked and don't seem relate to the patch.
Flags: needinfo?(janus926)
(Reporter)

Comment 13

3 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.
(Reporter)

Comment 14

3 years ago
Rebased again as bug 1218211 just landed, still failed Gij16 and 20, investigating.
(Reporter)

Comment 15

3 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?
Flags: needinfo?(apastor)
(Assignee)

Comment 16

3 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?
Flags: needinfo?(apastor) → needinfo?(janus926)
(Reporter)

Comment 17

3 years ago
I am not sure. Seems BrowserContextMenu relies on PinPageSystemDialog.onHide() to hide properly, won't move PinPageSystemDialog to AppWindowManager break it?
Flags: needinfo?(janus926)
(Assignee)

Comment 18

3 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!
Flags: needinfo?(janus926)
(Reporter)

Comment 19

3 years ago
Sure, that'd be better, thank you.
Assignee: janus926 → apastor
Flags: needinfo?(janus926)
Created attachment 8678942 [details] [review]
[gaia] albertopq:1217289-leak-pin-dialog > mozilla-b2g:master
(Assignee)

Comment 21

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

:ting, how does this look? Thanks!
Attachment #8678942 - Flags: feedback?(janus926)
(Assignee)

Comment 22

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

Clearing
Attachment #8677294 - Flags: review?(apastor)
(Reporter)

Comment 23

3 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.
Attachment #8678942 - Flags: feedback?(janus926) → feedback+
(Assignee)

Comment 24

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

Michael, would you take a look? Thanks!
Attachment #8678942 - Flags: review?(mhenretty)
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.
Attachment #8678942 - Flags: review?(mhenretty) → review+
(Assignee)

Comment 26

3 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!?
Flags: needinfo?(mhenretty)
(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.
Flags: needinfo?(mhenretty)
(Assignee)

Comment 28

3 years ago
master: https://github.com/mozilla-b2g/gaia/commit/717ee46c64fde7d69a1146f3c54e95b2ef20e706
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 29

3 years ago
Alberto, could you help to make sure this patch in v2.5 branch? Thanks.
status-b2g-v2.5: --- → unaffected
status-b2g-master: --- → fixed
Flags: needinfo?(apastor)
(Reporter)

Comment 30

3 years ago
Yes, it's there.
status-b2g-v2.5: unaffected → fixed
Flags: needinfo?(apastor)

Comment 31

3 years ago
Got it. Thanks.
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.