[TV][2.5] Browser_contextmenu is not killed correctly when its app window is killed

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jocheng, Assigned: lchang)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.5+, b2g-v2.5 fixed, b2g-master fixed)

Details

(Whiteboard: [ft:conndevices][partner-cherry-pick][partner-blocker])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
We implemented a kill() function but app_window calls destory() to remove its submodule. Need to figure out which function name is correct.
(Reporter)

Updated

3 years ago
Blocks: 1214243
blocking-b2g: --- → 2.5+
status-b2g-v2.5: --- → affected
status-b2g-master: --- → affected
(Reporter)

Updated

3 years ago
Whiteboard: [ft:conndevices][partner-cherry-pick][partner-blocker]
(Reporter)

Updated

3 years ago
Assignee: nobody → lchang
Priority: -- → P1
(Reporter)

Updated

3 years ago
Flags: needinfo?(lchang)
Created attachment 8707321 [details] [review]
[gaia] luke-chang:1238887_exit_on_contextmenu > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
Comment on attachment 8707321 [details] [review]
[gaia] luke-chang:1238887_exit_on_contextmenu > mozilla-b2g:master

Hi Rex,

I noticed the "destroy" function is implemented in BaseUI from which BrowserContextMenu inherits but the "kill" function of BrowserContextMenu seems not to be called anywhere. So we decide to rename "kill" to "_destroy" in BrowserContextMenu (and AppModalDialog as well) and make it be called in the "destroy" function in BaseUI.

After patching like above, the sub modules can be destructed correctly but I encountered a longstanding bug in FocusManager due to some racing conditions. So I have to fix it as well.

Please take a look at this patch. Thanks in advance.
Flags: needinfo?(lchang)
Attachment #8707321 - Flags: review?(rexboy)
Comment on attachment 8707321 [details] [review]
[gaia] luke-chang:1238887_exit_on_contextmenu > mozilla-b2g:master

Looks good to me.
AppWindow has kill() but seems submodules don't need that?
Attachment #8707321 - Flags: review?(rexboy) → review+
(Assignee)

Comment 4

3 years ago
Rex, Thanks and yes, it affects sub-modules only.


tests passed: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=8c256c8c73d57b0401de2c90b11dec1ccaff3cf9

landed on master: https://github.com/mozilla-b2g/gaia/commit/87fb9a4c60793fad569894bd888cfbe0d36e294f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 5

3 years ago
Comment on attachment 8707321 [details] [review]
[gaia] luke-chang:1238887_exit_on_contextmenu > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 2.5 feature broken
[User impact] if declined: there's a javascript error that causes context menu broken.
[Testing completed]: yes.
[Risk to taking this patch] (and alternatives if risky): low.
[String changes made]: N/A
Attachment #8707321 - Flags: approval-gaia-v2.5?
(Reporter)

Comment 6

3 years ago
Comment on attachment 8707321 [details] [review]
[gaia] luke-chang:1238887_exit_on_contextmenu > mozilla-b2g:master

Approve for TV 2.5
Attachment #8707321 - Flags: approval-gaia-v2.5? → approval-gaia-v2.5+
You need to log in before you can comment on or make changes to this bug.