fix(v3/linux): fix crash on panic in JS-bound Go methods #4856
+15
−2
Conversation
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
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
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughA signal-handler installation fix for Linux WebKit compatibility was implemented by deferring handler registration from application initialization to the first WebView creation, ensuring registration occurs after GTK/WebKit initialization completes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
Deploying wails with
|
| Latest commit: |
c91f029
|
| Status: | ⚡️ Build in progress... |
Adds a test application that can trigger panics from JS-bound Go methods to verify the signal handler fix for issue #3965 works correctly. The test app has buttons to: - Trigger a panic in a goroutine - Trigger an immediate panic - Call a safe method Before the fix, clicking 'Trigger Panic' would crash the app. After the fix, panics are recovered and logged.
…llback Move install_signal_handlers() call to after webkit_web_view_new_with_user_content_manager() to ensure WebKit has finished setting up its signal handlers before we add SA_ONSTACK. The previous g_idle_add approach was still too early - WebKit initialization continues after GTK init. This matches the v2 approach which waits for actual webview creation. Also add documentation about the known Go limitation (golang/go#7227) where signals may still be delivered on the wrong stack in some C interop scenarios.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Problem
When Go code called from JavaScript panics (e.g., nil pointer dereference), the app crashes with:
This happens because:
Solution
Call
install_signal_handlers()AFTERwebkit_web_view_new_with_user_content_manager()returns, ensuring we add SA_ONSTACK to signal handlers AFTER WebKit has installed its own. This matches the approach used in v2.Known Limitation
Note: This fix may not fully resolve all signal-related crashes. There is a known Go runtime limitation (golang/go#7227, open since 2014) where signals may still be delivered on the wrong stack when C libraries (like WebKit/GTK) are involved, even with SA_ONSTACK correctly set. This is a fundamental Go/C interop issue that requires Go runtime changes to fully resolve.
Type of change
How Has This Been Tested?
Fixes #3965
Note: This is the v3 version of the fix. See #4855 for the v2 version.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.