fix(w32): pass HMENU to DestroyMenu syscall#5228
Merged
leaanthony merged 22 commits intoMay 1, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughStaged Windows systray menu rebuilds now allocate into a fresh HMENU/bitmap set, propagate Win32 failures as errors with rollback and cleanup, track and free HBITMAPs, fix a DestroyMenu syscall argument mismatch, clear stale menu state during updates, and add a stress test and README to exercise USER/GDI deltas. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant STray as SystemTray
participant Win32M as Win32Menu
participant W32 as W32 Wrapper
participant WinAPI as Win32 API
App->>STray: SetMenu(newMenu)
STray->>Win32M: create staged HMENU & temp maps
Win32M->>W32: AppendMenu / SetMenuIcons (may allocate HBITMAPs)
alt success
W32-->>Win32M: Return HBITMAP handles
Win32M->>Win32M: store staged bitmaps
Win32M->>STray: swap in staged HMENU/state
Win32M->>W32: DeleteObject(old HBITMAPs)
Win32M->>WinAPI: DestroyMenu(old HMENU)
else failure
Win32M->>W32: DeleteObject(any new HBITMAPs)
Win32M->>WinAPI: DestroyMenu(staged HMENU)
Win32M->>STray: restore prior HMENU/state
end
App->>STray: Show menu
STray->>Win32M: TrackPopupMenuEx -> WinAPI shows popup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/pkg/application/menu_windows.go (1)
52-59:⚠️ Potential issue | 🟡 Minor
update()doesn't resetmenuMapping/currentMenuID, diverging from the parallel fix inpopupmenu_windows.go.
Win32Menu.Update()atpopupmenu_windows.go:193-211recreatesmenuMapping,checkboxItems, andradioGroupson every rebuild so stale pointer-keyed entries don't accumulate. The equivalentwindowsMenu.update()here only frees bitmaps and destroys the HMENU;w.menuMappingandw.currentMenuIDkeep growing across rebuilds. Across long-running churn this is a slow map-leak of*MenuItemreferences, and it also meansfreeBitmapswalks stale entries on each rebuild (currently harmless because eachprocessMenuassigns a freshnewMenuItemImplwithbitmap==0, but fragile if that invariant ever changes).🔧 Proposed fix
func (w *windowsMenu) update() { if w.hMenu != 0 { w.freeBitmaps() w32.DestroyMenu(w.hMenu) } w.hMenu = w32.NewPopupMenu() + w.menuMapping = make(map[int]*MenuItem) + w.currentMenuID = 0 w.processMenu(w.hMenu, w.menu) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/menu_windows.go` around lines 52 - 59, windowsMenu.update currently rebuilds the HMENU but leaves w.menuMapping and w.currentMenuID intact, causing stale pointer-keyed entries to accumulate; modify windowsMenu.update so after freeing bitmaps and destroying/creating the HMENU (i.e., keep freeBitmaps() then DestroyMenu/NewPopupMenu calls) you reinitialize w.menuMapping to a fresh map and reset w.currentMenuID to its initial value (e.g., 0) before calling w.processMenu so the mapping and ID counter don't grow across rebuilds.
🧹 Nitpick comments (8)
v3/pkg/application/popupmenu_windows.go (2)
64-81: Duplicated betweenpopupmenu_windows.goandmenu_windows.go— consider a shared helper.This
freeBitmapsimplementation is essentially identical towindowsMenu.freeBitmapsinv3/pkg/application/menu_windows.go:24-41(same slice walk, samemenuMappingwalk, sameimpl.bitmapclearing). Non-blocking, but a small helper likefreeMenuBitmaps(bitmaps []w32.HBITMAP, mapping map[int]*MenuItem)would keep the two in lockstep — the current duplication makes it easy to fix a bug in one place and miss the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/popupmenu_windows.go` around lines 64 - 81, Extract the duplicated bitmap cleanup logic from Win32Menu.freeBitmaps and windowsMenu.freeBitmaps into a shared helper (e.g., freeMenuBitmaps) that accepts the slice of HBITMAPs and the menuMapping, then update both Win32Menu.freeBitmaps and windowsMenu.freeBitmaps to call freeMenuBitmaps; ensure the helper iterates the provided bitmaps slice and calls w32.DeleteObject, walks the mapping entries, type-asserts to *windowsMenuItem, deletes impl.bitmap via w32.DeleteObject and sets impl.bitmap = 0, and finally clears the input slice/mapping references as the original functions did.
314-317:Destroy()is not idempotent — a second call will re-walkmenuMappingand callDestroyMenu(0).After
Destroy(),p.menuis not zeroed andp.menuMappingisn't cleared. Practically this is safe today (impl.bitmapwas set to 0 by the firstfreeBitmaps, andw32.DestroyMenu(0)returns FALSE without crashing — now actually correctly, thanks to the syscall fix in this PR), but it's fragile.updateMenuinsystemtray_windows.gois already the primary caller and it replacess.menuright after, so this doesn't bite in the current flow.🔧 Optional hardening
func (p *Win32Menu) Destroy() { + if p.menu == 0 { + return + } p.freeBitmaps() w32.DestroyMenu(p.menu) + p.menu = 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/popupmenu_windows.go` around lines 314 - 317, Destroy() on Win32Menu is not idempotent because it leaves p.menu and p.menuMapping populated so repeated calls re-walk and call w32.DestroyMenu(0); change Destroy to be safe by early-returning if p.menu == 0 (or nil equivalent), and after calling p.freeBitmaps() and w32.DestroyMenu(p.menu) set p.menu = 0 and clear/empty p.menuMapping (and any other references to the destroyed menu) so subsequent Destroy calls are no-ops; reference the Win32Menu.Destroy, p.menu, p.menuMapping, freeBitmaps, and updateMenu (systemtray_windows.go) when making the change.v3/pkg/w32/menu_windows_test.go (1)
48-73: Tolerance of 50 over 2000 iterations is reasonable but consider sampling more points for a stronger signal.The single-point start/end delta catches gross leakage but can miss gradual drift that stays under 50 across 2000 iterations yet would exceed it across 50,000. Not blocking, but for a genuinely airtight regression you could (a) sample mid-run and assert the delta is ~stable, or (b) raise iterations and keep tolerance small. The current form is sufficient to catch the pre-fix bug, which is the stated goal.
One minor thing:
getUserObjectCountre-loadsuser32.dllandkernel32.dllon every call viaNewLazyDLL. That's fine for a test that invokes it twice, but if someone later moves it into the hot loop, cache the*LazyProcvalues at package init.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/w32/menu_windows_test.go` around lines 48 - 73, The test TestMenuHandleLifecycleDoesNotLeak currently only samples start/end and calls getUserObjectCount which re-loads user32/kernel32 via NewLazyDLL on each call; to improve robustness, either add a mid-run sampling/assertion (call getUserObjectCount at one or more intermediate iterations and assert delta remains small) or increase iterations while keeping tolerance small, and modify getUserObjectCount to cache the DLL and Proc handles by creating the LazyDLL/LazyProc values at package init (e.g., package-level vars initialized once) instead of calling NewLazyDLL/NewProc on every getUserObjectCount invocation.v3/pkg/application/menu_windows.go (1)
24-41: Double-free hazard if the sameHBITMAPever ends up in bothw.bitmapsandimpl.bitmap.Today the two collections are disjoint (
processMenuonly populatesw.bitmaps;setBitmaponly populatesimpl.bitmap), so this is safe. But the invariant is entirely implicit: a future change that e.g. appends the runtimeSetBitmaphandle intow.bitmaps, or replaysprocessMenuwithout clearingw.bitmaps, would cause a doubleDeleteObjecton the same GDI handle — which is undefined behaviour and hard to diagnose.Consider either (a) a comment explicitly documenting the ownership split, or (b) unifying tracking in a single location (e.g. always store per-item bitmaps on
impl.bitmapand iteratemenuMappingonly). Not blocking — just flagging the trap for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/menu_windows.go` around lines 24 - 41, In freeBitmaps, avoid possible double-free between w.bitmaps and per-item windowsMenuItem.bitmap by deduplicating and deleting each HBITMAP exactly once: in windowsMenu.freeBitmaps build a set keyed by handle (from w.bitmaps and from each item.impl.(*windowsMenuItem).bitmap via w.menuMapping), then iterate the set and call w32.DeleteObject once per unique handle and clear the per-item impl.bitmap and w.bitmaps; alternatively, if you prefer minimal change, add a clear comment in freeBitmaps documenting the current ownership invariant (w.bitmaps vs windowsMenuItem.bitmap) so future changes don’t accidentally cause double DeleteObject.v3/UNRELEASED_CHANGELOG.md (1)
26-26: Optional: reference the linked issue for traceability.The changelog guidelines at the top of this file suggest referencing issue/PR numbers when applicable. Since this entry closes
#5227, adding the reference would help readers trace the fix back to the report.📝 Proposed addition
-- Fix a family of Windows systray `SetMenu` crashes caused by a broken `DestroyMenu` syscall that was passing four arguments instead of one, so every call returned FALSE and freed nothing. Also release HMENU and HBITMAP handles (including those allocated at runtime via `MenuItem.SetBitmap`) on menu rebuilds, reset stale checkbox/radio maps in `Win32Menu.Update`, and drop a redundant `Update()` call in `systemtray.updateMenu` that doubled allocations. Long-running systray apps no longer leak GDI/USER objects on each menu rebuild. +- Fix a family of Windows systray `SetMenu` crashes caused by a broken `DestroyMenu` syscall that was passing four arguments instead of one, so every call returned FALSE and freed nothing. Also release HMENU and HBITMAP handles (including those allocated at runtime via `MenuItem.SetBitmap`) on menu rebuilds, reset stale checkbox/radio maps in `Win32Menu.Update`, and drop a redundant `Update()` call in `systemtray.updateMenu` that doubled allocations. Long-running systray apps no longer leak GDI/USER objects on each menu rebuild. (`#5227`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/UNRELEASED_CHANGELOG.md` at line 26, Update the changelog entry text to reference the related issue/PR by appending a traceability note such as " (closes `#5227`)" or "See `#5227`" to the existing sentence about the Windows systray fix so readers can follow back to the report; edit the UNRELEASED_CHANGELOG.md entry that begins "Fix a family of Windows systray `SetMenu` crashes..." to include the issue/PR number for traceability.v3/examples/systray-stress/main.go (3)
180-187: Nit:logJSONdoesn't emit JSON.The inline comment already acknowledges it's prefix=value. Renaming to
logEvent(or similar) avoids grep confusion and a future reader assuming they can pipe the output intojq.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/examples/systray-stress/main.go` around lines 180 - 187, The function logJSON is misnamed because it emits space-separated key=value output rather than JSON; rename the function (e.g., to logEvent or logKV) and update all call sites to use the new name, adjust the inline comment to describe "prefix=value" logging, and keep the same signature (func logEvent(event string, fields map[string]any)) so behavior and interfaces remain unchanged.
211-215: Dead reference toicons.SystrayLight— either use it as a fallback or drop it.The blank assignment only silences an unused-import error, and the comment suggests a fallback that never happens (when
logois empty nothing is set on the tray). Either wire it in as the fallback icon, or remove both the line and theiconsimport.♻️ Suggested simplification
tray := app.SystemTray.New() - _ = icons.SystrayLight // ensure icons module is wired; tray uses defaults if we set nothing if len(logo) > 0 { tray.SetIcon(logo) + } else { + tray.SetIcon(icons.SystrayLight) }Or drop both the assignment and the
"github.com/wailsapp/wails/v3/pkg/icons"import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/examples/systray-stress/main.go` around lines 211 - 215, The file contains a dead reference to icons.SystrayLight that only silences an unused import; either use it as the fallback icon when logo is empty or remove the line and the icons import. Fix option A: change the logic around tray/SetIcon (symbols: tray, logo, tray.SetIcon) so if len(logo) == 0 you call tray.SetIcon(icons.SystrayLight) as the default; Fix option B: delete the blank assignment to icons.SystrayLight and remove the "github.com/wailsapp/wails/v3/pkg/icons" import so the unused reference is eliminated.
60-89: INPUT struct padding assumes 64-bit architecture; build constraint should be more specific.The hard-coded
_padding [8]byteinkeyboardInputanduint32alignment word ininputare sized for amd64/arm64 only. On Windows 386, these sizes don't match the real Win32INPUTlayout, andSendInputwould silently fail. While the file has a//go:build windowsconstraint, clarify this further with//go:build windows && (amd64 || arm64)if 386 support is not intended. Alternatively, add a comment documenting that the padding is 64-bit specific.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/examples/systray-stress/main.go` around lines 60 - 89, The INPUT struct padding is hardcoded for 64-bit and will break on 32-bit Windows; update the build constraint to restrict this file to 64-bit Windows by adding a top-level build tag like "//go:build windows && (amd64 || arm64)" (affecting the keyboardInput, input and sendEscapeKey definitions), or if 386 support is required, adjust the `_padding` size and the alignment word in `input` to match 32-bit Win32 layout and add a clear comment; ensure the change is applied where `keyboardInput`, `_padding`, `input`, and `sendEscapeKey` are defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/examples/systray-stress/main.go`:
- Around line 231-255: The exit helper always logs runtime_ms: 0 because start
time is only passed for the duration_reached branch; fix by capturing a single
start time variable (e.g., startTime := time.Now()) at the outer scope and
change exit (the exit func) to compute elapsed :=
time.Since(startTime).Milliseconds() and set fields["runtime_ms"]=elapsed before
logging; remove the redundant local start := time.Now() inside
OnApplicationEvent so all terminal branches (iter_target_reached,
handle_cap_exceeded, app_run_error, duration_reached) report the actual runtime.
- Around line 300-302: Update the stale comment to reflect that the SetBitmap
leak was fixed: state that freeBitmaps() now walks menuMapping and deletes
impl.bitmap for each windowsMenuItem (see freeBitmaps, menuMapping,
windowsMenuItem) so bitmaps are released on updateMenu/rebuild; note that the
mutate workload in this example verifies that this cleanup path still runs on
every rebuild rather than implying a leak remains.
In `@v3/pkg/application/menu_windows.go`:
- Around line 120-128: AppendMenu's return value is currently ignored in the
loop that builds the menu (w32.AppendMenu(parentMenu, flags, uintptr(itemID),
menuText)), which can hide append failures and lead to misleading SetMenuIcons
errors; update this block to check the AppendMenu success and on failure log via
globalApplication.error with context (including itemID/menuText) and abort the
rebuild (mirror popupmenu_windows.go buildMenu behavior) before attempting
SetMenuIcons; also consider changing the SetMenuIcons error handling in this
function to return (or at least log the append failure first) so w.bitmaps is
only appended when both AppendMenu and SetMenuIcons succeed.
In `@v3/pkg/application/popupmenu_windows.go`:
- Around line 170-182: The rebuild currently mutates p.menu and returns early on
AppendMenu/SetMenuIcons failures, leaving a partially-built menu; change
buildMenu to return an error and have Update() create the new HMENU into a local
variable (e.g., newMenu) and call buildMenu(newMenu, ...) so any
AppendMenu/SetMenuIcons failures cause buildMenu to return that error instead of
early-returning, then only swap p.menu and destroy the old menu when buildMenu
completes successfully; also propagate errors from recursive submenu calls in
buildMenu so an inner failure aborts the whole rebuild rather than appending a
partial submenu.
---
Outside diff comments:
In `@v3/pkg/application/menu_windows.go`:
- Around line 52-59: windowsMenu.update currently rebuilds the HMENU but leaves
w.menuMapping and w.currentMenuID intact, causing stale pointer-keyed entries to
accumulate; modify windowsMenu.update so after freeing bitmaps and
destroying/creating the HMENU (i.e., keep freeBitmaps() then
DestroyMenu/NewPopupMenu calls) you reinitialize w.menuMapping to a fresh map
and reset w.currentMenuID to its initial value (e.g., 0) before calling
w.processMenu so the mapping and ID counter don't grow across rebuilds.
---
Nitpick comments:
In `@v3/examples/systray-stress/main.go`:
- Around line 180-187: The function logJSON is misnamed because it emits
space-separated key=value output rather than JSON; rename the function (e.g., to
logEvent or logKV) and update all call sites to use the new name, adjust the
inline comment to describe "prefix=value" logging, and keep the same signature
(func logEvent(event string, fields map[string]any)) so behavior and interfaces
remain unchanged.
- Around line 211-215: The file contains a dead reference to icons.SystrayLight
that only silences an unused import; either use it as the fallback icon when
logo is empty or remove the line and the icons import. Fix option A: change the
logic around tray/SetIcon (symbols: tray, logo, tray.SetIcon) so if len(logo) ==
0 you call tray.SetIcon(icons.SystrayLight) as the default; Fix option B: delete
the blank assignment to icons.SystrayLight and remove the
"github.com/wailsapp/wails/v3/pkg/icons" import so the unused reference is
eliminated.
- Around line 60-89: The INPUT struct padding is hardcoded for 64-bit and will
break on 32-bit Windows; update the build constraint to restrict this file to
64-bit Windows by adding a top-level build tag like "//go:build windows &&
(amd64 || arm64)" (affecting the keyboardInput, input and sendEscapeKey
definitions), or if 386 support is required, adjust the `_padding` size and the
alignment word in `input` to match 32-bit Win32 layout and add a clear comment;
ensure the change is applied where `keyboardInput`, `_padding`, `input`, and
`sendEscapeKey` are defined.
In `@v3/pkg/application/menu_windows.go`:
- Around line 24-41: In freeBitmaps, avoid possible double-free between
w.bitmaps and per-item windowsMenuItem.bitmap by deduplicating and deleting each
HBITMAP exactly once: in windowsMenu.freeBitmaps build a set keyed by handle
(from w.bitmaps and from each item.impl.(*windowsMenuItem).bitmap via
w.menuMapping), then iterate the set and call w32.DeleteObject once per unique
handle and clear the per-item impl.bitmap and w.bitmaps; alternatively, if you
prefer minimal change, add a clear comment in freeBitmaps documenting the
current ownership invariant (w.bitmaps vs windowsMenuItem.bitmap) so future
changes don’t accidentally cause double DeleteObject.
In `@v3/pkg/application/popupmenu_windows.go`:
- Around line 64-81: Extract the duplicated bitmap cleanup logic from
Win32Menu.freeBitmaps and windowsMenu.freeBitmaps into a shared helper (e.g.,
freeMenuBitmaps) that accepts the slice of HBITMAPs and the menuMapping, then
update both Win32Menu.freeBitmaps and windowsMenu.freeBitmaps to call
freeMenuBitmaps; ensure the helper iterates the provided bitmaps slice and calls
w32.DeleteObject, walks the mapping entries, type-asserts to *windowsMenuItem,
deletes impl.bitmap via w32.DeleteObject and sets impl.bitmap = 0, and finally
clears the input slice/mapping references as the original functions did.
- Around line 314-317: Destroy() on Win32Menu is not idempotent because it
leaves p.menu and p.menuMapping populated so repeated calls re-walk and call
w32.DestroyMenu(0); change Destroy to be safe by early-returning if p.menu == 0
(or nil equivalent), and after calling p.freeBitmaps() and
w32.DestroyMenu(p.menu) set p.menu = 0 and clear/empty p.menuMapping (and any
other references to the destroyed menu) so subsequent Destroy calls are no-ops;
reference the Win32Menu.Destroy, p.menu, p.menuMapping, freeBitmaps, and
updateMenu (systemtray_windows.go) when making the change.
In `@v3/pkg/w32/menu_windows_test.go`:
- Around line 48-73: The test TestMenuHandleLifecycleDoesNotLeak currently only
samples start/end and calls getUserObjectCount which re-loads user32/kernel32
via NewLazyDLL on each call; to improve robustness, either add a mid-run
sampling/assertion (call getUserObjectCount at one or more intermediate
iterations and assert delta remains small) or increase iterations while keeping
tolerance small, and modify getUserObjectCount to cache the DLL and Proc handles
by creating the LazyDLL/LazyProc values at package init (e.g., package-level
vars initialized once) instead of calling NewLazyDLL/NewProc on every
getUserObjectCount invocation.
In `@v3/UNRELEASED_CHANGELOG.md`:
- Line 26: Update the changelog entry text to reference the related issue/PR by
appending a traceability note such as " (closes `#5227`)" or "See `#5227`" to the
existing sentence about the Windows systray fix so readers can follow back to
the report; edit the UNRELEASED_CHANGELOG.md entry that begins "Fix a family of
Windows systray `SetMenu` crashes..." to include the issue/PR number for
traceability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f590aebf-5aed-4f2f-a7ff-85733ec6a470
⛔ Files ignored due to path filters (1)
v3/examples/systray-stress/logo.pngis excluded by!**/*.png
📒 Files selected for processing (10)
v3/UNRELEASED_CHANGELOG.mdv3/examples/systray-stress/README.mdv3/examples/systray-stress/main.gov3/pkg/application/menu_windows.gov3/pkg/application/menuitem_windows.gov3/pkg/application/popupmenu_windows.gov3/pkg/application/systemtray_windows.gov3/pkg/w32/icon.gov3/pkg/w32/menu_windows_test.gov3/pkg/w32/user32.go
Contributor
Author
|
Addressed:
Consciously skipped:
Let me know if any of the skipped items should be revisited. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v3/pkg/application/menu_windows.go (1)
65-68:⚠️ Potential issue | 🟠 MajorSame orphan-bitmap concern as
Win32Menu.Update— more likely to trigger here.
windowsMenu.update()is called fromShowAton every context-menu display, so theoldHMENU != 0branch fires routinely. ThereleaseMenuBitmaps(oldBitmaps, oldMapping)call at line 66 runs afterprocessMenuhas swappeditem.implfor every item (line 83), so any runtimeSetBitmaphandle previously held on the old impl's.bitmapis unreachable by the time the mapping walk insidereleaseMenuBitmapsruns — it leaks on every successful rebuild. See the detailed comment and suggested fix onv3/pkg/application/popupmenu_windows.go(lines 233–251); the same pre-processMenuimpl-bitmap harvesting fix applies here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/menu_windows.go` around lines 65 - 68, windowsMenu.update currently calls releaseMenuBitmaps(oldBitmaps, oldMapping) after processMenu swaps item.impl, which causes bitmap handles on the old impls to become unreachable and leak; before calling processMenu (i.e., while oldHMENU != 0 and before any item.impl is replaced) iterate the current items and collect any bitmap handles held on the existing impls (the same handles releaseMenuBitmaps expects) into oldBitmaps/oldMapping, then call releaseMenuBitmaps immediately and only afterwards proceed to processMenu to replace item.impl and DestroyMenu(oldHMENU); reference windowsMenu.update, ShowAt, processMenu, item.impl, SetBitmap, releaseMenuBitmaps, oldHMENU, oldBitmaps and oldMapping when locating where to perform this pre-swap harvest.
🧹 Nitpick comments (1)
v3/examples/systray-stress/main.go (1)
1-380: LGTM — stress harness is well-structured and the previously-flagged issues (runtime_ms=0, stale leak comment) are addressed.The four-mode workload cleanly separates concerns:
churnexercises staged rebuilds,mutateregression-guards the runtime-SetBitmapcleanup path described in the PR, andshow/churn+showexercise the popup-display path. A couple of minor observations, none blocking:
showmode has no internal progress/handle-cap check; onlychurn/mutateemit progress lines and evaluatehandle_cap_exceeded. That means-mode=showwithout-durationwill run forever without surfacing leaks unless paired withchurn. Worth a README note or a similar periodic check if you wantshow-only runs to self-diagnose.- The dismiss goroutine in
show(lines 342–345) can fire afterOpenMenuhas already returned (e.g., ifTrackPopupMenuExfails fast), sending an ESC to whatever window is foreground. Fine for a local stress harness; less ideal if this ever runs unattended in CI.keyboardInput._padding [8]byteis sized for amd64INPUTunion layout; given Wails v3 targets amd64/arm64 Windows this is fine, but a brief comment noting the 64-bit assumption would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/examples/systray-stress/main.go` around lines 1 - 380, The show mode currently lacks periodic progress/handle-cap checks and its dismiss goroutine may send ESC after OpenMenu returns; update the show function to (1) emit the same progress log and handle/GDI cap checks used in churn/mutate (use getGuiResources, logEvent, cfg.logEvery, cfg.handleCap and call exit on exceed) and (2) prevent late ESC by coordinating the dismiss goroutine with the OpenMenu call (e.g., create a per-iteration done signal/channel that the goroutine selects on before calling sendEscapeKey, and close/signal it immediately after OpenMenu returns) so sendEscapeKey only runs while the menu is expected open.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/pkg/application/popupmenu_windows.go`:
- Around line 233-251: Before calling buildMenu (in popupmenu_windows.go update
path) snapshot the HBITMAP handles from the current item implementations so they
aren't lost when buildMenu replaces item.impl; specifically iterate oldMapping
(p.menuMapping) and collect each impl.bitmap into oldBitmaps (or a new slice)
before calling p.buildMenu(newHMENU, p.menuData). After a successful build, call
releaseMenuBitmaps with that pre-build bitmap slice and the oldMapping to free
those handles, and keep the existing failure-path cleanup logic untouched. Apply
the same pre-build bitmap snapshot approach in the parallel windowsMenu.update()
code path to prevent leaks when item.impl is reassigned.
---
Duplicate comments:
In `@v3/pkg/application/menu_windows.go`:
- Around line 65-68: windowsMenu.update currently calls
releaseMenuBitmaps(oldBitmaps, oldMapping) after processMenu swaps item.impl,
which causes bitmap handles on the old impls to become unreachable and leak;
before calling processMenu (i.e., while oldHMENU != 0 and before any item.impl
is replaced) iterate the current items and collect any bitmap handles held on
the existing impls (the same handles releaseMenuBitmaps expects) into
oldBitmaps/oldMapping, then call releaseMenuBitmaps immediately and only
afterwards proceed to processMenu to replace item.impl and
DestroyMenu(oldHMENU); reference windowsMenu.update, ShowAt, processMenu,
item.impl, SetBitmap, releaseMenuBitmaps, oldHMENU, oldBitmaps and oldMapping
when locating where to perform this pre-swap harvest.
---
Nitpick comments:
In `@v3/examples/systray-stress/main.go`:
- Around line 1-380: The show mode currently lacks periodic progress/handle-cap
checks and its dismiss goroutine may send ESC after OpenMenu returns; update the
show function to (1) emit the same progress log and handle/GDI cap checks used
in churn/mutate (use getGuiResources, logEvent, cfg.logEvery, cfg.handleCap and
call exit on exceed) and (2) prevent late ESC by coordinating the dismiss
goroutine with the OpenMenu call (e.g., create a per-iteration done
signal/channel that the goroutine selects on before calling sendEscapeKey, and
close/signal it immediately after OpenMenu returns) so sendEscapeKey only runs
while the menu is expected open.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50d861be-e758-46e8-bdfc-a93b65fffdd7
📒 Files selected for processing (3)
v3/examples/systray-stress/main.gov3/pkg/application/menu_windows.gov3/pkg/application/popupmenu_windows.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/pkg/application/popupmenu_windows.go (1)
210-263: Staged rebuild + rollback LGTM.The flow correctly stages new state, hoists runtime
SetBitmaphandles intooldBitmapsbeforebuildMenureassignsitem.impl, and only destroys the old HMENU tree once the build succeeds. The documented failure-path caveat (staleimplfor items processed before the failure point) is a reasonable trade-off for a path that only triggers under Win32 resource exhaustion.Minor observation: the transfer loop at lines 229-234 is duplicated verbatim in
menu_windows.goupdate()(lines 53-58). Could be extracted to a helper liketransferRuntimeBitmaps(mapping, bitmaps []HBITMAP) []HBITMAPalongsidereleaseMenuBitmaps, but only if you're already touching both files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/popupmenu_windows.go` around lines 210 - 263, The duplicate runtime-bitmap transfer loop present in Win32Menu.Update (for _, item := range oldMapping { ... }) is repeated in menu_windows.go's update(); extract that logic into a shared helper (e.g., transferRuntimeBitmaps) and call it from both Win32Menu.Update and the update function in menu_windows.go, ensuring the helper accepts the mapping and existing bitmap slice and returns the updated []HBITMAP; also add the helper near releaseMenuBitmaps to keep related bitmap utilities together and replace both in-place loops with calls to transferRuntimeBitmaps to remove duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/pkg/application/popupmenu_windows.go`:
- Around line 210-263: The duplicate runtime-bitmap transfer loop present in
Win32Menu.Update (for _, item := range oldMapping { ... }) is repeated in
menu_windows.go's update(); extract that logic into a shared helper (e.g.,
transferRuntimeBitmaps) and call it from both Win32Menu.Update and the update
function in menu_windows.go, ensuring the helper accepts the mapping and
existing bitmap slice and returns the updated []HBITMAP; also add the helper
near releaseMenuBitmaps to keep related bitmap utilities together and replace
both in-place loops with calls to transferRuntimeBitmaps to remove duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d59149e-cfbc-48df-aea3-337f076037c3
📒 Files selected for processing (2)
v3/pkg/application/menu_windows.gov3/pkg/application/popupmenu_windows.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/pkg/application/menu_bitmap_windows_test.go`:
- Around line 26-31: getGDIObjectCount currently ignores the syscall error and
doesn't validate the returned value, which can allow a zero (failed) sample to
silently pass; update getGDIObjectCount to capture and check the syscall
error/return values from testGetGuiResources (use the third return value or
lastErr equivalent), and ensure the returned n is non-zero (or log/fail the
test) so callers can detect a broken probe — modify the function to return an
error or assert/log when n==0 and surface that to the caller (references:
getGDIObjectCount, testGetGuiResources, testGR_GDIOBJECTS).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59817c9c-b97e-4cbf-ada9-baeafe4c26a7
📒 Files selected for processing (1)
v3/pkg/application/menu_bitmap_windows_test.go
This was referenced Apr 27, 2026
Contributor
Author
|
@leaanthony The CI is red due to changes unrelated to this PR. They are license compliance issues that were present in wails already, and require a maintainer to fix. Has it been fixed on v3-alpha? Does this just need a rebase? |
Contributor
Author
|
I see you changed the target branch. Is master the correct branch for this change? |
The wrapper was calling procDestroyMenu.Call(1, uintptr(hMenu), 0, 0), which on x86-64 Windows places the literal 1 in RCX (the single argument slot DestroyMenu expects) and the real handle in RDX, where it is ignored. Every call therefore returned FALSE (invalid handle) without freeing anything. As a result Win32Menu.Destroy() — invoked from windowsSystemTray.destroy on app shutdown and from the SetMenu rebuild path once the surrounding fixes land — has been a silent no-op. Fix the syscall to pass the handle as the single argument. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cate Update windowsSystemTray.updateMenu replaced s.menu without destroying the previous *Win32Menu, leaking its entire HMENU tree (top-level menu + every submenu created by buildMenu) on every SetMenu call. It also followed NewPopupMenu (which already runs Update internally) with a second explicit s.menu.Update, causing each rebuild to allocate the entire tree twice and abandon the first copy. Destroy the previous Win32Menu before replacing the pointer, and remove the redundant Update call. Paired with the DestroyMenu syscall fix that precedes this commit — if that commit is absent, s.menu.Destroy is silently a no-op and this change has no measurable effect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Win32Menu.Update overwrote p.menu with a freshly allocated HMENU without destroying the previous one, so every Update leaked the full HMENU tree (the popup itself plus every submenu HMENU created inside buildMenu). It also failed to reset p.checkboxItems and p.radioGroups, so repeated Update calls accumulated entries referencing destroyed item IDs. Destroy the previous HMENU — DestroyMenu recursively destroys attached submenus — and reset the checkbox / radio maps before rebuilding. Depends on the DestroyMenu syscall fix earlier in this series; without it the destroy call is silently a no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
buildMenu called globalApplication.fatal on any AppendMenu or SetMenuIcons failure, tearing down the entire app for a single unlucky menu item — including transient failures from USER / GDI handle exhaustion that the companion fixes in this series aim to prevent from ever happening. Downgrade both to globalApplication.error and abort the current rebuild so the previous (working) menu continues to be usable. Defense in depth rather than masking a root cause: the leak fixes should make AppendMenu failures vanishingly rare, but a production app should not crash if one ever occurs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… destroy w32.SetMenuIcons allocated a fresh HBITMAP per call via CreateHBITMAPFromImage and handed it to SetMenuItemBitmaps without returning the handle to the caller. No code path ever called DeleteObject on it. Per MSDN the application owns bitmaps set via SetMenuItemBitmaps — DestroyMenu does not free them — so each SetMenu rebuild leaked one HBITMAP per bitmap menu item (a fresh GDI object from CreateDIBSection), filling the 10,000/process GDI quota in a few thousand rebuilds for apps with bitmap items. Empirical repro: ~10 GDI handles/iter against a 10-bitmap menu. Change SetMenuIcons to return the allocated handles and update all three call sites to track and free them: - popupmenu_windows.go: Win32Menu.bitmaps, freed in Update() and Destroy(). - menu_windows.go: windowsMenu.bitmaps, freed in update(). Also downgrades the SetMenuIcons fatal here to error+continue, matching the earlier commit in this series that softened the popupmenu path. - menuitem_windows.go: windowsMenuItem.bitmap tracks the current handle so dynamic MenuItem.SetBitmap updates release the previous bitmap before installing the new one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stress harness that drives SystemTray.SetMenu under three workloads: pure churn, popup-open-plus-dismiss, and both concurrent. Logs GR_USEROBJECTS / GR_GDIOBJECTS deltas per N iterations and exits cleanly when iteration, duration, or handle caps are reached. Used to diagnose and verify the handle-leak + soft-fail fixes in the preceding commits, and retained as a regression surface against future changes to the Windows menu path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three tests against the Windows-only w32 package: - TestDestroyMenuReturnsTrueForValidHandle — calls CreatePopupMenu + DestroyMenu and asserts the latter returns true. Before the syscall signature fix this returned false because the handle argument was being placed in the wrong register slot. - TestDestroyMenuReturnsFalseForZeroHandle — sanity check that passing handle 0 returns false. - TestMenuHandleLifecycleDoesNotLeak — runs 2000 Create+Append+Destroy cycles and asserts the process's USER-object handle count does not grow unboundedly. On the pre-fix DestroyMenu this test fails at iteration 0 on the DestroyMenu assertion and would otherwise have leaked ~2000 handles. Verified to fail against the pre-fix DestroyMenu (handles the first commit in this series). Runs on Windows CI under the //go:build windows constraint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing -bitmaps churn mode rebuilds a fresh Menu every iteration, so MenuItems never have an impl at SetBitmap time and the Windows setBitmap path is a no-op on the native side. Mutate mode keeps a single Menu across iterations and calls SetBitmap on its items after the first build — that is the path where windowsMenuItem.setBitmap allocates an HBITMAP tracked only on impl.bitmap, which is orphaned when the next SetMenu rebuild replaces the impl. Pre-fix measurement (5 targets × 1000 iterations): gdi_delta=5007 after 1000 iters (≈5 leaks/iter, 1/target/iter) Post-fix measurement with the same workload: gdi_delta=12 flat across 1000 iters Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stroy HBITMAPs allocated by MenuItem.SetBitmap at runtime (after the initial build) live on windowsMenuItem.bitmap, not in the menu-level bitmaps slice. freeBitmaps only walked the slice, so every SetMenu rebuild replaced the impl and orphaned its HBITMAP — one leaked GDI object per item per rebuild. Extend freeBitmaps on both Win32Menu (popup) and windowsMenu (application) to walk menuMapping and release each impl's bitmap before DestroyMenu runs. Verified via systray-stress mutate mode: gdi_delta=5007→12 over 1000 iterations with 5 targets. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
….update Parity with Win32Menu.Update in popupmenu_windows.go — windowsMenu.update was leaving menuMapping and currentMenuID intact across rebuilds, so stale *MenuItem entries accumulated keyed by IDs that referenced HMENUs that no longer existed. Matches the fix already applied to the popup-menu twin. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n processMenu Parity with Win32Menu.buildMenu in popupmenu_windows.go — processMenu was discarding the AppendMenu return value and continuing on SetMenuIcons errors, so an AppendMenu failure silently surfaced as a confusing SetMenuIcons error on the next iteration. Match the popup-menu pattern: log the real error with item context and abort the rebuild. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tress The exit helper was seeding runtime_ms=0 and only the duration_reached branch passed a real value via extra — so iter_target_reached, handle_cap_exceeded, and app_run_error always reported 0ms, masking the actual run length for any supervisor comparing runs. Hoist start := time.Now() to the outer scope and compute time.Since(start) inside exit so every terminal branch reports correctly. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The header and inline comments described the runtime-SetBitmap leak as if it still occurred; the leak was fixed in this PR by having freeBitmaps walk menuMapping. Clarify that the mutate workload now guards against a regression of that cleanup path rather than reproducing an active bug. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d errors buildMenu and processMenu now return errors instead of logging and falling through. Errors from recursive submenu builds abort the whole rebuild rather than attaching a half-built submenu via MF_POPUP — the previous early-return path left the outer loop appending the partial submenu. Win32Menu.Update and windowsMenu.update stage the rebuild into a fresh HMENU and fresh maps, only destroying the old HMENU and swapping state once the build completes successfully. On failure the previous menu stays displayed and the partial HMENU is cleanly destroyed. The item.impl inconsistency across the failure boundary is documented in Update — the rebuild replaces item.impl as it goes, so any MenuItem touched before the failure point will hold a stale impl until the next successful rebuild. Acceptable for a path that only triggers on Win32 resource exhaustion. Pulled the common HBITMAP-release loop out into a file-private helper (releaseMenuBitmaps) since the success path now frees bitmaps tracked on a locally-held "old" state rather than on the receiver. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The function emits event=... key=value lines, not JSON — the name was leftover from an earlier iteration. Clarifies intent for readers and prevents anyone grepping for JSON output from being misled. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_ = icons.SystrayLight only existed to silence Go's unused-import check; nothing else in the file uses pkg/icons. Remove the blank reference and the import. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uild buildMenu/processMenu reassign item.impl at the top of every loop iteration, so by the time the staged Update's success path calls releaseMenuBitmaps(oldBitmaps, oldMapping), every item in oldMapping points at a freshly-created impl with bitmap == 0. The mapping walk finds nothing to free and any handle attached via runtime MenuItem.SetBitmap leaks — particularly bad for windowsMenu.update, which runs on every ShowAt of a context menu. Snapshot impl.bitmap handles into oldBitmaps before buildMenu runs, so the success path can rely on the slice alone. The failure path already restores oldBitmaps to the receiver, so the hoisted handles stay tracked for the next rebuild. Regression introduced by eff737c; caught by CodeRabbit on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Allocate a fresh HBITMAP on every iteration via MenuItem.SetBitmap, call Menu.Update to rebuild the HMENU, and measure the GR_GDIOBJECTS delta. A fully-leaking path orphans one HBITMAP per item per iteration (1500 handles over the run) — the 50-handle tolerance flags a single-item regression with comfortable noise headroom. This sits one altitude above the existing pkg/w32 DestroyMenu regression test: it exercises the full Menu.Update → processMenu → releaseMenuBitmaps cycle that CodeRabbit caught mid-staging on PR #5228, where item.impl replacement defeated the mapping-walk cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
getGDIObjectCount discarded the syscall error and did not validate the return value. GetGuiResources returns 0 on failure — in a hardened container or after a WinAPI change, both start and end samples would read 0, the delta would stay 0, and the regression guard would silently pass without measuring anything. Assert the reading is non-zero with t.Fatalf so a broken probe surfaces with the Win32 error instead of hiding behind a green check. Also drop the unused testGR_USEROBJECTS constant — the test only probes GDI. Addresses CodeRabbit review feedback on PR #5228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror the systray-stress churn workload at the unit level: allocate a fresh Win32Menu per iteration with build-time bitmaps via SetMenuIcons, then Destroy. Exercises the pkg/application-level cleanup path (freeBitmaps releasing p.bitmaps + DestroyMenu tearing down the HMENU tree) that the systray's tray.SetMenu flow hits. The existing pkg/w32 TestMenuHandleLifecycleDoesNotLeak covers only the raw DestroyMenu syscall; the harness covers this path only via a live Windows session. NewPopupMenu stores the parent HWND without dereferencing it unless ShowAt runs, so w32.GetDesktopWindow is a safe stand-in for a real window and keeps the test headless-CI-friendly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
johnmaguire
force-pushed
the
fix/windows-systray-setmenu-crashes
branch
from
03d75ba to
74aa2f6
Compare
The GetGuiResources probe in menu_bitmap_windows_test.go never produced a usable signal from a Go test process. Console processes start with zero GDI objects and the probe stays stuck at zero in both interactive sessions and CI runners, so the delta-based leak check could not discriminate between healthy cleanup and a fully-leaking path. Reverts the three commits that added and tried to stabilize this file (d60f7dc, 48c400f, 4724c0f). Coverage is not lost: - pkg/w32/menu_windows_test.go guards the raw DestroyMenu syscall signature that started this bug family. - examples/systray-stress/ exercises the Menu.Update / SetBitmap / SetMenu churn paths against a live Windows session where GetGuiResources actually reports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
I found #5272 so I believe master is correct, and I see that it is green now. Rebased, so this PR should go green soon. 🤞 |
Member
|
Thanks John! Sorry the move to master was confusing. Looking forward to simplifying a whole bunch of things now we have the one branch. |
Member
|
✅ Triaged by Wails PR Reviewer This PR has been reviewed and accepted. A test sub-issue has been created for Windows. Head Ref OID: This comment serves as a signature that this PR has been triaged. Future runs will skip this PR based on the headRefOid. |
leaanthony
approved these changes
May 1, 2026
Member
|
Thanks for taking the time on this 🙏 |
Member
|
Tested on Windows — verdict: ✓ pass
pkg/w32 test detail: The handle-leak test creates and destroys 2000 menu objects and confirms a USER handle delta of exactly 0, directly validating the Pre-existing failures (unrelated to this PR):
|
Contributor
Author
|
@leaanthony Absolutely, thanks for all your work on Wails. 🙇 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #5227
This fix was developed with Claude Code using empirical evidence. I included a systray-stress harness that was used to verify the fixes.
Fixes a family of Windows systray
SetMenucrashes that manifested asGDI/USER handle exhaustion in long-running apps. Root cause was a broken
DestroyMenusyscall — it was being called with fouruintptrarguments(
1, hMenu, 0, 0), which placed the literal1in the handle slot anddropped the real HMENU in the ignored second slot, so every call returned
FALSE without freeing anything. Go's
(*Proc).Call(a ...uintptr)passesits args verbatim to the Win32 function; there is no "arg count" prefix
convention.
Fixing the syscall surfaced several latent ownership bugs that had been
masked by
DestroyMenubeing a no-op:Win32Menu.Updaterecreated the HMENU tree without destroying theprevious one.
Win32Menu.Updatealso leftcheckboxItems/radioGroupsmapspopulated with item IDs from the destroyed HMENU, so later
CheckMenuItem/CheckRadiocalls addressed freed menus.SetMenuItemBitmapsare not freed byDestroyMenu(Microsoft docs are explicit).
SetMenuIconsdropped the handle refsentirely, leaking a bitmap per icon per rebuild.
MenuItem.SetBitmapwere onlytracked on the
windowsMenuItemimpl, not the menu-level slice, sothey leaked on every rebuild even after the per-menu bitmap tracking
was in place.
windowsSystemTray.updateMenuwas callingUpdate()on the newWin32Menuright afterNewPopupMenu(which already callsUpdate()in its constructor), doubling the allocations, and never destroyed
the previous
s.menu.All Win32 API contracts were re-verified against Microsoft Learn:
BOOL DestroyMenu([in] HMENU hMenu);and "DestroyMenu is recursive,that is, it will destroy the menu and all its submenus."
"When the menu is destroyed, these bitmaps are not destroyed; it is
up to the application to destroy them."
(*Proc).Call(a ...uintptr), "Call executes procedure p witharguments a."
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Verified on Windows 11 Pro (build 26100). Test matrix:
go test ./v3/pkg/w32/...on Windows — new regression tests inmenu_windows_test.golock theDestroyMenusyscall signature andassert USER-object handle usage is bounded across
Create+Append+Destroy cycles.
v3/examples/systray-stress— black-box harness with four workloads,handle-delta budgets, and iteration caps. Commands run:
systray-stress -mode=churn -iters=50000 -churn-gap=1ms— passeswith flat handle deltas.
systray-stress -mode=churn -bitmaps=true -iters=10000— exercisesinitial-build
SetMenuIcons. Flat GDI delta.systray-stress -mode=show -duration=30s— repeated OpenMenu + ESCdismiss. No crash.
systray-stress -mode=churn+show -iters=5000 -churn-gap=1ms -show-gap=20ms -dismiss-gap=10ms— concurrent churn and popup.Completed without crash; confirmed
InvokeSyncserializationalready protects
Destroyfrom racingTrackPopupMenuEx.systray-stress -mode=mutate -iters=1000 -log-every=100—exercises the runtime
MenuItem.SetBitmapcode path. Before thefix:
gdi_delta=5007over 1000 iterations. After the fix:gdi_delta=12(flat).Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Bug Fixes
Tests
Documentation
Examples