fix(v3/windows): keep host-owned rasterization scale under visual hosting#5761
Conversation
wailsapp#5734 re-enabled WebView2 automatic monitor-scale detection (ShouldDetectMonitorScaleChanges) unconditionally to fix the mixed-DPI GPU-process crash. That is correct for the default windowed (HWND-child) hosting mode, but wrong under UseVisualHosting (COREWEBVIEW2_HOSTING_MODE_WINDOW_TO_VISUAL): the content is a DirectComposition visual decoupled from the child HWND that automatic detection tracks, so on a mixed-DPI monitor cross detection reads the wrong monitor and settles the visual on a stale rasterization scale -- the whole UI renders shrunk (small fonts) until something forces a re-layout. Gate the detection-enable on hosting mode: - windowed: enable detection (unchanged; keeps wailsapp#5734's crash fix) - visual hosting: explicitly disable detection so the host owns the scale via resyncWebviewRasterizationScale (monitorScaleDetectionOn stays false). The module leaves detection at its platform default (enabled), so visual hosting must disable it explicitly -- merely skipping the enable would leave both Edge's detection and the host resync writing the scale (the two-writer race wailsapp#5734 targets). Host-owned scale is reliable now that the by-value PutRasterizationScale bug (wailsapp#5701) is fixed, and is the documented contract (WebView2Feedback wailsapp#3665: detection-off is correct when the host puts the scale on every change). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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)
Walkthrough
ChangesWebView2 Monitor-Scale Detection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their 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. 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/webview_window_windows.go (1)
2413-2425: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winVisual-hosting disable failure leaves the system in the documented two-writers crash state.
When
UseVisualHostingis true andPutShouldDetectMonitorScaleChanges(false)fails, the code only logs and moves on. Per the comment above (Lines 2408-2411), the module leaves detection at its platform default (enabled), so a failed disable leaves automatic detection on whilemonitorScaleDetectionOnremains false — which meansresyncWebviewRasterizationScaleruns its full body (Lines 2055-2057). That is exactly the "two writers racing → scale(0,0)" GPU-crash scenario this change is meant to prevent.Set
monitorScaleDetectionOn = trueon disable failure so the host resync stands down and Edge remains the single writer (degraded shrunk-UI is preferable to the GPU-process crash).🛡️ Proposed defensive fix
if globalApplication.options.Windows.UseVisualHosting { if err := c3.PutShouldDetectMonitorScaleChanges(false); err != nil { globalApplication.error("webview2: disable monitor scale detection (visual hosting): %v", err) + // Disable failed: detection stays at its enabled default. + // Let the host resync stand down so Edge remains the sole + // scale writer, avoiding the two-writers scale(0,0) crash. + w.monitorScaleDetectionOn = true } } else if err := c3.PutShouldDetectMonitorScaleChanges(true); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/webview_window_windows.go` around lines 2413 - 2425, In the UseVisualHosting path inside webview window setup, a failed PutShouldDetectMonitorScaleChanges(false) currently only logs and leaves monitorScaleDetectionOn false, which can still allow resyncWebviewRasterizationScale to run against the platform’s default enabled detection. Update the failure branch in the controller.GetICoreWebView2Controller3 block so monitorScaleDetectionOn is set to true when disabling scale detection fails, keeping the host resync logic suppressed and preserving the single-writer behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@v3/pkg/application/webview_window_windows.go`:
- Around line 2413-2425: In the UseVisualHosting path inside webview window
setup, a failed PutShouldDetectMonitorScaleChanges(false) currently only logs
and leaves monitorScaleDetectionOn false, which can still allow
resyncWebviewRasterizationScale to run against the platform’s default enabled
detection. Update the failure branch in the
controller.GetICoreWebView2Controller3 block so monitorScaleDetectionOn is set
to true when disabling scale detection fails, keeping the host resync logic
suppressed and preserving the single-writer behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00135368-915e-4efd-be3d-eaff61f660e1
📒 Files selected for processing (1)
v3/pkg/application/webview_window_windows.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/pkg/application/webview_window_windows.go (1)
1669-1678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead
GetClientRectnil branch.
w32.GetClientRectpanics on failure and otherwise returns a non-nil*RECT, so this guard does not prevent the describedFillRect(nil, ...)crash path. If a real failure path needs hardening, use an OK-style wrapper instead.Based on learnings, the repo’s Win32
GetClientRectwrapper panics on failure and otherwise returns non-nil; minimize/restore crash claims need a repro or stack trace.Proposed cleanup
- // GetClientRect can legitimately return nil for a window in a - // transient state (minimise/restore transitions are especially - // prone to triggering it). FillRect with a nil rect crashes, so it - // must be checked before painting. - if rc := w32.GetClientRect(w.hwnd); rc != nil { - colorRef := w32.COLORREF(uint32(col.Red) | uint32(col.Green)<<8 | uint32(col.Blue)<<16) - hbrush := w32.CreateSolidBrush(colorRef) - w32.FillRect(hdc, rc, hbrush) - w32.DeleteObject(w32.HGDIOBJ(hbrush)) - } + rc := w32.GetClientRect(w.hwnd) + colorRef := w32.COLORREF(uint32(col.Red) | uint32(col.Green)<<8 | uint32(col.Blue)<<16) + hbrush := w32.CreateSolidBrush(colorRef) + w32.FillRect(hdc, rc, hbrush) + w32.DeleteObject(w32.HGDIOBJ(hbrush))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/webview_window_windows.go` around lines 1669 - 1678, The nil-check around w32.GetClientRect in the painting path is dead code because this wrapper panics on failure and otherwise returns a non-nil RECT, so remove the unreachable branch in the window paint logic. If you need to harden the resize/minimize path, do it by switching the GetClientRect call site to an OK-style wrapper or another explicit error-handling path in the same painting method rather than guarding against nil here.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/pkg/application/webview_window_windows.go`:
- Around line 1669-1678: The nil-check around w32.GetClientRect in the painting
path is dead code because this wrapper panics on failure and otherwise returns a
non-nil RECT, so remove the unreachable branch in the window paint logic. If you
need to harden the resize/minimize path, do it by switching the GetClientRect
call site to an OK-style wrapper or another explicit error-handling path in the
same painting method rather than guarding against nil here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d0a6f16-8dc4-4745-968e-0f9d026298cc
📒 Files selected for processing (1)
v3/pkg/application/webview_window_windows.go
There was a problem hiding this comment.
Pull request overview
This PR adjusts WebView2 monitor-scale detection behavior on Windows so DPI/rasterization scale ownership matches the selected WebView2 hosting mode, preventing incorrect scaling under visual hosting while preserving the mixed-DPI GPU-process crash fix for windowed hosting.
Changes:
- Gate
ShouldDetectMonitorScaleChangesenabling/disabling based onUseVisualHosting. - Expand inline documentation in
setupChromium()to describe the rationale for scale ownership per hosting mode.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r state Query GetShouldDetectMonitorScaleChanges after the Put and set the single-writer flag from the controller's real state, not from the value requested. A failed PutShouldDetectMonitorScaleChanges could leave detection at its platform default (enabled) while monitorScaleDetectionOn stayed false, so resyncWebviewRasterizationScale would keep writing PutRasterizationScale concurrently with Edge — the two-writer scale(0,0) race this change removes. If the query itself fails, assume detection may be on and stand the host resync down. Addresses review feedback on wailsapp#5761.
|
Thanks @wayneforrest 🙏 |
… host-owned rasterization scale under visual hosting
Problem
With
UseVisualHosting = trueand resizing a window, from one screen to the next, and pausing midway,and then continue to drag (move) the window to land on the seconds scree, the window would have the wrong DPI applied.
In my testing resize+dragging was done on actual hardware, Windows 11 with a
1080pscreen, and dragging the window to a 4K screen, having a scaling set to 225%. The end result was a too tiny font on the 4K screen. After a window resize, it auto corrects as expected.I tested this change using both
UseVisualHosting = truein the WindowsOptions and having it set toUseVisualHosting = false.My go.mod was pinned to this exact version:
#5734 re-enabled WebView2 automatic monitor-scale detection (
ShouldDetectMonitorScaleChanges) unconditionally insetupChromium, to fix the mixed-DPI GPU-process crash (#5732). That is the right call for the default windowed (HWND-child) hosting mode.It is incorrect under
UseVisualHosting(COREWEBVIEW2_HOSTING_MODE_WINDOW_TO_VISUAL). In that mode the WebView2 content is a DirectComposition visual, decoupled from the child HWND that automatic detection tracks. Dragging across a mixed-DPI monitor boundary then makes detection read the wrong monitor and settle the visual on a stale rasterization scale — the whole UI renders shrunk (tiny fonts) until something forces a re-layout. Repro: two monitors at different scale (e.g. 150% / 100%), drag the window slowly across the boundary, pause, then continue.Fix
Gate the detection-enable on hosting mode in
setupChromium:resyncWebviewRasterizationScale(guarded bymonitorScaleDetectionOn, which stays false). The module leaves detection at its platform default (enabled), so visual hosting must disable it explicitly — merely skipping the enable would leave both Edge's detection and the host resync writing the scale (the two-writer race [v3][windows] Re-enable WebView2 monitor-scale detection to fix mixed-DPI GPU-process crash (#5732) #5734 targets).Host-owned scale is reliable now that the by-value
PutRasterizationScalebug (#5701) is fixed, and it is the documented contract per WebView2Feedback #3665 (detection-off is correct when the host puts the scale on every change).Testing
CGO_ENABLED=0 GOOS=windows GOARCH=amd64 go build ./pkg/application/andgo vetboth clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit