From d3a12844150cf6987369fab9b0c00a712671ca17 Mon Sep 17 00:00:00 2001 From: Dennis Huebner Date: Fri, 12 Jan 2024 09:59:03 +0100 Subject: [PATCH] Stack overflow with cyclic menu contribution (#13036) --- .../src/common/menu/menu-model-registry.ts | 27 +++++++++++++--- packages/core/src/common/menu/menu.spec.ts | 32 +++++++++++++++++-- .../menus/menus-contribution-handler.ts | 11 +++++-- 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/packages/core/src/common/menu/menu-model-registry.ts b/packages/core/src/common/menu/menu-model-registry.ts index c29d69ecf3cad..42b4a93ac9b96 100644 --- a/packages/core/src/common/menu/menu-model-registry.ts +++ b/packages/core/src/common/menu/menu-model-registry.ts @@ -14,13 +14,13 @@ // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 // ***************************************************************************** -import { injectable, inject, named } from 'inversify'; -import { Disposable } from '../disposable'; -import { CommandRegistry, Command } from '../command'; +import { inject, injectable, named } from 'inversify'; +import { Command, CommandRegistry } from '../command'; import { ContributionProvider } from '../contribution-provider'; -import { CompositeMenuNode, CompositeMenuNodeWrapper } from './composite-menu-node'; -import { CompoundMenuNode, MenuAction, MenuNode, MenuPath, MutableCompoundMenuNode, SubMenuOptions } from './menu-types'; +import { Disposable } from '../disposable'; import { ActionMenuNode } from './action-menu-node'; +import { CompositeMenuNode, CompositeMenuNodeWrapper } from './composite-menu-node'; +import { CompoundMenuNode, MenuAction, MenuNode, MenuNodeMetadata, MenuPath, MutableCompoundMenuNode, SubMenuOptions } from './menu-types'; export const MenuContribution = Symbol('MenuContribution'); @@ -157,6 +157,23 @@ export class MenuModelRegistry { linkSubmenu(parentPath: MenuPath | string, childId: string | MenuPath, options?: SubMenuOptions, group?: string): Disposable { const child = this.getMenuNode(childId); const parent = this.getMenuNode(parentPath, group); + + const isRecursive = (node: MenuNodeMetadata, childNode: MenuNodeMetadata): boolean => { + if (node.id === childNode.id) { + return true; + } + if (node.parent) { + return isRecursive(node.parent, childNode); + } + return false; + }; + + // check for menu contribution recursion + if (isRecursive(parent, child)) { + console.warn(`Recursive menu contribution detected: ${child.id} is already in hierarchy of ${parent.id}.`); + return Disposable.NULL; + } + const wrapper = new CompositeMenuNodeWrapper(child, parent, options); return parent.addNode(wrapper); } diff --git a/packages/core/src/common/menu/menu.spec.ts b/packages/core/src/common/menu/menu.spec.ts index 78b769c4d0da8..650ae274574d0 100644 --- a/packages/core/src/common/menu/menu.spec.ts +++ b/packages/core/src/common/menu/menu.spec.ts @@ -14,10 +14,10 @@ // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 // ***************************************************************************** -import { CommandContribution, CommandRegistry } from '../command'; -import { MenuContribution, MenuModelRegistry } from './menu-model-registry'; import * as chai from 'chai'; +import { CommandContribution, CommandRegistry } from '../command'; import { CompositeMenuNode } from './composite-menu-node'; +import { MenuContribution, MenuModelRegistry } from './menu-model-registry'; const expect = chai.expect; @@ -61,6 +61,25 @@ describe('menu-model-registry', () => { expect(openGroup.children.length).equals(2); expect(openGroup.label).undefined; }); + + it('Should not allow to register cyclic menus.', () => { + const fileMenu = ['main', 'File']; + const fileOpenMenu = [...fileMenu, '0_open']; + const fileCloseMenu = [...fileMenu, '1_close']; + const service = createMenuRegistry({ + registerMenus(menuRegistry: MenuModelRegistry): void { + menuRegistry.registerSubmenu(fileMenu, 'File'); + // open menu should not be added to open menu + menuRegistry.linkSubmenu(fileOpenMenu, fileOpenMenu); + // close menu should be added + menuRegistry.linkSubmenu(fileOpenMenu, fileCloseMenu); + } + }, { + registerCommands(reg: CommandRegistry): void { } + }); + const all = service.getMenu() as CompositeMenuNode; + expect(menuStructureToString(all.children[0] as CompositeMenuNode)).equals('File(0_open(1_close),1_close())'); + }); }); }); @@ -71,3 +90,12 @@ function createMenuRegistry(menuContrib: MenuContribution, commandContrib: Comma menuReg.onStart(); return menuReg; } + +function menuStructureToString(node: CompositeMenuNode): string { + return node.children.map(c => { + if (c instanceof CompositeMenuNode) { + return `${c.id}(${menuStructureToString(c)})`; + } + return c.id; + }).join(','); +} diff --git a/packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts b/packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts index 2ffb27f5951cd..4c23f436a7b8e 100644 --- a/packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts +++ b/packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts @@ -99,9 +99,12 @@ export class MenusContributionPointHandler { const targets = this.getMatchingMenu(contributionPoint as ContributionPoint) ?? [contributionPoint]; const { group, order } = this.parseGroup(item.group); const { submenu, command } = item; - if (submenu) { - targets.forEach(target => toDispose.push(this.menuRegistry.linkSubmenu(target, submenu!, { order, when: item.when }, group))); - } else if (command) { + if (submenu && command) { + console.warn( + `Menu item ${command} from plugin ${plugin.metadata.model.id} contributed both submenu and command. Only command will be registered.` + ); + } + if (command) { toDispose.push(this.commandAdapter.addCommand(command)); targets.forEach(target => { const node = new ActionMenuNode({ @@ -112,6 +115,8 @@ export class MenusContributionPointHandler { const parent = this.menuRegistry.getMenuNode(target, group); toDispose.push(parent.addNode(node)); }); + } else if (submenu) { + targets.forEach(target => toDispose.push(this.menuRegistry.linkSubmenu(target, submenu!, { order, when: item.when }, group))); } } } catch (error) {