Conversation
This comment has been minimized.
This comment has been minimized.
carlos-zamora
left a comment
There was a problem hiding this comment.
@lhecker Could you do me a favor and double check that this actually fixes the leak?
We have notes from our initial investigation here: #19710 (comment)
I swear we found that there was still an issue with XAML, but I set a breakpoint on the dtor for InteractivityAutomationPeer, TermControlAutomationPeer, XamlUiaTextRange, and UiaTextRangeBase and they all seem to be getting hit properly now. I even checked them without the change and I've confirmed that the UiaTextRangeBase objects weren't getting dtor'd until the term control was closed. Am I going crazy? 🙃
| { | ||
| [default_interface] runtimeclass InteractivityAutomationPeer : | ||
| Windows.UI.Xaml.Automation.Peers.AutomationPeer, | ||
| Windows.UI.Xaml.Automation.Peers.FrameworkElementAutomationPeer, |
There was a problem hiding this comment.
|
Ah fuck, I forgot that I promised to send a PR for this. |
lhecker
left a comment
There was a problem hiding this comment.
Yep, that all looks correct to me!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
|
|
||
| void SetControlBounds(Windows.Foundation.Rect bounds); | ||
| void SetControlPadding(Microsoft.Terminal.Core.Padding padding); | ||
| void ParentProvider(Windows.UI.Xaml.Automation.Peers.AutomationPeer parentProvider); |
There was a problem hiding this comment.
Should we also use FrameworkElementAutomationPeer here (I'm not quite sure about this, I just made a short overview of the PR and found this other place including AutomationPeer and just thought that it's probably the best and quickest to ask)? And thanks for the nice PR!
| void ParentProvider(Windows.UI.Xaml.Automation.Peers.FrameworkElementAutomationPeer parentProvider); |
There was a problem hiding this comment.
Good question! I think we're fine as is. It's currently implemented like this:
terminal/src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp
Lines 50 to 56 in 72f6f83
With ProviderFromPeer() returning an IRawElementProviderSimple directly (docs link).
In practice, we're only ever passing in a FrameworkElementAutomationPeer, but really we're just using it to call the ProviderFromPeer() function on it. I'd rather keep it more flexible than less, unless there's a separate reason (which I'm open to).


Summary of the Pull Request
This includes the memory leak fixes that @lhecker and I investigated as a part of #19710.
The
ITextRangeProviders (namelyUiaTextRanges) 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