chore: keep references to active menus to prevent gc

[CezaryKulakowski]

Fixes #19424.

How to reproduce

Launch this simple application:

const { app, BrowserWindow, Menu } = require('electron')

function createWindow () {
  let win = new BrowserWindow({
    width: 800,
    height: 600
  })
  let menu = Menu.buildFromTemplate(
    [
      { label: 'one', accelerator: 'Control+1' },
      { label: 'two', accelerator: 'Control+2' }
    ]
  )
  menu.popup({ "x": 50, "y": 50})
}

app.on('ready', createWindow)

After it's started press arrow down to cycle through both menu's entries.

Expected behavior

Nothing bad happens.

Actual behavior

After few seconds - when js's garbage collector is started - menu is being destroyed. Additionaly crash may occur (~50% reproducibility rate).

As object menu is not kept as global reference in js garbage collector destroys it. It's a matter of debate if that's correct behaviour (on one side object is "local" on js side but on the other menu is active and displayed). The bigger issue is that js object is being destroyed while c++ object is still alive and active. If user interacts with it (like changing focus of current element) when js object is already destroyed crash in v8 will occur. We get notification on c++ side that js object was destroyed by callback but the problem is that - by design - all instances of class WrappableBase, like Menu, are destroyed in two steps: first WrappableBase::FirstWeakCallback is called where we clear wrapper to js object, and after that WrappableBase::SecondWeakCallback is called where we destroy c++ object. Crash occurs when there is any call to Menu (like Menu::GetAcceleratorTextAt after focused element was changed) after first callback but before second callback was called.

With proposed fix we keep references to all currently displayed menus so garbage collector doesn't destroy active menus. If we decide that this is expected behaviour that popup can be destroyed by js garbage collector even when it's still displayed the solution would be to check if GetWrapper() doesn't return empty object in all calls in class Menu (e.g. Menu::IsCommandIdChecked, Menu::GetAcceleratorTextAt, Menu::IsCommandIdEnabled ...).

notes: Don't destroy active menus created as local objects in javascript

[codebytere]

@CezaryKulakowski thanks for this! Before we review it, would you please fill out the template properly with a more complete description, a filled template, and Release Notes as described in the template comment? We'll be able to review more effectively once that has been completed.

[CezaryKulakowski]

@codebytere please let me know if updated description is sufficient :).

[codebytere]

Agree that menus definitely shouldn't be gc-able while open, and it's probably not the best idea to tell users to retain the reference globally, so I think this is the best solution for now.

cc @MarshallOfSound for second 💭

(failure is expected for forks, i'll go away if this PR is rebased though it's not necessary to do prior to merge)

[MarshallOfSound]

Approving based on @deepak1556 's conclusion this won't leak 👍

[deepak1556]

it's probably not the best idea to tell users to retain the reference globally

Coming back to the original problem outlined, I would say the current behavior expecting a global reference for menu aligns with what we expect for BrowserWindow. These are objects that are meant to be maintained for the lifetime of the app or however the user decides in the main process, tying it to a particular scope would be difficult. Being explicit about this behavior is fine IMO.

If we decide that this is expected behaviour that popup can be destroyed by js garbage collector even when it's still displayed the solution would be to check if GetWrapper() doesn't return empty object in all calls in class

@CezaryKulakowski this sounds like a good thing to have.

/cc @zcbenz who wrote the original implementation.

[deepak1556]

Blocking to get consensus

[zcbenz]

If we decide that this is expected behaviour that popup can be destroyed by js garbage collector even when it's still displayed the solution would be to check if GetWrapper() doesn't return empty object in all calls in class Menu

It is hard to make things still work correctly when GetWrapper() is empty, if we return empty results in menu methods then the menu may end up in weird state, if we still execute the delegate methods then JS exceptions may happen as the menu object is gone.

[zcbenz]

I'm good with current solution.

[release-clerk]

Release Notes Persisted

Don't destroy active menus created as local objects in javascript

[codebytere]

/trop run backport-to 7-1-x,6-1-x

[trop]

The backport process for this PR has been manually initiated -
sending your commits to "7-1-x"!

[trop]

The backport process for this PR has been manually initiated -
sending your commits to "6-1-x"!

[trop]

I was unable to backport this PR to "7-1-x" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "6-1-x" cleanly;
you will need to perform this backport manually.

[codebytere]

@CezaryKulakowski could you potentially backport this to the above branches? I can try to get to it if not

[CezaryKulakowski]

@codebytere I will take care of this today or on Monday.

[CezaryKulakowski]

I've checked that this issue doesn't reproduce either on 6-1-x or 7-1-x. @codebytere do you have another test case for this?

[codebytere]

@CezaryKulakowski the original issue was opened for 6 though? either way i think we should backport for safety's sake. I'll handle it.

[trop]

@codebytere has manually backported this PR to "7-1-x", please check out #22151

[trop]

@codebytere has manually backported this PR to "6-1-x", please check out #22152