From 9445a75171821befeb0f66f338548965b65b73bf Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 10 Mar 2026 18:08:32 -0700 Subject: [PATCH] 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 (cherry picked from commit e2110e716cda41aba429472ac704f9324f72e6b4) Service-Card-Id: PVTI_lADOAF3p4s4BBcTlzgnCP0M Service-Version: 1.24 --- .github/actions/spelling/expect/expect.txt | 1 + .../InteractivityAutomationPeer.cpp | 29 ++++++++++--------- .../InteractivityAutomationPeer.h | 2 +- .../InteractivityAutomationPeer.idl | 2 +- .../TermControlAutomationPeer.cpp | 3 ++ src/cascadia/inc/cppwinrt_utils.h | 4 ++- src/types/ScreenInfoUiaProviderBase.cpp | 4 +-- 7 files changed, 26 insertions(+), 19 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index e7941f8801..354fa941e1 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -1770,6 +1770,7 @@ uld uldash uldb ulwave +Unaccess Unadvise unattend UNCPRIORITY diff --git a/src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp b/src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp index c20845cea8..e36e769b9e 100644 --- a/src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp +++ b/src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp @@ -50,7 +50,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().ProviderFromPeer(parentProvider) : nullptr; } // Method Description: @@ -182,15 +185,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(returnVal, parent.as().ProviderFromPeer(parent)); + const auto xutr = winrt::make_self(returnVal, _parentProvider); return xutr.as(); }; @@ -202,22 +201,24 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - com_array of Xaml Wrapped UiaTextRange (ITextRangeProviders) com_array 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(providers.size()); + const auto len = gsl::narrow(providers.size()); + com_array result{ len }; - std::vector 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 result{ vec }; - return result; } } diff --git a/src/cascadia/TerminalControl/InteractivityAutomationPeer.h b/src/cascadia/TerminalControl/InteractivityAutomationPeer.h index 4f5b4a79c6..d69d19d16e 100644 --- a/src/cascadia/TerminalControl/InteractivityAutomationPeer.h +++ b/src/cascadia/TerminalControl/InteractivityAutomationPeer.h @@ -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 _parentProvider; + winrt::Windows::UI::Xaml::Automation::Provider::IRawElementProviderSimple _parentProvider{ nullptr }; til::rect _controlBounds{}; til::rect _controlPadding{}; diff --git a/src/cascadia/TerminalControl/InteractivityAutomationPeer.idl b/src/cascadia/TerminalControl/InteractivityAutomationPeer.idl index 9eb79a6329..e99a79c5ad 100644 --- a/src/cascadia/TerminalControl/InteractivityAutomationPeer.idl +++ b/src/cascadia/TerminalControl/InteractivityAutomationPeer.idl @@ -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 { diff --git a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp index b978efd842..ef394accf6 100644 --- a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp +++ b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp @@ -122,6 +122,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: diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index 06828a9fe2..afc1ccca2a 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -291,9 +291,11 @@ std::vector> SafeArrayToOwningVector(SAFEARRAY* safeArray) std::vector> result{ gsl::narrow(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; } diff --git a/src/types/ScreenInfoUiaProviderBase.cpp b/src/types/ScreenInfoUiaProviderBase.cpp index 27dfb8c3b7..4a6f5b6212 100644 --- a/src/types/ScreenInfoUiaProviderBase.cpp +++ b/src/types/ScreenInfoUiaProviderBase.cpp @@ -257,7 +257,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); @@ -301,7 +301,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);