[a11y] Ensure buffer is initialized before interacting with it#11312
[a11y] Ensure buffer is initialized before interacting with it#11312carlos-zamora merged 3 commits intomainfrom
Conversation
ghost
added
Area-TerminalControl
|
@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. |
carlos-zamora
force-pushed
the
dev/cazamor/a11y-initialize-hang
branch
from
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.
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.
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.
@lhecker can you comment on this before we merge? Do you think this is OK as is?
There was a problem hiding this comment.
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.
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?
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
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: |
ghost
mentioned this pull request
|
🎉 Handy links: |
|
🎉 Handy links: |
4 participants
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
ScreenInfoUiaProviderand theUiaTextRangeare now covered.References
Closes #11135
#10971 & #11042
Detailed Description of the Pull Request / Additional comments
Originally, I tried applying this heuristic to all the
RuntimeClassInitializeonUiaTextRangeBasewith the philosophy of "a range pointing to an invalid buffer is invalid itself", but that caused a regression on MSFT 33353327.IUiaDataalso hasGetTextBuffer()return aTextBuffer&, which cannot be checked for nullness. Instead, I decided to add a function toIUiaDatathat 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