Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: keep references to active menus to prevent gc #19427

Merged

Conversation

@CezaryKulakowski
Copy link
Contributor

@CezaryKulakowski CezaryKulakowski commented Jul 24, 2019

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

Without this such menus would be destroyed by js garbage collector even
when they are still displayed.
@codebytere
Copy link
Member

@codebytere codebytere commented Jul 25, 2019

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

Loading

@CezaryKulakowski
Copy link
Contributor Author

@CezaryKulakowski CezaryKulakowski commented Jul 26, 2019

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

Loading

Copy link
Member

@codebytere codebytere left a comment

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)

Loading

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

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

Loading

@deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Jul 30, 2019

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.

Loading

Copy link
Member

@deepak1556 deepak1556 left a comment

Blocking to get consensus

Loading

@zcbenz
Copy link
Member

@zcbenz zcbenz commented Aug 5, 2019

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.

Loading

zcbenz
zcbenz approved these changes Aug 5, 2019
Copy link
Member

@zcbenz zcbenz left a comment

I'm good with current solution.

Loading

@codebytere codebytere requested a review from deepak1556 Aug 5, 2019
@codebytere codebytere changed the title chore: keep references to active menus created by api Menu chore: keep references to active menus to prevent gc Aug 6, 2019
@codebytere codebytere merged commit 50cc54e into electron:master Aug 6, 2019
8 of 9 checks passed
Loading
@release-clerk
Copy link

@release-clerk release-clerk bot commented Aug 6, 2019

Release Notes Persisted

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

Loading

@codebytere
Copy link
Member

@codebytere codebytere commented Jan 17, 2020

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

Loading

@trop
Copy link
Contributor

@trop trop bot commented Jan 17, 2020

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

Loading

@trop
Copy link
Contributor

@trop trop bot commented Jan 17, 2020

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

Loading

@trop
Copy link
Contributor

@trop trop bot commented Jan 17, 2020

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

Loading

@trop
Copy link
Contributor

@trop trop bot commented Jan 17, 2020

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

Loading

@codebytere
Copy link
Member

@codebytere codebytere commented Jan 17, 2020

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

Loading

@CezaryKulakowski
Copy link
Contributor Author

@CezaryKulakowski CezaryKulakowski commented Jan 17, 2020

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

Loading

@CezaryKulakowski
Copy link
Contributor Author

@CezaryKulakowski CezaryKulakowski commented Jan 20, 2020

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?

Loading

@codebytere
Copy link
Member

@codebytere codebytere commented Feb 11, 2020

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

Loading

@trop
Copy link
Contributor

@trop trop bot commented Feb 11, 2020

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

Loading

@trop
Copy link
Contributor

@trop trop bot commented Feb 11, 2020

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants