[electron] Executing the commands from menu items has no effect when the application starts. It worked with 1.36.0

[kittaakos]

Bug Description:

Originally from here: arduino/arduino-ide#2027 (review)

Clicking on the menu items have no effect when the application is starting. This has worked with 1.36.0. Presumably, the breaking change has been introduced with the electron version update and affects only the electron app, but this is just a guess so far: #12299.

Steps to Reproduce:

1. Open Theia Blueprint 1.37.0, and while the app is starting, click on File > New File. Nothing happens until the window is fully loaded and the last setMenu is called.
2. This works perfectly with Theia Blueprint 1.36.0. When I click New File thrice, I will see three new untitled editors once the app is ready.

1.36.0: 😄

menu_items_work_at_startup_1.36.0.mp4

1.37.0: 😞

menu_items_do_not_work_at_startup_1.37.0.mp4

Additional Information

• Operating System: macOS
• Theia Version: 1.37.0 Theia Blueprint

[kittaakos]

Presumably, the breaking change has been introduced with the electron version update and affects only the electron app, but this is just a guess so far: #12299.

Theia drops and recreates the menus on every setMenu call. When the application menu is dropped and a new one is created, the handler IDs change, and it's impossible to execute the command if the menu item (and its handler) is associated with the previous menu.

theia/packages/core/src/electron-browser/preload.ts

Line 69 in 9dd0921

commandHandlers.delete(mainMenuId);

theia/packages/core/src/electron-browser/preload.ts

Line 47 in 9dd0921

handlerId = nextHandlerId++;

theia/packages/core/src/electron-browser/preload.ts

Lines 198 to 204 in 9dd0921

	const map = commandHandlers.get(menuId);
	if (map) {
	const handler = map.get(handlerId);
	if (handler) {
	handler();
	}
	}

[tsmaeder]

@kittaakos now sure what the window of vulnerability is here: are you saying you can click on a menu between the dropping of the command handlers and setting the new menu in the following snippet?

        commandHandlers.delete(mainMenuId);
        const handlers = new Map<number, () => void>();
        commandHandlers.set(mainMenuId, handlers);
        ipcRenderer.send(CHANNEL_SET_MENU, mainMenuId, convertMenu(menu, handlers));

[kittaakos]

here: are you saying you can click on a menu between the dropping of the command handler and setting the new menu?

Thanks for the quick reply. In a nutshell, yes.

The rolling index-based ID generation for the handlers seems a stricter idea, which I like, but it has flaws.

In the Theia Blueprint application, the menu items are dropped and re-created a few times during (and right after) the app startup, and then the menu remains static. (I know, "static" is an oversimplification of things.) The menu might be more dynamic in other Theia-based apps, such as the Arduino IDE. The menu items are dropped and recreated multiple times while the app runs. The likelihood of hitting the bug the PR addresses is less when the menu is static and increases with the number of menu updates.

Problematic scenario:

• Menu created and set for the very first time. The user can use it.
• The user clicks the menu. Let's say, the user clicks the File menu, the OS renders it, and you can see the following items: New File, Save, etc., with the following handler IDs: 0, 1, etc.
• Meanwhile, the menu model gets updated, and the frontend requests a menu update from the electron main. The electron main does it. All looks good at this point; the handler IDs have been updated. Let's assume the File menu structure has not changed; it contains the same New File, Save, etc., but the new handler IDs: are 231, 232, etc. (The menu was open during this.)
• When the user clicks New File, the code wants to execute the handler with ID 0. But the new ID is 231.

I hope it helps you better understand the nature of the problem. Feel free to let me know if you need any other info or if your local testing shows different results. Thanks!

Additional context:

I investigated and patched the bug on macOS, but the defect has been reported on Windows.

[tsmaeder]

Wouldn't a simpler fix be to just keep the old handlers around until the new menu is set on the native side? The trouble with using the menu id's is that we're not guaranteeing the global uniqueness anywhere, so we might end up with handlers being overwritten.

[kittaakos]

The trouble with using the menu id's is that we're not guaranteeing the global uniqueness anywhere, so we might end up with handlers being overwritten.

Do you have a concrete example or is it the code smell? Why do they need to be globally unique in this case? We care about the "clickable" menu items, and all of them are backed by commands. The menu node IDs will be the command IDs. If there are two different menu items backed by the same command, they will do the same thing when the user clicks on them so they can share a handler. I do not see this as a problem but I might be overlooking a use case.

Wouldn't a simpler fix be to just keep the old handlers around until the new menu is set on the native side?

It's probably not simpler because this one is already ready for review, but I am open to any other solution. Which event would tell Theia, OK, you can release the old handlers? Do you have an idea where this additional code should go?

[tsmaeder]

Do you have a concrete example or is it the code smell?

It's not a code smell, it's an API design issue: if we use the id field, we rely on the API client using the id field correctly. That introduces coupling between our command infrastructure and the electron menu API. I am a stickler for not introducing coupling iff we can prevent it. (Rich Hickey has a great and fun talk about why: https://www.youtube.com/watch?v=SxdOUGdseq4 )

Why do they need to be globally unique in this case?

If no-one enforces the menu id's to be unique, you could have two menu items with the same id, but only one handler.

[kittaakos]

Thanks!

Wouldn't a simpler fix be to just keep the old handlers around until the new menu is set on the native side?

Do you have an idea where this additional code should go?

Can you give me some pointers on how you envision it?

[tsmaeder]

I think it might be as simple as this:

let currentMainMenuId = -1;

const api: TheiaCoreAPI = {
    setMenuBarVisible: (visible: boolean, windowName?: string) => ipcRenderer.send(CHANNEL_SET_MENU_BAR_VISIBLE, visible, windowName),
    setMenu: (menu: MenuDto[] | undefined) => {
        const menuId = nextMenuId++;
        const handlers = new Map<number, () => void>();
        commandHandlers.set(menuId, handlers);
        ipcRenderer.invoke(CHANNEL_SET_MENU, menuId, convertMenu(menu, handlers));
        commandHandlers.delete(currentMainMenuId);
        currentMainMenuId = menuId;
    },

and in electron-api-main.ts, we'd have to change the on('CHANNEL_SET_MENU'... to 'handle'

        ipcMain.handle(CHANNEL_SET_MENU, (event, menuId: number, menu: MenuDto[]) => {

that way, we would wait to throw away the old handlers until the new menu is actually set. So any invocations in the mean time would still find their handlers.

[tsmaeder]

Actually, invoke returns a promise we'll have to wait for, so we should probably change the typing of the API to Promise<void> instead of 'void', same as the popup method a bit further down.