From 0f122ca290125951c21d7c830d73a10337bab6b7 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 23 Sep 2021 15:14:03 -0700 Subject: [PATCH] [a11y] Ensure buffer is initialized before interacting with it (#11312) 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 --- src/Terminal.wprp | 2 + src/cascadia/TerminalCore/Terminal.hpp | 1 + .../TerminalCore/terminalrenderdata.cpp | 9 +++ src/host/renderData.hpp | 1 + src/host/ut_host/VtIoTests.cpp | 5 ++ src/types/IUiaData.h | 1 + src/types/ScreenInfoUiaProviderBase.cpp | 25 +++--- src/types/TermControlUiaProvider.cpp | 14 ++-- src/types/TermControlUiaTextRange.cpp | 14 ---- src/types/UiaTextRangeBase.cpp | 80 +++++++++++-------- 10 files changed, 83 insertions(+), 69 deletions(-) diff --git a/src/Terminal.wprp b/src/Terminal.wprp index d37c749c9..80a6e730b 100644 --- a/src/Terminal.wprp +++ b/src/Terminal.wprp @@ -12,6 +12,7 @@ + @@ -23,6 +24,7 @@ + diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index d3b5f1440..5eb4db473 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -199,6 +199,7 @@ public: const COORD GetSelectionEnd() const noexcept override; const std::wstring_view GetConsoleTitle() const noexcept override; void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute) override; + const bool IsUiaDataInitialized() const noexcept override; #pragma endregion void SetWriteInputCallback(std::function pfn) noexcept; diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index 6ccb5791d..1ab2b5af8 100644 --- a/src/cascadia/TerminalCore/terminalrenderdata.cpp +++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp @@ -250,3 +250,12 @@ bool Terminal::IsScreenReversed() const noexcept { return _screenReversed; } + +const bool Terminal::IsUiaDataInitialized() const noexcept +{ + // GH#11135: Windows Terminal needs to create and return an automation peer + // when a screen reader requests it. However, the terminal might not be fully + // initialized yet. So we use this to check if any crucial components of + // UiaData are not yet initialized. + return !!_buffer; +} diff --git a/src/host/renderData.hpp b/src/host/renderData.hpp index 8b858c541..b1699531d 100644 --- a/src/host/renderData.hpp +++ b/src/host/renderData.hpp @@ -69,5 +69,6 @@ public: const COORD GetSelectionAnchor() const noexcept; const COORD GetSelectionEnd() const noexcept; void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute attr); + const bool IsUiaDataInitialized() const noexcept override { return true; } #pragma endregion }; diff --git a/src/host/ut_host/VtIoTests.cpp b/src/host/ut_host/VtIoTests.cpp index 07ec963be..6e8d4e286 100644 --- a/src/host/ut_host/VtIoTests.cpp +++ b/src/host/ut_host/VtIoTests.cpp @@ -392,6 +392,11 @@ public: { } + const bool IsUiaDataInitialized() const noexcept + { + return true; + } + const std::wstring GetHyperlinkUri(uint16_t /*id*/) const noexcept { return {}; diff --git a/src/types/IUiaData.h b/src/types/IUiaData.h index 9cc6b8d9c..a1ea9990b 100644 --- a/src/types/IUiaData.h +++ b/src/types/IUiaData.h @@ -40,6 +40,7 @@ namespace Microsoft::Console::Types virtual const COORD GetSelectionAnchor() const noexcept = 0; virtual const COORD GetSelectionEnd() const noexcept = 0; virtual void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute attr) = 0; + virtual const bool IsUiaDataInitialized() const noexcept = 0; }; // See docs/virtual-dtors.md for an explanation of why this is weird. diff --git a/src/types/ScreenInfoUiaProviderBase.cpp b/src/types/ScreenInfoUiaProviderBase.cpp index b5cb4d442..1d15a837e 100644 --- a/src/types/ScreenInfoUiaProviderBase.cpp +++ b/src/types/ScreenInfoUiaProviderBase.cpp @@ -220,21 +220,19 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::SetFocus() IFACEMETHODIMP ScreenInfoUiaProviderBase::GetSelection(_Outptr_result_maybenull_ SAFEARRAY** ppRetVal) { + RETURN_HR_IF_NULL(E_INVALIDARG, ppRetVal); + *ppRetVal = nullptr; + _LockConsole(); auto Unlock = wil::scope_exit([&]() noexcept { _UnlockConsole(); }); - - RETURN_HR_IF_NULL(E_INVALIDARG, ppRetVal); - *ppRetVal = nullptr; - HRESULT hr = S_OK; + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); // make a safe array + HRESULT hr = S_OK; *ppRetVal = SafeArrayCreateVector(VT_UNKNOWN, 0, 1); - if (*ppRetVal == nullptr) - { - return E_OUTOFMEMORY; - } + RETURN_HR_IF_NULL(E_OUTOFMEMORY, *ppRetVal); WRL::ComPtr range; if (!_pData->IsSelectionActive()) @@ -272,19 +270,18 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetSelection(_Outptr_result_maybenull_ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetVisibleRanges(_Outptr_result_maybenull_ SAFEARRAY** ppRetVal) { + RETURN_HR_IF_NULL(E_INVALIDARG, ppRetVal); + *ppRetVal = nullptr; + _LockConsole(); auto Unlock = wil::scope_exit([&]() noexcept { _UnlockConsole(); }); - - RETURN_HR_IF_NULL(E_INVALIDARG, ppRetVal); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); // make a safe array *ppRetVal = SafeArrayCreateVector(VT_UNKNOWN, 0, 1); - if (*ppRetVal == nullptr) - { - return E_OUTOFMEMORY; - } + RETURN_HR_IF_NULL(E_OUTOFMEMORY, *ppRetVal); WRL::ComPtr range; const auto bufferSize = _pData->GetTextBuffer().GetSize(); diff --git a/src/types/TermControlUiaProvider.cpp b/src/types/TermControlUiaProvider.cpp index c39f7133b..3ced07561 100644 --- a/src/types/TermControlUiaProvider.cpp +++ b/src/types/TermControlUiaProvider.cpp @@ -16,9 +16,6 @@ HRESULT TermControlUiaProvider::RuntimeClassInitialize(_In_ ::Microsoft::Console RETURN_IF_FAILED(ScreenInfoUiaProviderBase::RuntimeClassInitialize(uiaData)); _controlInfo = controlInfo; - - // TODO GitHub #1914: Re-attach Tracing to UIA Tree - //Tracing::s_TraceUia(nullptr, ApiCall::Constructor, nullptr); return S_OK; } @@ -26,11 +23,6 @@ IFACEMETHODIMP TermControlUiaProvider::Navigate(_In_ NavigateDirection direction _COM_Outptr_result_maybenull_ IRawElementProviderFragment** ppProvider) noexcept { RETURN_HR_IF_NULL(E_INVALIDARG, ppProvider); - - // TODO GitHub #1914: Re-attach Tracing to UIA Tree - /*ApiMsgNavigate apiMsg; - apiMsg.Direction = direction; - Tracing::s_TraceUia(this, ApiCall::Navigate, &apiMsg);*/ *ppProvider = nullptr; if (direction == NavigateDirection_Parent) @@ -122,6 +114,12 @@ HRESULT TermControlUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple RETURN_HR_IF_NULL(E_INVALIDARG, ppUtr); *ppUtr = nullptr; + _pData->LockConsole(); + auto Unlock = wil::scope_exit([&]() noexcept { + _pData->UnlockConsole(); + }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized() || !_pData->IsSelectionActive()); + const auto start = _pData->GetSelectionAnchor(); // we need to make end exclusive diff --git a/src/types/TermControlUiaTextRange.cpp b/src/types/TermControlUiaTextRange.cpp index ba5128885..5d95dbd88 100644 --- a/src/types/TermControlUiaTextRange.cpp +++ b/src/types/TermControlUiaTextRange.cpp @@ -63,20 +63,6 @@ IFACEMETHODIMP TermControlUiaTextRange::Clone(_Outptr_result_maybenull_ ITextRan return hr; } -#if defined(_DEBUG) && defined(UiaTextRangeBase_DEBUG_MSGS) - OutputDebugString(L"Clone\n"); - std::wstringstream ss; - ss << _id << L" cloned to " << (static_cast(*ppRetVal))->_id; - std::wstring str = ss.str(); - OutputDebugString(str.c_str()); - OutputDebugString(L"\n"); -#endif - // TODO GitHub #1914: Re-attach Tracing to UIA Tree - // tracing - /*ApiMsgClone apiMsg; - apiMsg.CloneId = static_cast(*ppRetVal)->GetId(); - Tracing::s_TraceUia(this, ApiCall::Clone, &apiMsg);*/ - return S_OK; } diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 08133944c..c32a6a900 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -220,15 +220,18 @@ IFACEMETHODIMP UiaTextRangeBase::CompareEndpoints(_In_ TextPatternRangeEndpoint _Out_ int* pRetVal) noexcept try { - RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr); + RETURN_HR_IF_NULL(E_INVALIDARG, pRetVal); *pRetVal = 0; + _pData->LockConsole(); + auto Unlock = wil::scope_exit([&]() noexcept { + _pData->UnlockConsole(); + }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); + // get the text range that we're comparing to const UiaTextRangeBase* range = static_cast(pTargetRange); - if (range == nullptr) - { - return E_INVALIDARG; - } + RETURN_HR_IF_NULL(E_INVALIDARG, range); // get endpoint value that we're comparing to const auto other = range->GetEndpoint(targetEndpoint); @@ -240,10 +243,7 @@ try // This is a temporary solution to comparing two UTRs from different TextBuffers // Ensure both endpoints fit in the current buffer. const auto bufferSize = _pData->GetTextBuffer().GetSize(); - if (!bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true)) - { - return E_FAIL; - } + RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true)); // compare them *pRetVal = bufferSize.CompareInBounds(mine, other, true); @@ -259,6 +259,7 @@ IFACEMETHODIMP UiaTextRangeBase::ExpandToEnclosingUnit(_In_ TextUnit unit) noexc auto Unlock = wil::scope_exit([&]() noexcept { _pData->UnlockConsole(); }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); try { @@ -444,6 +445,12 @@ try RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr); *ppRetVal = nullptr; + _pData->LockConsole(); + auto Unlock = wil::scope_exit([&]() noexcept { + _pData->UnlockConsole(); + }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); + // AttributeIDs that require special handling switch (attributeId) { @@ -605,6 +612,12 @@ try RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr); *ppRetVal = nullptr; + _pData->LockConsole(); + auto Unlock = wil::scope_exit([&]() noexcept { + _pData->UnlockConsole(); + }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); + const std::wstring queryText{ text, SysStringLen(text) }; const auto bufferSize = _getOptimizedBufferSize(); const auto sensitivity = ignoreCase ? Search::Sensitivity::CaseInsensitive : Search::Sensitivity::CaseSensitive; @@ -730,6 +743,12 @@ try RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr); VariantInit(pRetVal); + _pData->LockConsole(); + auto Unlock = wil::scope_exit([&]() noexcept { + _pData->UnlockConsole(); + }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); + // AttributeIDs that require special handling switch (attributeId) { @@ -817,13 +836,14 @@ CATCH_RETURN(); IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_ SAFEARRAY** ppRetVal) noexcept { + RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr); + *ppRetVal = nullptr; + _pData->LockConsole(); auto Unlock = wil::scope_exit([&]() noexcept { _pData->UnlockConsole(); }); - - RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr); - *ppRetVal = nullptr; + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); try { @@ -925,21 +945,19 @@ CATCH_RETURN(); IFACEMETHODIMP UiaTextRangeBase::GetText(_In_ int maxLength, _Out_ BSTR* pRetVal) noexcept try { - RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr); + RETURN_HR_IF_NULL(E_INVALIDARG, pRetVal); + RETURN_HR_IF(E_INVALIDARG, maxLength < -1); *pRetVal = nullptr; - if (maxLength < -1) - { - return E_INVALIDARG; - } + _pData->LockConsole(); + auto Unlock = wil::scope_exit([&]() noexcept { + _pData->UnlockConsole(); + }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); const auto maxLengthOpt = (maxLength == -1) ? std::nullopt : std::optional{ maxLength }; - _pData->LockConsole(); - auto Unlock = wil::scope_exit([this]() noexcept { - _pData->UnlockConsole(); - }); const auto text = _getTextValue(maxLengthOpt); Unlock.reset(); @@ -1013,6 +1031,7 @@ try auto Unlock = wil::scope_exit([&]() noexcept { _pData->UnlockConsole(); }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); // We can abstract this movement by moving _start // GH#7342: check if we're past the documentEnd @@ -1075,15 +1094,13 @@ IFACEMETHODIMP UiaTextRangeBase::MoveEndpointByUnit(_In_ TextPatternRangeEndpoin { RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr); *pRetVal = 0; - if (count == 0) - { - return S_OK; - } _pData->LockConsole(); auto Unlock = wil::scope_exit([&]() noexcept { _pData->UnlockConsole(); }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); + RETURN_HR_IF(S_OK, count == 0); // GH#7342: check if we're past the documentEnd // If so, clamp each endpoint to the end of the document. @@ -1141,10 +1158,8 @@ try }); const UiaTextRangeBase* range = static_cast(pTargetRange); - if (range == nullptr) - { - return E_INVALIDARG; - } + RETURN_HR_IF_NULL(E_INVALIDARG, range); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); // TODO GH#5406: create a different UIA parent object for each TextBuffer // This is a temporary solution to comparing two UTRs from different TextBuffers @@ -1152,10 +1167,7 @@ try const auto bufferSize = _pData->GetTextBuffer().GetSize(); const auto mine = GetEndpoint(endpoint); const auto other = range->GetEndpoint(targetEndpoint); - if (!bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true)) - { - return E_FAIL; - } + RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true)); SetEndpoint(endpoint, range->GetEndpoint(targetEndpoint)); @@ -1171,6 +1183,7 @@ try auto Unlock = wil::scope_exit([&]() noexcept { _pData->UnlockConsole(); }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); if (IsDegenerate()) { @@ -1215,6 +1228,7 @@ try auto Unlock = wil::scope_exit([&]() noexcept { _pData->UnlockConsole(); }); + RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized()); const auto oldViewport = _pData->GetViewport().ToInclusive(); const auto viewportHeight = _getViewportHeight(oldViewport);