mirror of
https://github.com/microsoft/terminal.git
synced 2026-07-08 17:46:05 +00:00
Fix memory leaks with UIA (#19950)
## Summary of the Pull Request This includes the memory leak fixes that @lhecker and I investigated as a part of #19710. The `ITextRangeProvider`s (namely `UiaTextRange`s) weren't being destroyed after they were done being used by the screen reader. ## Validation Steps Performed In my own testing, I set a breakpoint on the destructor for `UiaTextRangeBase`. Prior to this change, that destructor would mainly be called when the terminal control was closed, which would result in us leaking these objects. With this change, I've confirmed that these text ranges are being destroyed immediately after they are done being used (without needing to close the terminal control). ## PR Checklist Closes #19710
This commit is contained in:
1
.github/actions/spelling/expect/expect.txt
vendored
1
.github/actions/spelling/expect/expect.txt
vendored
@@ -1772,6 +1772,7 @@ uldash
|
||||
uldb
|
||||
ULONGLONG
|
||||
ulwave
|
||||
Unaccess
|
||||
Unadvise
|
||||
unattend
|
||||
UNCPRIORITY
|
||||
|
||||
@@ -49,7 +49,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
|
||||
|
||||
void InteractivityAutomationPeer::ParentProvider(AutomationPeer parentProvider)
|
||||
{
|
||||
_parentProvider = parentProvider;
|
||||
// LOAD-BEARING: use _parentProvider->ProviderFromPeer(_parentProvider) instead of this->ProviderFromPeer(*this).
|
||||
// Since we split the automation peer into TermControlAutomationPeer and InteractivityAutomationPeer,
|
||||
// using "this" returns null. This can cause issues with some UIA Client scenarios like any navigation in Narrator.
|
||||
_parentProvider = parentProvider ? parentProvider.as<IAutomationPeerProtected>().ProviderFromPeer(parentProvider) : nullptr;
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
@@ -181,15 +184,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
|
||||
|
||||
XamlAutomation::ITextRangeProvider InteractivityAutomationPeer::_CreateXamlUiaTextRange(UIA::ITextRangeProvider* returnVal) const
|
||||
{
|
||||
// LOAD-BEARING: use _parentProvider->ProviderFromPeer(_parentProvider) instead of this->ProviderFromPeer(*this).
|
||||
// Since we split the automation peer into TermControlAutomationPeer and InteractivityAutomationPeer,
|
||||
// using "this" returns null. This can cause issues with some UIA Client scenarios like any navigation in Narrator.
|
||||
const auto parent{ _parentProvider.get() };
|
||||
if (!parent)
|
||||
if (!_parentProvider)
|
||||
{
|
||||
return nullptr;
|
||||
}
|
||||
const auto xutr = winrt::make_self<XamlUiaTextRange>(returnVal, parent.as<IAutomationPeerProtected>().ProviderFromPeer(parent));
|
||||
const auto xutr = winrt::make_self<XamlUiaTextRange>(returnVal, _parentProvider);
|
||||
return xutr.as<XamlAutomation::ITextRangeProvider>();
|
||||
};
|
||||
|
||||
@@ -201,22 +200,24 @@ namespace winrt::Microsoft::Terminal::Control::implementation
|
||||
// - com_array of Xaml Wrapped UiaTextRange (ITextRangeProviders)
|
||||
com_array<XamlAutomation::ITextRangeProvider> InteractivityAutomationPeer::WrapArrayOfTextRangeProviders(SAFEARRAY* textRanges)
|
||||
{
|
||||
if (!_parentProvider)
|
||||
{
|
||||
return {};
|
||||
}
|
||||
|
||||
// transfer ownership of UiaTextRanges to this new vector
|
||||
auto providers = SafeArrayToOwningVector<::Microsoft::Terminal::TermControlUiaTextRange>(textRanges);
|
||||
auto count = gsl::narrow<int>(providers.size());
|
||||
const auto len = gsl::narrow<uint32_t>(providers.size());
|
||||
com_array<XamlAutomation::ITextRangeProvider> result{ len };
|
||||
|
||||
std::vector<XamlAutomation::ITextRangeProvider> vec;
|
||||
vec.reserve(count);
|
||||
for (auto i = 0; i < count; i++)
|
||||
for (uint32_t i = 0; i < len; ++i)
|
||||
{
|
||||
if (auto xutr = _CreateXamlUiaTextRange(providers[i].detach()))
|
||||
{
|
||||
vec.emplace_back(std::move(xutr));
|
||||
result[i] = std::move(xutr);
|
||||
}
|
||||
}
|
||||
|
||||
com_array<XamlAutomation::ITextRangeProvider> result{ vec };
|
||||
|
||||
return result;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -80,7 +80,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
|
||||
|
||||
::Microsoft::WRL::ComPtr<::Microsoft::Terminal::TermControlUiaProvider> _uiaProvider;
|
||||
winrt::Microsoft::Terminal::Control::implementation::ControlInteractivity* _interactivity;
|
||||
weak_ref<Windows::UI::Xaml::Automation::Peers::AutomationPeer> _parentProvider;
|
||||
winrt::Windows::UI::Xaml::Automation::Provider::IRawElementProviderSimple _parentProvider{ nullptr };
|
||||
|
||||
til::rect _controlBounds{};
|
||||
til::rect _controlPadding{};
|
||||
|
||||
@@ -3,7 +3,7 @@
|
||||
namespace Microsoft.Terminal.Control
|
||||
{
|
||||
[default_interface] runtimeclass InteractivityAutomationPeer :
|
||||
Windows.UI.Xaml.Automation.Peers.AutomationPeer,
|
||||
Windows.UI.Xaml.Automation.Peers.FrameworkElementAutomationPeer,
|
||||
Windows.UI.Xaml.Automation.Provider.ITextProvider
|
||||
{
|
||||
|
||||
|
||||
@@ -121,6 +121,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
|
||||
// GH#13978: If the TermControl has already been removed from the UI tree, XAML might run into weird bugs.
|
||||
// This will prevent the `dispatcher.RunAsync` calls below from raising UIA events on the main thread.
|
||||
_termControl = {};
|
||||
|
||||
// Solve the circular reference between us and the content automation peer.
|
||||
_contentAutomationPeer.ParentProvider(nullptr);
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
|
||||
@@ -324,9 +324,11 @@ std::vector<wil::com_ptr<T>> SafeArrayToOwningVector(SAFEARRAY* safeArray)
|
||||
std::vector<wil::com_ptr<T>> result{ gsl::narrow<std::size_t>(count) };
|
||||
for (int i = 0; i < count; i++)
|
||||
{
|
||||
result[i].attach(pVals[i]);
|
||||
result[i] = pVals[i];
|
||||
}
|
||||
|
||||
THROW_IF_FAILED(SafeArrayUnaccessData(safeArray));
|
||||
THROW_IF_FAILED(SafeArrayDestroy(safeArray));
|
||||
return result;
|
||||
}
|
||||
|
||||
|
||||
@@ -234,7 +234,7 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetSelection(_Outptr_result_maybenull_
|
||||
UiaTracing::TextProvider::GetSelection(*this, *range.Get());
|
||||
|
||||
LONG currentIndex = 0;
|
||||
hr = SafeArrayPutElement(*ppRetVal, ¤tIndex, range.Detach());
|
||||
hr = SafeArrayPutElement(*ppRetVal, ¤tIndex, range.Get());
|
||||
if (FAILED(hr))
|
||||
{
|
||||
SafeArrayDestroy(*ppRetVal);
|
||||
@@ -278,7 +278,7 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetVisibleRanges(_Outptr_result_mayben
|
||||
UiaTracing::TextProvider::GetVisibleRanges(*this, *range.Get());
|
||||
|
||||
LONG currentIndex = 0;
|
||||
hr = SafeArrayPutElement(*ppRetVal, ¤tIndex, range.Detach());
|
||||
hr = SafeArrayPutElement(*ppRetVal, ¤tIndex, range.Get());
|
||||
if (FAILED(hr))
|
||||
{
|
||||
SafeArrayDestroy(*ppRetVal);
|
||||
|
||||
Reference in New Issue
Block a user