Repository navigation
Conversation
dosubot
Bot
added
the
size:XS
WalkthroughA changelog entry was added documenting a fix for a macOS crash when closing a window with pending async Go-bound calls. The internal logic of the Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript (async call)
participant Go as Go-bound function
participant Window as macosWebviewWindow
participant Parent as Parent Window
JS->>Go: Initiate async Go-bound call
Go-->>JS: Completes, triggers execJS
Go->>Window: execJS(js)
alt Window or Parent destroyed
Window->>Window: Early return, skip JS execution
else Window and Parent alive
Window->>Window: Execute JS via C.windowExecJS
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
Bot
added
the
Documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/src/content/docs/changelog.mdx (1)
126-131: Minor wording / consistency nitpickThe new bullet reads:
Fixed macOS window close with pending async Go-bound function call crashes …
Grammar feels off (plural crashes vs. singular fix items). Everywhere else the pattern is “Fixed … crash” or “Fixed … issue”. For consistency:
- - Fixed macOS window close with pending async Go-bound function call crashes + - Fixed crash when closing a macOS window with a pending async Go-bound function callPure docs update – apply if you care about wording consistency.
🧰 Tools
🪛 LanguageTool
[style] ~126-~126: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...m/leaanthony) based on work by @Kron. - Fixed focus calls when window is disabled or ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~127-~127: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...m/leaanthony) based on work by @Kron. - Fixed system trays not showing after taskbar ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~128-~128: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...m/leaanthony) based on work by @Kron. - Fixed fallbackResponseWriter not implementing...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/content/docs/changelog.mdx(1 hunks)v3/pkg/application/webview_window_darwin.go(1 hunks)
🔇 Additional comments (1)
v3/pkg/application/webview_window_darwin.go (1)
1041-1052: Early-exit guard prevents use-after-free, but a race still exists👍 The additional
w.parent.isDestroyed()check is the right direction for avoiding the macOS crash.
⚠️ However, a very small time window remains between the guard (line 1048) and the actual invocation ofC.windowExecJS(line 1051): the parent may be destroyed after the check but before the C call runs, re-introducing the use-after-free.Consider one of the following to make the call 100 % safe:
- if w.nsWindow == nil || w.parent.isDestroyed() { + // Capture pointer once and validate as late as possible + ns := w.nsWindow + if ns == nil || w.parent.isDestroyed() { return } - C.windowExecJS(w.nsWindow, C.CString(js)) + // Re-check immediately before the C call. + if !w.parent.isDestroyed() { + C.windowExecJS(ns, C.CString(js)) + }or mark
w.nsWindow = nilatomically insidedestroy()right after the Objective-C window is closed; the nil check would then be sufficient.Please double-check that
destroy()is always invoked before the Objective-C object is freed and that it runs on the same goroutine to avoid data races.
|
|
Hey @joshhardy - I had to update the semgrep runner too and both changes ended up being in the same branch so your changes are merged via #4356 with full attribution. Will close this for now. |
3 participants
Description
This fixes an issue where pending async results from Go-bound JS calls would trigger
C.windowExecJSinwebview_window_darwin.goeven if the underlying window was destroyed by adding a guard to check if the parent window is destroyed.This fixes a crash due to use-after-free (invalid webView access).
Fixes #4353
Type of change
Please select the option that is relevant.
How Has This Been Tested?
This is a macOS-specific issue that has been tested on macOS and confirmed to fix my production app as well as my minimal repro example app. All changes are in a darwin file.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Documentation
Bug Fixes