-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[a11y] Ensure buffer is initialized before interacting with it #11312
Conversation
@miniksa made an update to the branch (added comments and initialization checks in ScreenInfoUiaProviderBase). You said Once we verify that this works on Windows Server, this'll be out of draft. |
0900a9e
to
d7750f5
Compare
I've done a manual test on https://microsoft.visualstudio.com/c93e867a-8815-43c1-92c4-e7dd5404f1e1/_build/results?buildId=39411475 on the Windows Server 2022 machine by launching the Terminal built with these changes using the tablet input keyboard and it loads up normally. |
@miniksa we should be able to just cherry-pick this into 1.10 too |
if (!_pData->IsUiaDataInitialized()) | ||
{ | ||
return E_FAIL; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this on the wrong side of the lock? Or alternatively, shouldn't the value be atomic or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh I guess if it comes back as a dirty "false" it only goes to "true" once and a missed early call is OK. So maybe nevermind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lhecker can you comment on this before we merge? Do you think this is OK as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically it's not okay. Practically it'll work on any modern CPU.
The reason it's not save theoretically is because the standard says:
If a data race occurs, the behavior of the program is undefined.
So theoretically speaking, the moment you have a race condition your computer could turn into a mahou shoujo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what's better to you? Moving it all past the existing call to the til::ticket_lock
or using an std::atomic<bool>
on this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm fine shipping this as-is, but the WIL macros are nice
https://microsoft.visualstudio.com/c93e867a-8815-43c1-92c4-e7dd5404f1e1/_build/results?buildId=39422176 works on the Windows Server 2022 machine by launching the Terminal built with these changes using the tablet input keyboard and it loads up normally. (Verified) |
Adds a check before every UIA function call to ensure the terminal (specifically the buffer) is initialized before doing work. Both the `ScreenInfoUiaProvider` and the `UiaTextRange` are now covered. ## References Closes #11135 #10971 & #11042 ## Detailed Description of the Pull Request / Additional comments Originally, I tried applying this heuristic to all the `RuntimeClassInitialize` on `UiaTextRangeBase` with the philosophy of "a range pointing to an invalid buffer is invalid itself", but that caused a regression on [MSFT 33353327](https://microsoft.visualstudio.com/OS/_workitems/edit/33353327). `IUiaData` also has `GetTextBuffer()` return a `TextBuffer&`, which cannot be checked for nullness. Instead, I decided to add a function to `IUiaData` that checks if we have a valid state. Since this is shared with Conhost and Conhost doesn't have this issue, I simply make that function say that it's always in a valid state. ## Validation Steps Performed - [X] Narrator can detect newly created terminals - [X] (On Windows Server 2022) Windows Terminal does not hang on launch
Adds a check before every UIA function call to ensure the terminal (specifically the buffer) is initialized before doing work. Both the `ScreenInfoUiaProvider` and the `UiaTextRange` are now covered. ## References Closes #11135 #10971 & #11042 ## Detailed Description of the Pull Request / Additional comments Originally, I tried applying this heuristic to all the `RuntimeClassInitialize` on `UiaTextRangeBase` with the philosophy of "a range pointing to an invalid buffer is invalid itself", but that caused a regression on [MSFT 33353327](https://microsoft.visualstudio.com/OS/_workitems/edit/33353327). `IUiaData` also has `GetTextBuffer()` return a `TextBuffer&`, which cannot be checked for nullness. Instead, I decided to add a function to `IUiaData` that checks if we have a valid state. Since this is shared with Conhost and Conhost doesn't have this issue, I simply make that function say that it's always in a valid state. ## Validation Steps Performed - [X] Narrator can detect newly created terminals - [X] (On Windows Server 2022) Windows Terminal does not hang on launch
## Summary of the Pull Request The deadlock was caused by `ScreenInfoUiaProviderBase::GetSelection()` calling `TermControlUiaProvider::GetSelectionRange` (both of which attempted to lock the console). This PR removes the lock and initialization check from `TermControlUiaProvider`. It is no longer necessary because the only one that calls it is `SIUPB::GetSelection()`. Additionally, this adds some code that was useful in debugging this race condition. That should help us figure out any locking issues that may come up in the future. ## References #11312 Closes #11385 ## Validation Steps Performed ✅ Repro steps don't cause hang
## Summary of the Pull Request The deadlock was caused by `ScreenInfoUiaProviderBase::GetSelection()` calling `TermControlUiaProvider::GetSelectionRange` (both of which attempted to lock the console). This PR removes the lock and initialization check from `TermControlUiaProvider`. It is no longer necessary because the only one that calls it is `SIUPB::GetSelection()`. Additionally, this adds some code that was useful in debugging this race condition. That should help us figure out any locking issues that may come up in the future. ## References #11312 Closes #11385 ## Validation Steps Performed ✅ Repro steps don't cause hang
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
Summary of the Pull Request
Adds a check before every UIA function call to ensure the terminal (specifically the buffer) is initialized before doing work. Both the
ScreenInfoUiaProvider
and theUiaTextRange
are now covered.References
Closes #11135
#10971 & #11042
Detailed Description of the Pull Request / Additional comments
Originally, I tried applying this heuristic to all the
RuntimeClassInitialize
onUiaTextRangeBase
with the philosophy of "a range pointing to an invalid buffer is invalid itself", but that caused a regression on MSFT 33353327.IUiaData
also hasGetTextBuffer()
return aTextBuffer&
, which cannot be checked for nullness. Instead, I decided to add a function toIUiaData
that checks if we have a valid state. Since this is shared with Conhost and Conhost doesn't have this issue, I simply make that function say that it's always in a valid state.Validation Steps Performed