fix(v2/linux): fix crash on panic in JS-bound Go methods#4855
Conversation
WebKit2GTK installs signal handlers after gtk_main() starts, overriding our SA_ONSTACK fix. This causes Go panics (e.g., nil pointer dereference) in JS-bound methods to crash with 'non-Go code set up signal handler without SA_ONSTACK flag'. Fix by deferring signal handler installation via g_idle_add() to run after GTK main loop starts, ensuring we fix handlers AFTER WebKit has installed its own. Fixes #3965
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughDefers GTK signal handler installation until after GTK initialization completes by scheduling handler setup via GTK idle callbacks. This prevents signal handler race conditions on Linux where handlers were installed before GTK was fully initialized. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/internal/frontend/desktop/linux/frontend.go (1)
76-84: LGTM! Consider adding explanatory comments.The implementation correctly uses
g_idle_addto defer signal handler installation until after the GTK main loop starts, which ensures WebKit2GTK has installed its handlers first. The callback properly returnsG_SOURCE_REMOVEto execute only once.Consider adding a brief comment explaining the WebKit timing issue to help future maintainers understand why this deferral is necessary. For example:
// WebKit2GTK installs signal handlers without SA_ONSTACK after gtk_main() starts, // overriding the Go runtime's handlers. We use g_idle_add to fix them after WebKit // initialization completes. See: https://github.com/wailsapp/wails/issues/3965
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/internal/frontend/desktop/linux/frontend.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Sammy-T
Repo: wailsapp/wails PR: 4570
File: v2/internal/frontend/desktop/linux/window_webkit6.go:97-108
Timestamp: 2025-10-17T23:16:11.570Z
Learning: For webkit_6/GTK4 builds in v2/internal/frontend/desktop/linux/window_webkit6.go, GTK widget creation should not be wrapped in invokeOnMainThread. The activation mechanism (activateWg + onActivate export) already handles thread safety, and additional wrapping would cause issues.
Learnt from: APshenkin
Repo: wailsapp/wails PR: 4480
File: v2/internal/frontend/desktop/darwin/message.h:17-19
Timestamp: 2025-08-08T09:13:16.916Z
Learning: In Wails v2 bindings origin verification, processBindingMessage intentionally has different signatures across platforms: Darwin includes an isMainFrame bool (WKWebKit provides it), Linux uses two params (message, source) as WebKitGTK doesn’t expose main-frame info there, and Windows handles origin checks in Go via WebView2 sender/args without a C bridge. This divergence is acceptable/expected per maintainer (APshenkin).
📚 Learning: 2025-10-17T23:16:11.570Z
Learnt from: Sammy-T
Repo: wailsapp/wails PR: 4570
File: v2/internal/frontend/desktop/linux/window_webkit6.go:97-108
Timestamp: 2025-10-17T23:16:11.570Z
Learning: For webkit_6/GTK4 builds in v2/internal/frontend/desktop/linux/window_webkit6.go, GTK widget creation should not be wrapped in invokeOnMainThread. The activation mechanism (activateWg + onActivate export) already handles thread safety, and additional wrapping would cause issues.
Applied to files:
v2/internal/frontend/desktop/linux/frontend.go
📚 Learning: 2025-12-29T08:02:06.122Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4839
File: docs/src/content/docs/reference/window.mdx:616-620
Timestamp: 2025-12-29T08:02:06.122Z
Learning: In Wails v3, the correct API for creating windows is `app.Window.New()` and `app.Window.NewWithOptions(...)`, not `app.NewWebviewWindow()` or `app.NewWebviewWindowWithOptions(...)`. The Application struct exposes a Window field of type *WindowManager that provides these methods.
Applied to files:
v2/internal/frontend/desktop/linux/frontend.go
📚 Learning: 2026-01-04T08:01:00.038Z
Learnt from: symball
Repo: wailsapp/wails PR: 4853
File: v2/internal/system/system.go:128-152
Timestamp: 2026-01-04T08:01:00.038Z
Learning: In v2/internal/system/system.go, shared functions like checkLibrary are defined without build tags but are only invoked from platform-specific files (system_linux.go, system_windows.go, system_darwin.go) that have build constraints. Reviewers should ensure there are no runtime OS checks in system.go and that platform-specific behavior is controlled via build tags. If runtime switches exist, remove them in favor of compile-time platform constraints to reduce overhead and improve correctness.
Applied to files:
v2/internal/frontend/desktop/linux/frontend.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: semgrep/ci
- GitHub Check: Run Go Tests (windows-latest, 1.23)
- GitHub Check: Run Go Tests (ubuntu-24.04, 1.23)
- GitHub Check: Run Go Tests (ubuntu-22.04, 1.23)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
v2/internal/frontend/desktop/linux/frontend.go (1)
217-217: The signal handler deferral via g_idle_add() is correctly timed and fixes the intended issue.The implementation is sound. By calling
fix_signal_handlers_after_gtk_init()beforegtk_main(), the idle callback is registered to execute once the main loop is active. This ensures signal handlers are fixed after WebKit2GTK installs its own handlers duringgtk_main()startup, allowing theSA_ONSTACKflag to be properly applied to prevent panics in Go-bound methods when signals occur.The change has been tested and verified to work correctly on webkit2_41.
Deploying wails with
|
| Latest commit: |
f451efd
|
| Status: | ⚡️ Build in progress... |
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Summary
Problem
When Go code called from JavaScript panics (e.g., nil pointer dereference), the app crashes with:
This happens because:
install_signal_handlers()was called beforegtk_main()Solution
Use
g_idle_add()to schedule signal handler installation after GTK main loop starts, ensuring we fix handlers AFTER WebKit has installed its own.Type of change
How Has This Been Tested?
Fixes #3965
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.