commit 9cbe62581b7f907b2e597173e12dd75b10fce17c
parent 7dde23bd00d6e08168eae48aefd9970f89944330
Author: Marco Bonardo <mbonardo@mozilla.com>
Date: Thu, 23 Oct 2025 12:25:54 +0000
Bug 1995696 - Don't populate Places menus if they create a recursive structure r=places-reviewers,Standard8
Differential Revision: https://phabricator.services.mozilla.com/D269599
Diffstat:
3 files changed, 198 insertions(+), 0 deletions(-)
diff --git a/browser/components/places/content/browserPlacesViews.js b/browser/components/places/content/browserPlacesViews.js
@@ -795,6 +795,15 @@ class PlacesViewBase {
}
if (popup._placesNode && PlacesUIUtils.getViewForNode(popup) == this) {
+ if (this.#isPopupForRecursiveFolderShortcut(popup)) {
+ // Show as an empty container for now. We may want to show a better
+ // message in the future, but since we are likely to remove recursive
+ // shortcuts in maintenance at a certain point, this should be enough.
+ this._setEmptyPopupStatus(popup, true);
+ popup._built = true;
+ return;
+ }
+
if (!popup._placesNode.containerOpen) {
popup._placesNode.containerOpen = true;
}
@@ -817,6 +826,33 @@ class PlacesViewBase {
aObject.removeEventListener(aEventNames[i], this, aCapturing);
}
}
+
+ /**
+ * Walks up the parent chain to detect whether a folder shortcut resolves to
+ * a folder already present in the ancestry.
+ *
+ * @param {DOMElement} popup
+ * @returns {boolean} Whether this popup is for a recursive folder shortcut.
+ */
+ #isPopupForRecursiveFolderShortcut(popup) {
+ if (
+ !popup._placesNode ||
+ !PlacesUtils.nodeIsFolderOrShortcut(popup._placesNode)
+ ) {
+ return false;
+ }
+ let guid = PlacesUtils.getConcreteItemGuid(popup._placesNode);
+ for (
+ let parentView = popup.parentNode?.parentNode;
+ parentView?._placesNode;
+ parentView = parentView.parentNode?.parentNode
+ ) {
+ if (PlacesUtils.getConcreteItemGuid(parentView._placesNode) == guid) {
+ return true;
+ }
+ }
+ return false;
+ }
}
/**
diff --git a/browser/components/places/tests/browser/browser.toml b/browser/components/places/tests/browser/browser.toml
@@ -211,6 +211,8 @@ https_first_disabled = true
["browser_paste_resets_cut_highlights.js"]
+["browser_recursive_hierarchies.js"]
+
["browser_remove_bookmarks.js"]
["browser_sidebar_bookmarks_telemetry.js"]
diff --git a/browser/components/places/tests/browser/browser_recursive_hierarchies.js b/browser/components/places/tests/browser/browser_recursive_hierarchies.js
@@ -0,0 +1,160 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Verify that menu popups are not populated when doing so would create a
+ * recursive hierarchy. Some consumers may walk menu trees and could enter an
+ * infinite loop if recursion were allowed.
+ */
+
+"use strict";
+
+async function openMenuAndWaitForPopup(menu) {
+ let popup = menu.menupopup;
+ let popupShown = BrowserTestUtils.waitForEvent(popup, "popupshown");
+ // This seems necessary, at least for the toolbar.
+ EventUtils.synthesizeNativeMouseEvent({
+ type: "mousemove",
+ target: menu,
+ atCenter: true,
+ });
+ menu.open = true;
+ await popupShown;
+ return popup;
+}
+
+function findNode(guid, view) {
+ for (let node of view.childNodes) {
+ console.log("visiting node", node, node._placesNode?.bookmarkGuid);
+ if (node._placesNode?.bookmarkGuid == guid) {
+ return node;
+ }
+ }
+ return null;
+}
+
+function fakeOpenMenu(popup) {
+ popup.dispatchEvent(new MouseEvent("popupshowing", { bubbles: true }));
+ popup.dispatchEvent(new MouseEvent("popupshown", { bubbles: true }));
+}
+
+function findPlacesNode(guid, container) {
+ for (let i = 0; i < container.childCount; i++) {
+ let node = container.getChild(i);
+ if (node.bookmarkGuid == guid) {
+ return node;
+ }
+ }
+ return null;
+}
+
+add_task(async function test() {
+ // Make sure the bookmarks bar is visible and restore its state on cleanup.
+ let toolbar = document.getElementById("PersonalToolbar");
+ ok(toolbar, "PersonalToolbar should not be null");
+ if (toolbar.collapsed) {
+ await promiseSetToolbarVisibility(toolbar, true);
+ registerCleanupFunction(function () {
+ return promiseSetToolbarVisibility(toolbar, false);
+ });
+ }
+
+ let menubar = document.getElementById("toolbar-menubar");
+ // Force the menu to be shown.
+ const kAutohide = menubar.getAttribute("autohide");
+ menubar.removeAttribute("autohide");
+ registerCleanupFunction(function () {
+ menubar.setAttribute("autohide", kAutohide);
+ });
+
+ registerCleanupFunction(PlacesUtils.bookmarks.eraseEverything);
+
+ // Create a folder structure with a recursive shortcut.
+ let folderA = await PlacesUtils.bookmarks.insert({
+ parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+ title: "Folder A",
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ });
+ let selfShortcut = await PlacesUtils.bookmarks.insert({
+ parentGuid: folderA.guid,
+ title: "Shortcut to A",
+ url: `place:parent=${folderA.guid}`,
+ });
+ let toolbarShortcut = await PlacesUtils.bookmarks.insert({
+ parentGuid: folderA.guid,
+ title: "Shortcut to toolbar",
+ url: `place:parent=${PlacesUtils.bookmarks.toolbarGuid}`,
+ });
+
+ // This also ensures that toolbar content is updated.
+ Assert.ok(!(await PlacesToolbarHelper.getIsEmpty()), "Toolbar is not empty");
+
+ // Open the popup for the shortcut from the bookmarks toolbar.
+ let toolbarItems = document.getElementById("PlacesToolbarItems");
+ let folderANode = findNode(folderA.guid, toolbarItems);
+ let folderAPopup = await openMenuAndWaitForPopup(folderANode);
+ let selfShortcutNode = findNode(selfShortcut.guid, folderAPopup);
+ let selfShortcutPopup = await openMenuAndWaitForPopup(selfShortcutNode);
+
+ Assert.ok(
+ !selfShortcutPopup._placesNode.containerOpen,
+ "Self shortcut container is not open"
+ );
+ Assert.ok(
+ selfShortcutPopup.hasAttribute("emptyplacesresult"),
+ "Self shortcut popup is empty"
+ );
+
+ let toolbarShortcutNode = findNode(
+ toolbarShortcut.guid,
+ folderANode.menupopup
+ );
+ let toolbarShortcutPopup = await openMenuAndWaitForPopup(toolbarShortcutNode);
+
+ Assert.ok(
+ !toolbarShortcutPopup._placesNode.containerOpen,
+ "Toolbar shortcut container is not open"
+ );
+ Assert.ok(
+ toolbarShortcutPopup.hasAttribute("emptyplacesresult"),
+ "Toolbar shortcut popup is empty"
+ );
+
+ // Also insert a toolbar shortcut in the bookmarks menu and check the
+ // previously inserted recursive toolbar shortcut there.
+ info("Test native bookmarks menu");
+ let shortcutInMenu = await PlacesUtils.bookmarks.insert({
+ parentGuid: PlacesUtils.bookmarks.menuGuid,
+ title: "Shortcut to menu",
+ url: `place:parent=${PlacesUtils.bookmarks.menuGuid}`,
+ });
+
+ if (Services.appinfo.nativeMenubar) {
+ // With native menubar we can only simulate popupshowing as we cannot
+ // directly open menus.
+ let bookmarksMenuPopup = document.getElementById("bookmarksMenuPopup");
+ fakeOpenMenu(bookmarksMenuPopup);
+ let shortcutInMenuNode = findNode(shortcutInMenu.guid, bookmarksMenuPopup);
+ fakeOpenMenu(shortcutInMenuNode.menupopup);
+ Assert.ok(
+ !shortcutInMenuNode.menupopup._placesNode.containerOpen,
+ "menu shortcut container is not open"
+ );
+ } else {
+ let bookmarksMenu = document.getElementById("bookmarksMenu");
+ let bookmarksPopup = await openMenuAndWaitForPopup(bookmarksMenu);
+ let shortcutInMenuNode = findNode(shortcutInMenu.guid, bookmarksPopup);
+ let shortcutInMenuPopup = await openMenuAndWaitForPopup(
+ shortcutInMenuNode,
+ true
+ );
+ Assert.ok(
+ !shortcutInMenuPopup._placesNode.containerOpen,
+ "Toolbar shortcut container is not open"
+ );
+ Assert.ok(
+ shortcutInMenuPopup.hasAttribute("emptyplacesresult"),
+ "Toolbar shortcut popup is empty"
+ );
+ }
+});
