Repository navigation
fix(v3): prevent Menu.Update() segfault when app is not running - #5291
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds early-return guards to menu update paths: menu.Update now returns when there is no global application or the app is not running; platform-specific Linux update now returns when native handle or menu is nil before processing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.go (1)
64-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the guard before any menu work, and read
runningunderrunLock.
processRadioGroups()andnewMenuImpl()still execute before the early return, so the pre-run path can still touch platform state. Also,globalApplication.runningis protected byrunLockelsewhere, so reading it directly here introduces a race.Suggested fix
func (m *Menu) Update() { + app := globalApplication + if app == nil { + return + } + app.runLock.Lock() + running := app.running + app.runLock.Unlock() + if !running { + return + } + m.processRadioGroups() if m.impl == nil { m.impl = newMenuImpl(m) } - if globalApplication == nil || !globalApplication.running { - return - } m.impl.update() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/menu.go` around lines 64 - 72, In Menu.Update move the early-return guard to the top and read globalApplication.running while holding the application's runLock: first check globalApplication != nil under a runLock read lock and if not running return immediately, then proceed to call m.processRadioGroups() and initialize m.impl via newMenuImpl(m) and finally call m.impl.update(); ensure you acquire/release the runLock (RLock/RUnlock) when reading running to avoid the race.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@v3/pkg/application/menu.go`:
- Around line 64-72: In Menu.Update move the early-return guard to the top and
read globalApplication.running while holding the application's runLock: first
check globalApplication != nil under a runLock read lock and if not running
return immediately, then proceed to call m.processRadioGroups() and initialize
m.impl via newMenuImpl(m) and finally call m.impl.update(); ensure you
acquire/release the runLock (RLock/RUnlock) when reading running to avoid the
race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6b4ee39-f70e-41d8-ae60-ae56e68d54be
📒 Files selected for processing (2)
v3/pkg/application/menu.gov3/pkg/application/menu_linux.go
Moves the nil/running check to the top of Update() before processRadioGroups() and newMenuImpl() to prevent GTK crash on Linux when called before app initialization. Also reads running under runLock to fix a data race. Suggested-by: CodeRabbit review on PR wailsapp#5291
There was a problem hiding this comment.
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.go (1)
21-24:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
ContextMenu.Update/Destroycan still panic when app is nil
Menu.Update()now returns safely pre-run, but Line 23 and Line 27 still unconditionally dereferenceglobalApplication.NewContextMenu()callsUpdate()immediately, so this remains a pre-init crash path.Suggested fix
func (m *ContextMenu) Update() { m.Menu.Update() - globalApplication.ContextMenu.Add(m.name, m) + if app := globalApplication; app != nil { + app.ContextMenu.Add(m.name, m) + } } func (m *ContextMenu) Destroy() { - globalApplication.ContextMenu.Remove(m.name) + if app := globalApplication; app != nil { + app.ContextMenu.Remove(m.name) + } }Also applies to: 26-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/menu.go` around lines 21 - 24, ContextMenu.Update and ContextMenu.Destroy unconditionally dereference globalApplication (calling globalApplication.ContextMenu.Add/Remove) and can panic if the app is nil; modify both methods (ContextMenu.Update and ContextMenu.Destroy) to first check that globalApplication and globalApplication.ContextMenu are non-nil before calling Add/Remove, returning early if either is nil so NewContextMenu can safely call Update pre-init.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@v3/pkg/application/menu.go`:
- Around line 21-24: ContextMenu.Update and ContextMenu.Destroy unconditionally
dereference globalApplication (calling globalApplication.ContextMenu.Add/Remove)
and can panic if the app is nil; modify both methods (ContextMenu.Update and
ContextMenu.Destroy) to first check that globalApplication and
globalApplication.ContextMenu are non-nil before calling Add/Remove, returning
early if either is nil so NewContextMenu can safely call Update pre-init.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 921a0cc1-cd4c-4fa9-8332-e902118a4725
📒 Files selected for processing (1)
v3/pkg/application/menu.go
Prevents panic when NewContextMenu() is called before app initialization, since Update() is invoked immediately on construction. Addresses CodeRabbit review on PR wailsapp#5291
|
✅ Triaged by Wails PR Reviewer This PR has been reviewed and accepted. Test sub-issues have been created for all platforms. 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. |
Linux verification — PASSEDPlatform: Ubuntu 24.04.4 LTS (Noble Numbat), Webkit2Gtk v2.50.4, Go 1.25.0, amd64 Results
Reproducer outputSmoke test output (v3/examples/menu, 8s run)Pre-existing failure note
Verdict
|
…ent Menu.Update() segfault when app is not running
Description
Fixes a segmentation fault that occurs when
Menu.Update()is called before the Wails application enters a running state.Bug
Calling
Menu.Update()beforeapp.Run()causes a panic on macOS due toInvokeSyncattempting to dispatch to the main thread before the run loop is initialized:On Linux, similar nil pointer dereferences can occur if the native menu hasn't been initialized.
Root Cause
Menu.Update()unconditionally callsm.impl.update(), which on macOS usesInvokeSyncto dispatch work to the main thread. If the application hasn't started yet, the main thread dispatcher is not ready, leading to a crash.Fix
menu.go: Added a guard inUpdate()to check ifglobalApplicationis running. If not, the method returns silently. This defers menu updates until the application is ready.menu_linux.go: Added a nil check form.nativeandm.menuin theupdate()method to prevent nil pointer dereferences on Linux.Both changes allow
Update()calls made during initialization to be harmlessly ignored, with the menu being properly updated once the application starts.Closes #5290.
Summary by CodeRabbit