Stack overflow with cyclic menu contribution (#13036)

eclipse-theia · Jan 12, 2024 · d3a1284 · d3a1284
1 parent 160dce9
commit d3a1284
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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) {