Fix Windows canvas lockup: remove SetWindowPos from blit()#628
Fix Windows canvas lockup: remove SetWindowPos from blit()#628highperformancecoder merged 6 commits intomasterfrom
Conversation
…_PAINT handler - RESTService/addon.cc: Make destroyFrame synchronous on Windows/macOS like renderFrame. The child window is created on the JS thread (via synchronous renderFrame), so destroyFrame must also run on the JS thread to avoid cross-thread SendMessageA calls in ~WindowInformation() that block the minsky thread while holding minskyCmdMutex, causing a deadlock when the JS thread tries to acquire minskyCmdMutex. - model/windowInformation.cc: Add ValidateRect in windowProc WM_PAINT handler when GWLP_USERDATA is 0 to prevent infinite WM_PAINT loop when window is being destroyed. - model/windowInformation.cc: Add SWP_NOACTIVATE flag to SetWindowPos in blit() to prevent unnecessary window activation messages on every paint, reducing message cascade issues with screen sharing tools like Zoom. Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
…SetWindowPos from blit - RESTService/addon.cc: drawNativeWindows() now swaps nativeWindowsToRedraw under minskyCmdMutex then releases the lock before calling draw(), matching the existing macOSXDrawNativeWindows() pattern. Previously holding minskyCmdMutex across the entire draw() call could deadlock: GDI/Cairo operations inside draw() can need the JS thread's message pump while the JS thread is blocked trying to acquire minskyCmdMutex for a synchronous command. - model/windowInformation.cc: Removed SetWindowPos(HWND_TOP,...) from blit(). Calling it on every WM_PAINT fired WM_WINDOWPOSCHANGED messages that could cascade into further WM_PAINT messages and triggered screen-sharing hooks (e.g. Zoom) on every paint, keeping the JS thread's message pump busy and preventing it from returning to the libuv event loop to service minsky thread BlockingCall resolutions. - RESTService/addon.cc: Corrected the comment on synchronous destroyFrame dispatch: the real reason is that Windows requires a window to be destroyed on the same thread that created it, not (as previously stated) that minskyCmdMutex blocks windowProc. - model/windowInformation.cc: Expanded the ValidateRect else-branch comment to clarify when GWLP_USERDATA is zero (WindowInformation already torn down). Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
RESTService/addon.cc
Outdated
| decltype(nativeWindowsToRedraw) windowsToRedraw; | ||
| { | ||
| const lock_guard<recursive_mutex> lock(minskyCmdMutex); | ||
| windowsToRedraw.swap(nativeWindowsToRedraw); |
There was a problem hiding this comment.
The whole purpose of this mutex is to prevent the model from changing whilst a view is being redrawn. This change invalidates that whole promise.
There was a problem hiding this comment.
Reverted in 718825d. The mutex must be held during draw() to protect the model — releasing it before the draw was incorrect.
RESTService/addon.cc
Outdated
| // ~WindowInformation() (and its SendMessageA/SendMessageW) from a thread | ||
| // that does not own the child HWND, which is undefined behaviour and can | ||
| // cause hangs or crashes. | ||
| if (command.ends_with(".renderFrame") || command.ends_with(".destroyFrame")) |
There was a problem hiding this comment.
destroyFrame does not actually destroy the window, but rather posts a WM_CLOSE on the window. I don;t think this fixes anything.
There was a problem hiding this comment.
@copilot respond to comments
There was a problem hiding this comment.
Reverted in 718825d. Since destroyFrame only posts WM_CLOSE (via SendMessageA in ~WindowInformation), making it synchronous on the JS thread doesn't address the deadlock. Only renderFrame remains in the synchronous dispatch list as it was originally.
There was a problem hiding this comment.
Both issues were addressed in 718825d: the destroyFrame addition was reverted (only renderFrame remains in the synchronous dispatch list), and drawNativeWindows() was restored to hold minskyCmdMutex across draw(). The only net change in this PR is the removal of SetWindowPos from blit() in windowInformation.cc.
…ispatch per review feedback - Revert drawNativeWindows() back to holding minskyCmdMutex during draw() as intended: the mutex protects the model from being modified while a view is being redrawn, and releasing it before draw() would break that invariant. - Revert destroyFrame from the synchronous dispatch list: destroyFrame only posts WM_CLOSE to the window and does not fix the underlying deadlock. Only renderFrame needs to be dispatched synchronously on the JS thread. The only change retained in this PR is the removal of SetWindowPos from blit() in windowInformation.cc, which prevents cascaded WM_WINDOWPOSCHANGED/ WM_PAINT messages that kept the JS thread's message pump busy indefinitely. Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughModified Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific canvas lockup by preventing a paint → window-position-change → paint cascade, and adds safety handling for post-destruction paint messages.
Changes:
- Removed
SetWindowPos(HWND_TOP, ...)fromblit()to stopWM_WINDOWPOSCHANGEDbeing triggered on everyWM_PAINT. - Added a
ValidateRectfallback whenGWLP_USERDATAindicatesWindowInformationhas been torn down. - Minor cleanups in tests and value-initialization for schema generation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testPubTab.cc | Removes unused dynamic_cast result variable in a test loop. |
| test/testMinsky.cc | Removes unused variableCast() result variable in a test. |
| schema/optional.h | Value-initializes a temporary to avoid indeterminate state during XSD generation. |
| model/windowInformation.cc | Removes SetWindowPos from blit() and validates paint rects after teardown to prevent infinite WM_PAINT. |
| RavelCAPI | Updates submodule pointer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| else | ||
| // GWLP_USERDATA is zeroed in ~WindowInformation before the window is | ||
| // closed, so reaching here means the WindowInformation has already been | ||
| // torn down. Validate the rect so Windows stops generating WM_PAINT | ||
| // messages for a window that no longer has a backing buffer. | ||
| ValidateRect(hwnd, nullptr); |
| // Note: SetWindowPos was previously called here on every blit to keep the | ||
| // child window at HWND_TOP, but that fired WM_WINDOWPOSCHANGED on every | ||
| // paint which could cascade into further WM_PAINT messages and interact | ||
| // badly with screen-sharing hooks (e.g. Zoom). The window is already | ||
| // positioned correctly at creation time in WindowInformation(). |
Random canvas lockup during editing on Windows, caused by
SetWindowPos(HWND_TOP,…)being called insideblit()on everyWM_PAINT.Root cause —
SetWindowPos(HWND_TOP,…)on everyblit()blit()calledSetWindowPos(HWND_TOP,…)at the end of everyWM_PAINT. This firesWM_WINDOWPOSCHANGEDwhich can re-invalidate the window, producing aWM_PAINT→blit()→SetWindowPos→WM_PAINTcascade. With screen-sharing hooks (e.g. Zoom) eachSetWindowPoscall can be arbitrarily slow, keeping the JS thread trapped in the Windows message pump indefinitely — preventing it from returning to the libuv event loop to service pending work.This is also why
~WindowInformationclears the lockup: zeroingGWLP_USERDATAand sending a syntheticWM_PAINTbreaks the cascade by routing subsequent paint messages toValidateRectinstead ofblit().Removed
SetWindowPosfromblit(). The child HWND is already correctly positioned and z-ordered at creation time inWindowInformation().Also
ValidateRectelse-branch: addedValidateRectcall with a comment clarifying thatGWLP_USERDATA == 0meansWindowInformationhas already been torn down, preventing an infiniteWM_PAINTloop after destruction.📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.
This change is
Summary by CodeRabbit