Track menus per thread, not per process - #3
Merged
Merged
Conversation
The guard that stops an HMENU being destroyed while Windows is displaying it counted in a plain static field. TrackPopupMenuEx runs its nested message loop on the calling thread and the menu it shows belongs to that thread, so the depth belongs to the thread too. It surfaced as a CI failure that passed on the branch and failed on master: xUnit runs test classes in parallel, StaRunner gives each test its own thread, and two classes deliberately enter a tracking scope to prove that Rebuild refuses while a popup is open. While either held the scope, an unrelated rebuild in a third class threw "Cannot rebuild the menu bar while a popup menu is open". A race, so it failed intermittently. The same hole is reachable outside the tests. WinForms permits more than one UI thread, each with its own pump, and a popup tracked on one of them would have blocked a legitimate rebuild on another. ThreadStatic is what the type's own remarks always claimed it was. The regression test holds a scope on one STA thread and rebuilds a menu bar on another; removing ThreadStatic again makes it fail. Also, in the sample: the category tree right-aligned its text in Hebrew but never mirrored, because RightToLeft is an ambient property and RightToLeftLayout is not. An unmirrored tree keeps its left-to-right key bindings, so Right arrow expanded a node whose children were drawn to the left. The listening script now states which arrow should do what, since that is not something a reader can infer. Claude-Session: https://claude.ai/code/session_01TJ8i7jHmkUjVCjVccLp6Hf
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
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.
Summary
Fixes the CI failure on master.
MenuTrackingScopecounted tracking depth in a plainstaticfield; it is now[ThreadStatic].A menu belongs to the thread that created it —
TrackPopupMenuExruns its nested message loop on the calling thread — so the depth belongs to the thread too.Why it failed on master but not on the branch
It is a race. xUnit runs test classes in parallel,
StaRunnergives each test its own thread, and two test classes deliberately enter a tracking scope to proveRebuildrefuses while a popup is open. While either held the scope, an unrelated rebuild in a third class threwCannot rebuild the menu bar while a popup menu is open.Not only a test problem
WinForms permits more than one UI thread, each with its own pump. A popup tracked on one would have blocked a legitimate rebuild on another.
[ThreadStatic]is what the type's own remarks already claimed it was.Verification
Tracking_OnOneThread_DoesNotBlockAnotherholds a scope on one STA thread and rebuilds a menu bar on another. Removing[ThreadStatic]again makes it fail — checked, not assumed. 154 tests pass, three consecutive runs.Also included
The sample's category tree right-aligned its text in Hebrew but never mirrored:
RightToLeftis ambient,RightToLeftLayoutis not. An unmirrored tree keeps its left-to-right key bindings, so Right arrow expanded a node whose children were drawn to the left. Verified via the window'sWS_EX_LAYOUTRTLbefore and after.https://claude.ai/code/session_01TJ8i7jHmkUjVCjVccLp6Hf