fix(win32): remove WS_VISIBLE and add assert in GetClientArea #1539
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
Contributor
Reviewer's GuideEnsures Win32Window has a valid window_handle_ immediately after CreateWindow and adds defensive checks around window usage, while gating dark mode window attributes behind an OS version check to avoid crashes on unsupported Windows builds. Sequence diagram for Win32Window creation and safe early usagesequenceDiagram
actor App
participant Win32Window
participant Win32API as Win32_API
participant WndProc
App->>Win32Window: Create(title, origin, size)
Win32Window->>Win32API: CreateWindow(window_class, title, styles, this)
Win32API-->>Win32Window: HWND window
Win32Window->>Win32Window: window_handle_ = window
Win32Window->>Win32API: ShowWindow(window, SW_SHOWNORMAL)
Win32Window->>Win32API: UpdateWindow(window)
Win32Window->>Win32Window: UpdateTheme(window)
Win32Window-->>App: bool created
Note over Win32API,WndProc: During CreateWindow, WM_NCCREATE is sent
Win32API->>WndProc: WM_NCCREATE(window, lpCreateParams=this)
WndProc->>Win32Window: that = lpCreateParams
alt handle not yet set
WndProc->>Win32Window: that->window_handle_ = window
end
App->>Win32Window: Show()
Win32Window->>Win32Window: if (!window_handle_) return false
Win32Window->>Win32API: ShowWindow(window_handle_, SW_SHOWNORMAL)
App->>Win32Window: SetChildContent(content)
Win32Window->>Win32Window: if (!window_handle_) return
Win32Window->>Win32Window: child_content_ = content
Win32Window->>Win32API: SetParent(content, window_handle_)
Win32Window->>Win32Window: frame = GetClientArea()
App->>Win32Window: GetClientArea()
Win32Window->>Win32Window: if (window_handle_) GetClientRect(window_handle_)
Win32Window-->>App: RECT frame
Class diagram for updated Win32Window handle and theme managementclassDiagram
class Win32Window {
HWND window_handle_
HWND child_content_
bool Create(std_wstring title, Point origin, Size size)
bool Show()
void SetChildContent(HWND content)
RECT GetClientArea()
void OnDestroy()
void UpdateTheme(HWND window)
static Win32Window* GetThisFromHandle(HWND window)
}
class GlobalHelpers {
+void EnableFullDpiSupportIfAvailable(HWND hwnd)
+bool IsDarkModeSupported()
}
Win32Window ..> GlobalHelpers : uses
Win32Window ..> HWND : manages
Win32Window ..> RECT : returns
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Changing the window style to
WS_OVERLAPPEDWINDOW | WS_VISIBLEand callingShowWindow/UpdateWindowinsideCreatealters the previous lifecycle contract where callers explicitly decided when to show the window; consider preserving the old behavior and only fixing the race onwindow_handle_initialization. - The new guard clauses that silently no-op when
window_handle_is null (e.g., inShow,SetChildContent,GetClientArea) may hide logic errors; consider surfacing these as explicit failures (assert/log) so callers know they are using the window too early. IsDarkModeSupportedis called on every theme update and usesVerifyVersionInfoW; consider caching its result in astaticor similar to avoid repeated version checks and keep the hot path lightweight.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing the window style to `WS_OVERLAPPEDWINDOW | WS_VISIBLE` and calling `ShowWindow`/`UpdateWindow` inside `Create` alters the previous lifecycle contract where callers explicitly decided when to show the window; consider preserving the old behavior and only fixing the race on `window_handle_` initialization.
- The new guard clauses that silently no-op when `window_handle_` is null (e.g., in `Show`, `SetChildContent`, `GetClientArea`) may hide logic errors; consider surfacing these as explicit failures (assert/log) so callers know they are using the window too early.
- `IsDarkModeSupported` is called on every theme update and uses `VerifyVersionInfoW`; consider caching its result in a `static` or similar to avoid repeated version checks and keep the hot path lightweight.
## Individual Comments
### Comment 1
<location> `windows/runner/win32_window.cpp:57-66` </location>
<code_context>
}
+// Checks if the current Windows version supports dark mode
+bool IsDarkModeSupported() {
+ // DWMWA_USE_IMMERSIVE_DARK_MODE requires Windows 10 Build 17763 (version 1809) or higher
+ OSVERSIONINFOEXW osvi = {};
+ DWORDLONG const dwlConditionMask = VerSetConditionMask(
+ VerSetConditionMask(
+ VerSetConditionMask(
+ 0, VER_MAJORVERSION, VER_GREATER_EQUAL),
+ VER_MINORVERSION, VER_GREATER_EQUAL),
+ VER_BUILDNUMBER, VER_GREATER_EQUAL);
+
+ osvi.dwOSVersionInfoSize = sizeof(osvi);
+ osvi.dwMajorVersion = 10;
+ osvi.dwMinorVersion = 0;
+ osvi.dwBuildNumber = 17763; // Windows 10 version 1809
+
+ return VerifyVersionInfoW(&osvi,
+ VER_MAJORVERSION | VER_MINORVERSION | VER_BUILDNUMBER,
+ dwlConditionMask) != FALSE;
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The explicit OS version check for dark mode support is fragile and may misreport capabilities.
Using `VerifyVersionInfoW` here is fragile: it relies on correct Windows 10+ manifesting and on version helper APIs that are effectively deprecated. If the manifest isn’t aligned, this can report that the OS doesn’t meet the version even when `DWMWA_USE_IMMERSIVE_DARK_MODE` is available, so dark mode never gets enabled. Instead, consider calling `DwmSetWindowAttribute` (optionally after a one-time runtime probe) and checking its return value; older builds will just ignore or fail the call safely.
</issue_to_address>
### Comment 2
<location> `windows/runner/win32_window.cpp:159-160` </location>
<code_context>
double scale_factor = dpi / 96.0;
+ // Create window with pointer to this instance
HWND window = CreateWindow(
- window_class, title.c_str(), WS_OVERLAPPEDWINDOW,
+ window_class, title.c_str(), WS_OVERLAPPEDWINDOW | WS_VISIBLE,
Scale(origin.x, scale_factor), Scale(origin.y, scale_factor),
Scale(size.width, scale_factor), Scale(size.height, scale_factor),
</code_context>
<issue_to_address>
**issue (bug_risk):** Making the window visible in CreateWindow changes the lifecycle and may cause unexpected early showing.
This also causes the window to be shown twice: once by `WS_VISIBLE` during `CreateWindow`, and again in `Create()`/`Show()`. That can break callers that rely on `OnCreate` finishing or on explicitly deciding when to show the window, and may introduce flicker. Consider either removing `WS_VISIBLE` and keeping the explicit `ShowWindow` calls, or keeping `WS_VISIBLE` and making `Show()` a no-op when the window is already visible.
</issue_to_address>
### Comment 3
<location> `windows/runner/win32_window.cpp:169-176` </location>
<code_context>
+ // Set window_handle_ immediately after CreateWindow returns
+ window_handle_ = window;
+
+ // Show the window immediately
+ ShowWindow(window, SW_SHOWNORMAL);
+ UpdateWindow(window);
+
UpdateTheme(window);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Showing/updating the window inside Create() may not align with callers’ expectations about when the window becomes visible.
Having `Create()` also call `ShowWindow`/`UpdateWindow` removes the caller’s ability to control when the window becomes visible and may introduce re-entrancy if messages are processed during initialization. If `Create()` previously only established the handle and `Show()` managed visibility, consider preserving that contract and keeping the window hidden here, or making `Show()` a no-op that doesn’t perform additional work beyond what `Create()` already did.
```suggestion
// Set window_handle_ immediately after CreateWindow returns
window_handle_ = window;
UpdateTheme(window);
```
</issue_to_address>
### Comment 4
<location> `windows/runner/win32_window.cpp:291-293` </location>
<code_context>
RECT Win32Window::GetClientArea() {
RECT frame;
- GetClientRect(window_handle_, &frame);
+ if (window_handle_) {
+ GetClientRect(window_handle_, &frame);
+ } else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning a zero rect for a null window_handle_ might mask incorrect call sites.
This avoids a null dereference but may hide invalid state and let callers proceed with a bogus size. Consider making the invalid state explicit (e.g., a debug assert or documented precondition that `window_handle_` must be valid) so layout logic doesn’t silently treat the client area as legitimately empty.
Suggested implementation:
```cpp
RECT Win32Window::GetClientArea() {
// GetClientArea assumes that window_handle_ has been created and is valid.
// A null window_handle_ indicates a usage error at the call site.
RECT frame{0, 0, 0, 0};
assert(window_handle_ && "GetClientArea called with null window_handle_");
if (window_handle_) {
GetClientRect(window_handle_, &frame);
}
return frame;
}
```
1. Ensure `#include <cassert>` (or your project's equivalent debug/assert header) is present near the top of `windows/runner/win32_window.cpp` so that `assert` is available.
2. Optionally, update any documentation or comments for `Win32Window` to state that `GetClientArea` requires a valid `window_handle_` and should only be called after the window has been created.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
g-k-s-03
force-pushed
the
fix-win32-window-handle-race
branch
from
6959ed6 to
923dea0
Compare
g-k-s-03
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
closes #1528
Description:
Previously, window_handle_ was set only in WndProc during WM_NCCREATE. This caused a race condition if methods like Show(), SetChildContent(), or GetClientArea() were called immediately after CreateWindow(), potentially leading to crashes.
Changes:
Set window_handle_ immediately after CreateWindow() in Win32Window::Create().
Added a safeguard in WndProc to ensure window_handle_ is set.
Updated dark mode handling to check Windows version before applying attributes.
Impact:
Calls to window methods are now safe immediately after creation.
Prevents crashes due to uninitialized window handle.
Testing:
Verified Show(), SetChildContent(), GetClientArea(), and Destroy() immediately after creation.
Dark mode behavior tested on supported Windows builds.
Summary by Sourcery
Resolve race conditions around Win32 window handle initialization and make dark mode handling conditional on OS support.
Bug Fixes:
Enhancements: