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

fix: prevent Menu.buildFromTemplate with empty array #23308

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Apr 28, 2020

Description of Change

Closes #23282.

Prevent issues with menu creation and subsequent pane focus from menu bar by preventing menus from being created from an empty array. I can't conceive a valid use case for this, since if one wants to remove a menu they should be be passing null to win.setMenu() or calling win.removeMenu(). This issue is also specific to top-level menus, and not submenus, so the new check and exception is scoped to top-level menus.

cc @zcbenz @jkleinsc @MarshallOfSound

Checklist

Release Notes

Notes: Fixed a potential crash when menu is created from an empty template.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 28, 2020
@codebytere codebytere changed the title fix: prevent buildFromTemplate with empty array fix: prevent Menu.buildFromTemplate with empty array Apr 28, 2020
@zcbenz
Copy link
Contributor

zcbenz commented Apr 28, 2020

Would the crash in original issue still happen when calling win.setMenu(new Menu)?

@dezsiszabi
Copy link

dezsiszabi commented Apr 28, 2020

Just to give a usecase.

We have a wrapper package around electron window creation/menu creation. It exposes a WindowBuilder, TrayBuilder, MenuBuilder etc (using a fluent API/builder pattern to compose the electron application).

In this wrapper code we called a code similar to

window.setMenu(Menu.buildFromTemplate(this.menuBuilders.map(mb => mb.build())));

Of course the issue should be prevented in our code by adding defensive checks and not call setMenu when there are no menu builders.

That said, I'd still prefer an early failure which doesn't result in a crash. (I mean a crash shouldn't ever happen no matter how insanely the JavaScript API was (ab)used.)

Either:

  1. Fail early by throwing an exception if Menu.buildFromTemplate is called with an empty array
    OR
  2. Menu.buildFromTemplate should return null if called with an empty array.

In both cases the documentation should mention the behavior when Menu.buildFromTemplate is called with an empty array.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 29, 2020
@codebytere
Copy link
Member Author

codebytere commented Apr 29, 2020

@zcbenz it shouldn't from my read of this function, but i could be wrong!

@codebytere codebytere merged commit f50f725 into master Apr 30, 2020
@release-clerk
Copy link

release-clerk bot commented Apr 30, 2020

Release Notes Persisted

Fixed a potential crash when menu is created from an empty template.

@codebytere codebytere deleted the prevent-empty-menu branch April 30, 2020 15:29
@trop
Copy link
Contributor

trop bot commented Apr 30, 2020

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

@trop
Copy link
Contributor

trop bot commented Apr 30, 2020

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

@trop
Copy link
Contributor

trop bot commented Apr 30, 2020

I have automatically backported this PR to "9-x-y", please check out #23351

@trop
Copy link
Contributor

trop bot commented Apr 30, 2020

@codebytere has manually backported this PR to "8-x-y", please check out #23353

@trop
Copy link
Contributor

trop bot commented Apr 30, 2020

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

@codebytere
Copy link
Member Author

New information came to light about how this may break existing codepaths - i'm going to open a new PR that avoids this breakage and backport that.

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

Successfully merging this pull request may close these issues.

Crash when pressing ALT and setMenu is called with Menu.buildFromTemplate([])
3 participants