fix(v3/w32): call GetStockObject instead of GetDeviceCaps - #5875
fix(v3/w32): call GetStockObject instead of GetDeviceCaps#5875roachadam wants to merge 2 commits into
Conversation
github-actions
Bot
added
Documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChanges
Windows GDI wrapper fix
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 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. Comment |
358f49a to
dfd883b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@v3/pkg/w32/gdi32_windows_test.go`:
- Around line 53-55: Extend the Windows test around SelectObject to cover
failure by calling it with an invalid GDI object or device context and asserting
it returns 0, confirming failures do not panic. Capture the return value from
the restoration call and assert it equals font, while preserving the existing
successful-selection assertions.
🪄 Autofix
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cd0d9416-3ee5-4a2f-915a-b4e786ef1ef9
📒 Files selected for processing (3)
v3/UNRELEASED_CHANGELOG.mdv3/pkg/w32/gdi32.gov3/pkg/w32/gdi32_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- v3/pkg/w32/gdi32.go
- v3/UNRELEASED_CHANGELOG.md
| previous := w32.SelectObject(hdc, font) | ||
| i.True(previous != 0) | ||
| w32.SelectObject(hdc, previous) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cover failed selections and verify restoration.
The test only exercises successful SelectObject calls. A regression to the old panic-on-failure behavior would pass this test. Add a Windows test with an invalid GDI object or device context and assert that w32.SelectObject(...) returns 0. Also capture the return value on Line 55 and assert that it equals font; the current call does not verify restoration.
🤖 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/w32/gdi32_windows_test.go` around lines 53 - 55, Extend the Windows
test around SelectObject to cover failure by calling it with an invalid GDI
object or device context and asserting it returns 0, confirming failures do not
panic. Capture the return value from the restoration call and assert it equals
font, while preserving the existing successful-selection assertions.
roachadam
changed the title
GetStockObject called procGetDeviceCaps rather than procGetStockObject, even though procGetStockObject is declared a few lines above it. GetDeviceCaps takes (HDC, index), so passing a stock-object constant as the HDC is an invalid handle and the call returns 0. The function returned 0 for every input and had never worked. It has no callers in v3, which is why it went unnoticed - the bug only surfaces for a new caller, and then as a GDI failure rather than anything obvious. Adds a Windows regression test that fails on the pre-fix code: the returned handles must be non-zero, and the DEFAULT_GUI_FONT handle must be usable when selected into a memory DC. This is the same class of defect as the DestroyMenu argument bug already covered by pkg/w32/menu_windows_test.go.
dfd883b to
244cc48
Compare
|
❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
|
@copilot resolve the merge conflicts on this branch. |
3 participants
Description
w32.GetStockObjectinvoked the wrong Win32 export and returned 0 for every input. It calledprocGetDeviceCapsrather thanprocGetStockObject, even thoughprocGetStockObjectis declared a few lines above atgdi32.go:40and is otherwise unused.GetDeviceCapstakes(HDC, index), so passing a stock-object constant such asDEFAULT_GUI_FONT(17) as theHDCis an invalid handle and the call returns 0. The function had never worked.There are no callers in v3 today, which is why it went unnoticed. It only surfaces for a new caller, and then indirectly — the null handle is passed to some other GDI function, which fails for reasons that point away from the actual cause.
This is the same class of defect as the
DestroyMenuargument bug already covered bypkg/w32/menu_windows_test.go, where a syscall wrapper passed the wrong arguments and silently returned failure for every call.Fixes #5874
Type of change
How Has This Been Tested?
Adds
v3/pkg/w32/gdi32_windows_test.go, which runs on Windows CI via the//go:build windowsconstraint. It fails on the pre-fix code, where every call returns 0.The test checks several stock objects are non-zero, then verifies the
DEFAULT_GUI_FONThandle is genuinely usable — selecting it into a memory DC (not the screen DC, so there are no side effects on the CI desktop) and confirming a previous object comes back.No manual testing was required; the behaviour is fully reproducible in CI.
GOOS=windows GOARCH=amd64 go build ./pkg/w32/ ./pkg/application/succeeds, andgo vet ./pkg/w32/reports the same pre-existing findings as master — this branch adds none.Since
GetStockObjecthas no callers in v3, this cannot regress existing behaviour.Not applicable to macOS or Linux: the change is inside a
//go:build windowsfile.Test Configuration
Verified via the added test on Windows CI. My development machine is macOS, so I cannot provide
wails3 doctoroutput; the test is the verification here.Checklist:
website/src/pages/changelog.mdxwith details of this PR (v3 changelog entries are added automatically) — n/a, v3 changeI have also added an entry to
v3/UNRELEASED_CHANGELOG.mdto control the wording, as described in CONTRIBUTING.md.Note on a related observation: while confirming this, I noticed
SelectObjectpanics on failure rather than returning 0, which turns a null handle from this bug into a process termination since it is called from inside Windows message handlers. I deliberately did not change it here — panicking on failure is the established convention acrosspkg/w32(39 call sites across 10 files, includingSetTextColorandSetBkModein this same file), so altering one function in isolation would be arbitrary. Happy to raise it separately as a design question.AI Usage Disclosure: I used AI in investigating and preparing this change to confirm my own initial findings.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation