From 6e13c1ec56ab3c85036e6460d95bfe51e9e88fd3 Mon Sep 17 00:00:00 2001 From: wonfen Date: Wed, 7 Oct 2026 10:14:00 +0800 Subject: [PATCH 1/4] fix(windows): recycle menu command ids to keep them within 16 bits --- src/platform_impl/windows/mod.rs | 92 +++++++++++++++++++++++++++++--- 1 file changed, 86 insertions(+), 6 deletions(-) diff --git a/src/platform_impl/windows/mod.rs b/src/platform_impl/windows/mod.rs index 727a900e..a8ca64f3 100644 --- a/src/platform_impl/windows/mod.rs +++ b/src/platform_impl/windows/mod.rs @@ -16,14 +16,15 @@ use crate::{ accelerator::MenuAccelerator, dpi::Position, items::{IconType, MenuItemAction, PredefinedMenuItemType}, - util::{AddOp, Counter}, + util::AddOp, AboutMetadata, MenuEvent, MenuTheme, }; use std::{ cell::RefCell, - collections::{HashMap, HashSet}, + collections::{HashMap, HashSet, VecDeque}, rc::Rc, + sync::{Mutex, PoisonError}, }; use windows_sys::Win32::{ Foundation::{FALSE, HWND, LPARAM, LRESULT, POINT, WPARAM}, @@ -35,7 +36,54 @@ use windows_sys::Win32::{ type Hwnd = isize; /// Internal command ids. Used for the `WM_COMMAND` message to identify which menu item was clicked. -static COUNTER: Counter = Counter::new_with_start(1000); +static COMMAND_IDS: Mutex = Mutex::new(CommandIds::new()); + +/// `WM_COMMAND` and `ACCEL::cmd` carry only 16 bits, so an id above `u16::MAX` can never be +/// matched. Ids of dropped items are therefore recycled, oldest first, once fresh ones run out. +struct CommandIds { + next: u32, + free: VecDeque, +} + +impl CommandIds { + const fn new() -> Self { + Self { + next: 1000, + free: VecDeque::new(), + } + } + + fn next(&mut self) -> u32 { + if self.next > u16::MAX as u32 { + if let Some(id) = self.free.pop_front() { + return id; + } + } + let id = self.next; + self.next = self.next.saturating_add(1); + id + } + + fn release(&mut self, id: u32) { + if id <= u16::MAX as u32 { + self.free.push_back(id); + } + } +} + +fn next_command_id() -> u32 { + COMMAND_IDS + .lock() + .unwrap_or_else(PoisonError::into_inner) + .next() +} + +fn release_command_id(id: u32) { + COMMAND_IDS + .lock() + .unwrap_or_else(PoisonError::into_inner) + .release(id); +} #[cfg(feature = "snapshot")] pub(crate) fn dispatch_on_main_thread(f: F) @@ -180,6 +228,8 @@ impl Drop for PlatformMenu { DestroyMenu(self.hmenu); DestroyMenu(self.hpopupmenu); } + + release_command_id(self.id); } } @@ -237,12 +287,14 @@ impl Drop for PlatformMenuItem { for entry in self.accelerator_tables.values() { entry.table.borrow_mut().remove(id) } + + release_command_id(self.id); } } impl PlatformMenu { pub fn new() -> Self { - let id = COUNTER.next(); + let id = next_command_id(); Self { id, hmenu: unsafe { CreateMenu() }, @@ -433,7 +485,7 @@ impl PlatformMenu { impl PlatformMenuItem { pub fn new(click: MenuItemAction) -> Self { Self { - id: COUNTER.next(), + id: next_command_id(), click, parents: Vec::new(), accelerator_tables: HashMap::new(), @@ -448,7 +500,7 @@ impl PlatformMenuItem { pub fn new_submenu(click: MenuItemAction) -> Self { Self { - id: COUNTER.next(), + id: next_command_id(), click, parents: Vec::new(), accelerator_tables: HashMap::new(), @@ -1276,3 +1328,31 @@ fn show_about_dialog(hwnd: Hwnd, metadata: &AboutMetadata) { }); } } + +#[cfg(test)] +mod tests { + use super::CommandIds; + use std::collections::{HashSet, VecDeque}; + + #[test] + fn command_ids_stay_unique_and_within_wm_command_range_under_churn() { + // Start just below the limit so the churn crosses it quickly (this also runs under Miri). + let mut ids = CommandIds { + next: u16::MAX as u32 - 100, + free: VecDeque::new(), + }; + let mut live = VecDeque::new(); + let mut live_set = HashSet::new(); + for _ in 0..2_000 { + let id = ids.next(); + assert!(id <= u16::MAX as u32, "{id} does not fit in WM_COMMAND"); + assert!(live_set.insert(id), "{id} handed out while still live"); + live.push_back(id); + if live.len() > 50 { + let released = live.pop_front().unwrap(); + live_set.remove(&released); + ids.release(released); + } + } + } +} From c581061514a920a91bbe2878e681500e89a25fdc Mon Sep 17 00:00:00 2001 From: wonfen Date: Wed, 7 Oct 2026 23:46:09 +0800 Subject: [PATCH 2/4] chore(macos): drop needless borrow flagged by clippy 1.99 --- src/platform_impl/macos/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform_impl/macos/mod.rs b/src/platform_impl/macos/mod.rs index 2d99fa12..cc15f951 100644 --- a/src/platform_impl/macos/mod.rs +++ b/src/platform_impl/macos/mod.rs @@ -582,7 +582,7 @@ impl PlatformMenuItem { .any(|owned| std::ptr::eq(menu, &*owned.1)) }; - if app.windowsMenu().as_deref().is_some_and(&owns_menu) { + if app.windowsMenu().as_deref().is_some_and(owns_menu) { app.setWindowsMenu(None); } if app.helpMenu().as_deref().is_some_and(owns_menu) { From 64ff6a9a70c14b95bf609a178dfca71a9a9d96b3 Mon Sep 17 00:00:00 2001 From: amrbashir Date: Thu, 8 Oct 2026 07:05:37 +0300 Subject: [PATCH 3/4] wrap command ids instead of recycling dropped ones --- .changes/windows-wrap-command-ids.md | 5 + src/platform_impl/windows/mod.rs | 166 +++++++++------------------ src/platform_impl/windows/util.rs | 38 +++++- 3 files changed, 99 insertions(+), 110 deletions(-) create mode 100644 .changes/windows-wrap-command-ids.md diff --git a/.changes/windows-wrap-command-ids.md b/.changes/windows-wrap-command-ids.md new file mode 100644 index 00000000..339adfca --- /dev/null +++ b/.changes/windows-wrap-command-ids.md @@ -0,0 +1,5 @@ +--- +"muda": patch +--- + +On Windows, Fix menu items going dead after a process has created about 64.5k menus and items. diff --git a/src/platform_impl/windows/mod.rs b/src/platform_impl/windows/mod.rs index a8ca64f3..4d37a4ac 100644 --- a/src/platform_impl/windows/mod.rs +++ b/src/platform_impl/windows/mod.rs @@ -9,22 +9,21 @@ mod util; pub(crate) use self::icon::WinIcon as PlatformIcon; use self::{ dark_menu_bar::{WM_UAHDRAWMENU, WM_UAHDRAWMENUITEM}, - util::Owned, + util::{CounterU16, Owned}, }; use crate::{ accelerator::MenuAccelerator, dpi::Position, items::{IconType, MenuItemAction, PredefinedMenuItemType}, - util::AddOp, + util::{AddOp, Counter}, AboutMetadata, MenuEvent, MenuTheme, }; use std::{ cell::RefCell, - collections::{HashMap, HashSet, VecDeque}, + collections::{HashMap, HashSet}, rc::Rc, - sync::{Mutex, PoisonError}, }; use windows_sys::Win32::{ Foundation::{FALSE, HWND, LPARAM, LRESULT, POINT, WPARAM}, @@ -35,55 +34,30 @@ use windows_sys::Win32::{ /// Type alias for a window handle (HWND) in Windows. type Hwnd = isize; -/// Internal command ids. Used for the `WM_COMMAND` message to identify which menu item was clicked. -static COMMAND_IDS: Mutex = Mutex::new(CommandIds::new()); +/// The first identifier Win32 reserves for window menu (system) commands. +const FIRST_SYSTEM_COMMAND_ID: u16 = SC_SIZE as u16; -/// `WM_COMMAND` and `ACCEL::cmd` carry only 16 bits, so an id above `u16::MAX` can never be -/// matched. Ids of dropped items are therefore recycled, oldest first, once fresh ones run out. -struct CommandIds { - next: u32, - free: VecDeque, -} - -impl CommandIds { - const fn new() -> Self { - Self { - next: 1000, - free: VecDeque::new(), - } - } - - fn next(&mut self) -> u32 { - if self.next > u16::MAX as u32 { - if let Some(id) = self.free.pop_front() { - return id; - } - } - let id = self.next; - self.next = self.next.saturating_add(1); - id - } - - fn release(&mut self, id: u32) { - if id <= u16::MAX as u32 { - self.free.push_back(id); - } - } -} +/// Command ids, used for the `WM_COMMAND` message to identify which menu item was clicked. Only +/// handed out to items `WM_COMMAND` can report. +/// +/// The high end stops short of [`FIRST_SYSTEM_COMMAND_ID`]. A `WM_COMMAND` normally never carries +/// an `SC_*` code, since the window menu reports through `WM_SYSCOMMAND` instead, but a host that +/// tracks that menu itself with `TrackPopupMenu` does receive one as a `WM_COMMAND`, on the very +/// window our subclass is watching. So we stay below the line to avoid conflicts with system commands. +/// +/// The low end is only a courtesy: it keeps us off zero, which Win32 spends on a dismissed +/// `TrackPopupMenu` and on every separator, see https://learn.microsoft.com/en-us/windows/win32/menurc/wm-command#menus +static COMMAND_IDS: CounterU16 = CounterU16::new_with_range(1000, FIRST_SYSTEM_COMMAND_ID - 1); -fn next_command_id() -> u32 { - COMMAND_IDS - .lock() - .unwrap_or_else(PoisonError::into_inner) - .next() -} +/// The id a submenu holds in place of a command id, Win32 addresses a submenu by the container it +/// owns, and never reports one as a command. [`COMMAND_IDS`] starts above zero, so this can never +/// collide with the id of an item that does take commands. +const NO_COMMAND_ID: u16 = 0; -fn release_command_id(id: u32) { - COMMAND_IDS - .lock() - .unwrap_or_else(PoisonError::into_inner) - .release(id); -} +/// Identities of root menus, used to key the accelerator table each item shares with the menus it +/// is attached to. These never reach Win32, so unlike command ids they must not wrap: two live +/// menus sharing an identity would corrupt that bookkeeping. +static MENU_IDS: Counter = Counter::new_with_start(1); #[cfg(feature = "snapshot")] pub(crate) fn dispatch_on_main_thread(f: F) @@ -105,16 +79,17 @@ pub(crate) fn run_predefined_action(_item_type: &crate::items::PredefinedMenuIte /// and also by all submenus and items of the menu so that they can add and remove their own accelerators. struct AcceleratorTable { handle: HACCEL, - entries: HashMap, + /// Keyed by the command id in each entry's `ACCEL::cmd`. + entries: HashMap, } impl AcceleratorTable { - fn insert(&mut self, id: u32, accel: ACCEL) { + fn insert(&mut self, id: u16, accel: ACCEL) { self.entries.insert(id, accel); self.rebuild(); } - fn remove(&mut self, id: u32) { + fn remove(&mut self, id: u16) { if self.entries.remove(&id).is_some() { self.rebuild(); } @@ -169,6 +144,7 @@ struct ParentMenu { /// A root menu bar that can be attached to a window. pub(crate) struct PlatformMenu { + /// Identity in the accelerator tables of this menu's items. Not a command id. id: u32, hmenu: HMENU, hpopupmenu: HMENU, @@ -228,14 +204,13 @@ impl Drop for PlatformMenu { DestroyMenu(self.hmenu); DestroyMenu(self.hpopupmenu); } - - release_command_id(self.id); } } /// A menu item that can be attached to a menu or submenu. pub(crate) struct PlatformMenuItem { - id: u32, + /// The command id `WM_COMMAND` reports a click with, or [`NO_COMMAND_ID`] for a submenu. + command_id: u16, /// What a click does. click: MenuItemAction, parents: Vec, @@ -283,18 +258,15 @@ impl Drop for PlatformMenuItem { } // 5. Remove the item from all accelerator tables it is in. - let id = self.id(); for entry in self.accelerator_tables.values() { - entry.table.borrow_mut().remove(id) + entry.table.borrow_mut().remove(self.command_id) } - - release_command_id(self.id); } } impl PlatformMenu { pub fn new() -> Self { - let id = next_command_id(); + let id = MENU_IDS.next(); Self { id, hmenu: unsafe { CreateMenu() }, @@ -471,7 +443,7 @@ impl PlatformMenu { )]) } - fn find_by_id(&self, id: u32) -> Option>> { + fn find_by_id(&self, id: u16) -> Option>> { find_by_id(id, &self.children) } @@ -485,7 +457,7 @@ impl PlatformMenu { impl PlatformMenuItem { pub fn new(click: MenuItemAction) -> Self { Self { - id: next_command_id(), + command_id: COMMAND_IDS.next(), click, parents: Vec::new(), accelerator_tables: HashMap::new(), @@ -500,7 +472,8 @@ impl PlatformMenuItem { pub fn new_submenu(click: MenuItemAction) -> Self { Self { - id: next_command_id(), + // A submenu is never the target of a `WM_COMMAND`, so it takes no command id. + command_id: NO_COMMAND_ID, click, parents: Vec::new(), accelerator_tables: HashMap::new(), @@ -519,7 +492,7 @@ impl PlatformMenuItem { /// what `MF_POPUP` inserts, so it is what `MF_BYCOMMAND` matches. fn id(&self) -> u32 { if self.hmenu.is_null() { - self.id + u32::from(self.command_id) } else { self.hmenu as u32 } @@ -623,10 +596,8 @@ impl PlatformMenuItem { text: &str, accelerator: Option<&MenuAccelerator>, ) -> crate::Result<()> { - let id = self.id(); - let accel = accelerator - .map(|accelerator| accelerator.to_accel(id as _)) + .map(|accelerator| accelerator.to_accel(self.command_id)) .transpose()?; self.set_text(text, accelerator); @@ -636,8 +607,8 @@ impl PlatformMenuItem { let mut table = entry.table.borrow_mut(); match accel { - Some(accel) => table.insert(id, accel), - None => table.remove(id), + Some(accel) => table.insert(self.command_id, accel), + None => table.remove(self.command_id), } } @@ -725,7 +696,7 @@ impl PlatformMenuItem { unregister_accelerator_tables_from_subtree(&mut child, &self.accelerator_tables); } - fn find_by_id(&self, id: u32) -> Option>> { + fn find_by_id(&self, id: u16) -> Option>> { find_by_id(id, self.children.as_deref().unwrap_or_default()) } @@ -801,7 +772,7 @@ fn attach_item( let accel = args .accelerator .as_ref() - .map(|accelerator| accelerator.to_accel(id as _)) + .map(|accelerator| accelerator.to_accel(child.command_id)) .transpose()?; child.accel = accel; @@ -862,7 +833,6 @@ fn register_accelerator_tables_for_subtree( child: &mut PlatformMenuItem, tables: &HashMap, ) { - let id = child.id(); let accel = child.accel; for (&menu_id, parent_entry) in tables { @@ -874,6 +844,7 @@ fn register_accelerator_tables_for_subtree( if entry.count == 0 { if let Some(accel) = accel { + let id = child.command_id; parent_entry.table.borrow_mut().insert(id, accel); } } @@ -892,15 +863,13 @@ fn unregister_accelerator_tables_from_subtree( child: &mut PlatformMenuItem, tables: &HashMap, ) { - let id = child.id(); - for (menu_id, parent_entry) in tables { if let Some(entry) = child.accelerator_tables.get_mut(menu_id) { entry.count = entry.count.saturating_sub(parent_entry.count); if entry.count == 0 { let entry = child.accelerator_tables.remove(menu_id).unwrap(); - entry.table.borrow_mut().remove(id); + entry.table.borrow_mut().remove(child.command_id); } } } @@ -932,14 +901,19 @@ fn forget_one_parent(hmenu: HMENU, parents: &mut Vec) { } } -/// Finds a menu item by its ID in the specified children and their descendants. +/// Finds a menu item by its command id in the specified children and their descendants. fn find_by_id( - id: u32, + id: u16, children: &[Rc>], ) -> Option>> { + // Skip, see [`NO_COMMAND_ID`] for more details. + if id == NO_COMMAND_ID { + return None; + } + for child in children { let item = child.borrow(); - if item.id == id { + if item.command_id == id { return Some(child.clone()); } @@ -953,7 +927,7 @@ fn find_by_id( } /// Shows a context menu at the specified position, or at the current cursor position if no position is specified. -unsafe fn show_context_menu(hwnd: HWND, hmenu: HMENU, position: Option) -> Option { +unsafe fn show_context_menu(hwnd: HWND, hmenu: HMENU, position: Option) -> Option { let pt = if let Some(pos) = position { let dpi = util::hwnd_dpi(hwnd); let scale_factor = util::dpi_to_scale_factor(dpi); @@ -982,6 +956,8 @@ unsafe fn show_context_menu(hwnd: HWND, hmenu: HMENU, position: Option std::ptr::null(), ); + // Zero is the documented return for a menu the user dismissed, and `TPM_RETURNCMD` hands back + // the full stored identifier, so anything a command id cannot hold is not one of ours. (result > 0).then_some(result.try_into().ok()).flatten() } @@ -1042,7 +1018,7 @@ unsafe extern "system" fn menu_subclass_proc( } WM_COMMAND => { - let id = util::LOWORD(wparam as _) as u32; + let id = util::LOWORD(wparam as _); let item = match uidsubclass { MENU_SUBCLASS_ID => { @@ -1328,31 +1304,3 @@ fn show_about_dialog(hwnd: Hwnd, metadata: &AboutMetadata) { }); } } - -#[cfg(test)] -mod tests { - use super::CommandIds; - use std::collections::{HashSet, VecDeque}; - - #[test] - fn command_ids_stay_unique_and_within_wm_command_range_under_churn() { - // Start just below the limit so the churn crosses it quickly (this also runs under Miri). - let mut ids = CommandIds { - next: u16::MAX as u32 - 100, - free: VecDeque::new(), - }; - let mut live = VecDeque::new(); - let mut live_set = HashSet::new(); - for _ in 0..2_000 { - let id = ids.next(); - assert!(id <= u16::MAX as u32, "{id} does not fit in WM_COMMAND"); - assert!(live_set.insert(id), "{id} handed out while still live"); - live.push_back(id); - if live.len() > 50 { - let released = live.pop_front().unwrap(); - live_set.remove(&released); - ids.release(released); - } - } - } -} diff --git a/src/platform_impl/windows/util.rs b/src/platform_impl/windows/util.rs index aa891635..7f5bd7c4 100644 --- a/src/platform_impl/windows/util.rs +++ b/src/platform_impl/windows/util.rs @@ -2,7 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-License-Identifier: MIT -use std::sync::LazyLock; +use std::sync::{ + atomic::{AtomicU16, Ordering}, + LazyLock, +}; use windows_sys::{ core::HRESULT, Win32::{ @@ -74,6 +77,39 @@ impl Free for HBITMAP { } } +/// A counter similar to [crate::util::Counter] but wraps around instead of leaving its range. +pub struct CounterU16 { + start: u16, + end: u16, + current: AtomicU16, +} + +impl CounterU16 { + /// Counts over `start..=end`, inclusive of both. + pub const fn new_with_range(start: u16, end: u16) -> Self { + assert!(start < end, "a counter needs a range of at least two ids"); + + Self { + start, + end, + current: AtomicU16::new(start), + } + } + + pub fn next(&self) -> u16 { + let previous = self + .current + .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |id| { + Some(if id == self.end { self.start } else { id + 1 }) + }); + + // The update closure always returns `Some`, so both arms hold the previous value. + match previous { + Ok(id) | Err(id) => id, + } + } +} + pub fn encode_wide>(string: S) -> Vec { std::os::windows::prelude::OsStrExt::encode_wide(string.as_ref()) .chain(std::iter::once(0)) From c1cbc087e84b7bb3290cfce513ec9449250ed50b Mon Sep 17 00:00:00 2001 From: amrbashir Date: Thu, 8 Oct 2026 07:12:56 +0300 Subject: [PATCH 4/4] clippy --- src/platform_impl/windows/util.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/platform_impl/windows/util.rs b/src/platform_impl/windows/util.rs index 7f5bd7c4..b5d1319b 100644 --- a/src/platform_impl/windows/util.rs +++ b/src/platform_impl/windows/util.rs @@ -97,6 +97,9 @@ impl CounterU16 { } pub fn next(&self) -> u16 { + // TODO: remove this deprecated allowance once we raise MSRV to 1.95 + // where we can use `try_update` instead. + #[allow(deprecated)] let previous = self .current .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |id| {