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

[a11y] Ensure buffer is initialized before interacting with it #11312

Merged
merged 3 commits into from
Sep 23, 2021
Merged

[a11y] Ensure buffer is initialized before interacting with it #11312

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7750f5
fix hang
carlos-zamora Sep 22, 2021
047fe91
fix MockRenderData
carlos-zamora Sep 23, 2021
dd2f704
consistently lock before checking if initialized
carlos-zamora Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Terminal.wprp
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
12 changes: 10 additions & 2 deletions src/types/ScreenInfoUiaProviderBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,10 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetSelection(_Outptr_result_maybenull_
{
return E_OUTOFMEMORY;
}
else if (!_pData->IsUiaDataInitialized())
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
return E_FAIL;
}

WRL::ComPtr<UiaTextRangeBase> range;
if (!_pData->IsSelectionActive())
Expand Down Expand Up @@ -272,13 +276,17 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetSelection(_Outptr_result_maybenull_

IFACEMETHODIMP ScreenInfoUiaProviderBase::GetVisibleRanges(_Outptr_result_maybenull_ SAFEARRAY** ppRetVal)
{
RETURN_HR_IF_NULL(E_INVALIDARG, ppRetVal);
if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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?

_LockConsole();
auto Unlock = wil::scope_exit([&]() noexcept {
_UnlockConsole();
});

RETURN_HR_IF_NULL(E_INVALIDARG, ppRetVal);

// make a safe array
*ppRetVal = SafeArrayCreateVector(VT_UNKNOWN, 0, 1);
if (*ppRetVal == nullptr)
Expand Down
12 changes: 4 additions & 8 deletions src/types/TermControlUiaProvider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
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 @@ -121,6 +113,10 @@ HRESULT TermControlUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple
{
RETURN_HR_IF_NULL(E_INVALIDARG, ppUtr);
*ppUtr = nullptr;
if (!_pData->IsUiaDataInitialized() || !_pData->IsSelectionActive())
{
return E_FAIL;
}

const auto start = _pData->GetSelectionAnchor();

Expand Down
14 changes: 0 additions & 14 deletions src/types/TermControlUiaTextRange.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
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
60 changes: 59 additions & 1 deletion src/types/UiaTextRangeBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,11 @@ try
RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr);
*pRetVal = 0;

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

// get the text range that we're comparing to
const UiaTextRangeBase* range = static_cast<UiaTextRangeBase*>(pTargetRange);
if (range == nullptr)
Expand Down Expand Up @@ -260,6 +265,11 @@ IFACEMETHODIMP UiaTextRangeBase::ExpandToEnclosingUnit(_In_ TextUnit unit) noexc
_pData->UnlockConsole();
});

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

try
{
_expandToEnclosingUnit(unit);
Expand Down Expand Up @@ -444,6 +454,11 @@ try
RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr);
*ppRetVal = nullptr;

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

// AttributeIDs that require special handling
switch (attributeId)
{
Expand Down Expand Up @@ -605,6 +620,11 @@ try
RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr);
*ppRetVal = nullptr;

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

const std::wstring queryText{ text, SysStringLen(text) };
const auto bufferSize = _getOptimizedBufferSize();
const auto sensitivity = ignoreCase ? Search::Sensitivity::CaseInsensitive : Search::Sensitivity::CaseSensitive;
Expand Down Expand Up @@ -730,6 +750,11 @@ try
RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr);
VariantInit(pRetVal);

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

// AttributeIDs that require special handling
switch (attributeId)
{
Expand Down Expand Up @@ -825,6 +850,11 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_
RETURN_HR_IF(E_INVALIDARG, ppRetVal == nullptr);
*ppRetVal = nullptr;

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

try
{
// vector to put coords into. they go in as four doubles in the
Expand Down Expand Up @@ -933,6 +963,11 @@ try
return E_INVALIDARG;
}

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

const auto maxLengthOpt = (maxLength == -1) ?
std::nullopt :
std::optional<unsigned int>{ maxLength };
Expand Down Expand Up @@ -1009,6 +1044,11 @@ try
RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr);
*pRetVal = 0;

if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

_pData->LockConsole();
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
Expand Down Expand Up @@ -1075,7 +1115,11 @@ IFACEMETHODIMP UiaTextRangeBase::MoveEndpointByUnit(_In_ TextPatternRangeEndpoin
{
RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr);
*pRetVal = 0;
if (count == 0)
if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}
else if (count == 0)
{
return S_OK;
}
Expand Down Expand Up @@ -1145,6 +1189,10 @@ try
{
return E_INVALIDARG;
}
else if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

// TODO GH#5406: create a different UIA parent object for each TextBuffer
// This is a temporary solution to comparing two UTRs from different TextBuffers
Expand All @@ -1167,6 +1215,11 @@ CATCH_RETURN();
IFACEMETHODIMP UiaTextRangeBase::Select() noexcept
try
{
if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

_pData->LockConsole();
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
Expand Down Expand Up @@ -1211,6 +1264,11 @@ IFACEMETHODIMP UiaTextRangeBase::RemoveFromSelection() noexcept
IFACEMETHODIMP UiaTextRangeBase::ScrollIntoView(_In_ BOOL alignToTop) noexcept
try
{
if (!_pData->IsUiaDataInitialized())
{
return E_FAIL;
}

_pData->LockConsole();
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
Expand Down