Skip to content

Commit

Permalink
[a11y] Ensure buffer is initialized before interacting with it (#11312)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
carlos-zamora committed Sep 23, 2021
1 parent 0480597 commit b0cb46f
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 69 deletions.
2 changes: 2 additions & 0 deletions src/Terminal.wprp
Expand Up @@ -12,6 +12,7 @@
<EventProvider Id="EventProvider_TerminalWin32Host" Name="56c06166-2e2e-5f4d-7ff3-74f4b78c87d6" />
<EventProvider Id="EventProvider_TerminalRemoting" Name="d6f04aad-629f-539a-77c1-73f5c3e4aa7b" />
<EventProvider Id="EventProvider_TerminalDirectX" Name="c93e739e-ae50-5a14-78e7-f171e947535d" />
<EventProvider Id="EventProvider_TerminalUIA" Name="e7ebce59-2161-572d-b263-2f16a6afb9e5"/>
<Profile Id="Terminal.Verbose.File" Name="Terminal" Description="Terminal" LoggingMode="File" DetailLevel="Verbose">
<Collectors>
<EventCollectorId Value="EventCollector_Terminal">
Expand All @@ -23,6 +24,7 @@
<EventProviderId Value="EventProvider_TerminalWin32Host" />
<EventProviderId Value="EventProvider_TerminalRemoting" />
<EventProviderId Value="EventProvider_TerminalDirectX" />
<EventProviderId Value="EventProvider_TerminalUIA" />
</EventProviders>
</EventCollectorId>
</Collectors>
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalCore/Terminal.hpp
Expand Up @@ -199,6 +199,7 @@ class Microsoft::Terminal::Core::Terminal final :
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<void(std::wstring&)> pfn) noexcept;
Expand Down
9 changes: 9 additions & 0 deletions src/cascadia/TerminalCore/terminalrenderdata.cpp
Expand Up @@ -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;
}
1 change: 1 addition & 0 deletions src/host/renderData.hpp
Expand Up @@ -69,5 +69,6 @@ class RenderData final :
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
};
5 changes: 5 additions & 0 deletions src/host/ut_host/VtIoTests.cpp
Expand Up @@ -392,6 +392,11 @@ class MockRenderData : public IRenderData, IUiaData
{
}

const bool IsUiaDataInitialized() const noexcept
{
return true;
}

const std::wstring GetHyperlinkUri(uint16_t /*id*/) const noexcept
{
return {};
Expand Down
1 change: 1 addition & 0 deletions src/types/IUiaData.h
Expand Up @@ -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.
Expand Down
25 changes: 11 additions & 14 deletions src/types/ScreenInfoUiaProviderBase.cpp
Expand Up @@ -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<UiaTextRangeBase> range;
if (!_pData->IsSelectionActive())
Expand Down Expand Up @@ -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<UiaTextRangeBase> range;
const auto bufferSize = _pData->GetTextBuffer().GetSize();
Expand Down
14 changes: 6 additions & 8 deletions src/types/TermControlUiaProvider.cpp
Expand Up @@ -16,21 +16,13 @@ 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;
}

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)
Expand Down Expand Up @@ -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
Expand Down
14 changes: 0 additions & 14 deletions src/types/TermControlUiaTextRange.cpp
Expand Up @@ -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<UiaTextRangeBase*>(*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<UiaTextRangeBase*>(*ppRetVal)->GetId();
Tracing::s_TraceUia(this, ApiCall::Clone, &apiMsg);*/

return S_OK;
}

Expand Down
80 changes: 47 additions & 33 deletions src/types/UiaTextRangeBase.cpp
Expand Up @@ -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<UiaTextRangeBase*>(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);
Expand All @@ -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);
Expand All @@ -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
{
Expand Down Expand Up @@ -446,6 +447,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)
{
Expand Down Expand Up @@ -607,6 +614,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 = _getBufferSize();
const auto sensitivity = ignoreCase ? Search::Sensitivity::CaseInsensitive : Search::Sensitivity::CaseSensitive;
Expand Down Expand Up @@ -732,6 +745,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)
{
Expand Down Expand Up @@ -819,13 +838,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
{
Expand Down Expand Up @@ -927,21 +947,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<unsigned int>{ maxLength };
_pData->LockConsole();
auto Unlock = wil::scope_exit([this]() noexcept {
_pData->UnlockConsole();
});
const auto text = _getTextValue(maxLengthOpt);
Unlock.reset();

Expand Down Expand Up @@ -1015,6 +1033,7 @@ try
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
});
RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized());

const auto wasDegenerate = IsDegenerate();
if (count != 0)
Expand Down Expand Up @@ -1070,15 +1089,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);

try
{
Expand Down Expand Up @@ -1116,21 +1133,16 @@ try
});

const UiaTextRangeBase* range = static_cast<UiaTextRangeBase*>(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
// Ensure both endpoints fit in the current buffer.
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));

Expand All @@ -1146,6 +1158,7 @@ try
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
});
RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized());

if (IsDegenerate())
{
Expand Down Expand Up @@ -1190,6 +1203,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);
Expand Down

0 comments on commit b0cb46f

Please sign in to comment.