Fix Windows canvas lockup: remove SetWindowPos from blit()#628
Fix Windows canvas lockup: remove SetWindowPos from blit()#628
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.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 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 Migrating from UI to YAML configuration.Use the |
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