js garbage collector destroys active popup menus

[CezaryKulakowski]

Javascript's garbage collector destroys popup menus created by menu.popup from api Menu even when they are still active. It can lead to a crash when during destruction phase (when js object is already destroyed but c++ instance of class electron::api::Menu still lives) user makes any interaction with it, like changing focused element.

[CezaryKulakowski]

To reproduce the problem it's enough to create simple application which creates context menu using api Menu (I've used the one from https://www.npmjs.com/package/electron-context-menu). When menu is opened it will be automatically closed after first run of js garbage collector (it's being launched once a minute). Additionaly if we keep using the menu (like pressing arrow down to cycle through menu entries) crash may occur when menu is going to be destroyed (~50% reproducibility rate).

[CezaryKulakowski]

I've created pr with working solution: #19427

[deermichel]

Do you hold a global reference to the Menu on the JS side? It sounds a bit to me like here where the references lives in local scope and naturally goes out of scope after the function returns.

[CezaryKulakowski]

No. I agree that this problem is caused by a faulty implementation of popup menu in package electron-context-menu, but bad js shouldn't cause a crash in v8.

[CezaryKulakowski]

The other solution would be checking if GetWrapper() isn't null in all calls (for example: in api::Menu::GetAcceleratorForCommandIdWithParams) but I don't know if it's a better solution.

[CezaryKulakowski]

To make a full picture: when js object has been destroyed by gc we get information on c++ side. Then c++ object is being destroyed in two phases: in first run we reset wrapper to js object and in second run we destroy the c++ object. If any call to the menu which still exists and is visible happens after step one but before step two we will get a crash in v8. It's quite easy to reproduce.