Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix two race conditions around pseudo window visibility #13832

Merged
1 commit merged into from Aug 24, 2022
Merged

Fix two race conditions around pseudo window visibility #13832

1 commit merged into from Aug 24, 2022

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Aug 23, 2022

This commit fixes two race conditions:

  • SetPseudoWindowCallback set the _pseudoWindowMessageCallback
    callback after the Win32 message thread was already spawned.
    This issue was fixed by instead using the ServiceLocator to get
    a hold of the global VtIo instance which is created statically.
  • XtermEngine::SetWindowVisibility was called without holding the
    console lock. This issue was fixed by simply acquiring it first.

Closes MSFT:40913882

Validation Steps Performed

  • Add IsConsoleLocked() assertion in VtEngine::_Write
  • Run Windows Terminal
  • No assertion failures ✅

@lhecker lhecker added Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Crash Crashes are real bad news. Priority-0 Bugs that we consider release-blocking/recall-class (P0) labels Aug 23, 2022
@DHowett
Copy link
Member

DHowett commented Aug 23, 2022

@msftbot make sure @zadjii-msft signs off

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 23, 2022
@ghost
Copy link

ghost commented Aug 23, 2022

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @zadjii-msft

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@DHowett
Copy link
Member

DHowett commented Aug 23, 2022

I think this will primarily hit when there is (1) a lot of VT output and (2) an application calls ShowWindow(GetConsoleWindow(), SW_MINIMIZE) a lot. We may want to just... build a quick stress harness and make sure we can repro this before we call it fixed.

Thoughts?

@ghost ghost merged commit 4f3afee into main Aug 24, 2022
@ghost ghost deleted the dev/lhecker/40913882-visibility-race branch August 24, 2022 00:06
@AveYo AveYo mentioned this pull request Sep 2, 2022
Closed
@DHowett
Copy link
Member

DHowett commented Sep 6, 2022

@zadjii-msft should this be part of 1.15?

@zadjii-msft zadjii-msft added this to To Cherry Pick in 1.15 Servicing Pipeline via automation Sep 6, 2022
@zadjii-msft
Copy link
Member

@DHowett almost definitely

@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.15 Servicing Pipeline Sep 6, 2022
DHowett pushed a commit that referenced this pull request Sep 6, 2022
@lhecker @DHowett
Fix two race conditions around pseudo window visibility (#13832)
c413036 
This commit fixes two race conditions:
* `SetPseudoWindowCallback` set the `_pseudoWindowMessageCallback`
  callback after the Win32 message thread was already spawned.
  This issue was fixed by instead using the `ServiceLocator` to get
  a hold of the global `VtIo` instance which is created statically.
* `XtermEngine::SetWindowVisibility` was called without holding the
  console lock. This issue was fixed by simply acquiring it first.

Closes MSFT:40913882

## Validation Steps Performed
* Add `IsConsoleLocked()` assertion in `VtEngine::_Write`
* Run Windows Terminal
* No assertion failures ✅

(cherry picked from commit 4f3afee)
Service-Card-Id: 85487461
Service-Version: 1.15
@ghost
Copy link

ghost commented Sep 13, 2022

🎉Windows Terminal v1.15.252 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 13, 2022

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

@DHowett DHowett moved this from Cherry Picked to Shipped in 1.15 Servicing Pipeline Sep 21, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees
No one assigned
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conpty For console issues specifically related to conpty Severity-Crash Crashes are real bad news.
Projects
No open projects
1.15 Servicing Pipeline
Shipped
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lhecker @DHowett @zadjii-msft