Skip to content

Commit

Permalink
An attempted fix for the SignalTextChanged crash (#13876)
Browse files Browse the repository at this point in the history
This is conjecture - I was totally unable to repro the original crash here.
Based on the stacks in MSFT:39994969, it looks like we try to fire off a
`RaiseAutomationEvent`, which calls through UIA core, eventually to the point of
calling `ComPtr<WUX::Automation::Peers::IAutomationPeer>::{dtor}`. I'm guessing
based on the stacks that the TermControl has already been released and cleaned
up. However, the lambda in the `RunAsync` calls here only takes a ref to the
TCAP. The TCAP has an outstanding reference (maybe on the other side of the UIA
fence), and gets successfully resolved as strong, but when calling to
`RaiseAutomationEvent`, the `owner` we passed in is gonezo.

This explicitly passes a `weak_ref` to `TermControlAutomationPeer`, rather than
a raw ptr, so we can actually check if the control is still alive before _we_
dereference it. If it is, great, we've got a strong ref to it now and it won't
get torn down.

Again, this is hearsay. Without a repro, the only way we can confirm this is
gone is by just hoping the crashes go away. 🤞

* Might close #13357 (we'll reopen if it doesn't?)
* narrator still works

(cherry picked from commit fe0d570)
Service-Card-Id: 85401131
Service-Version: 1.15
  • Loading branch information
zadjii-msft authored and DHowett committed Sep 6, 2022
1 parent 2f6b546 commit 6027a36
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 27 deletions.
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
margins.Top,
margins.Right,
margins.Bottom };
_automationPeer = winrt::make<implementation::TermControlAutomationPeer>(this, padding, interactivityAutoPeer);
_automationPeer = winrt::make<implementation::TermControlAutomationPeer>(get_strong(), padding, interactivityAutoPeer);
return _automationPeer;
}
}
Expand Down
72 changes: 48 additions & 24 deletions src/cascadia/TerminalControl/TermControlAutomationPeer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ static constexpr bool IsReadable(std::wstring_view text)

namespace winrt::Microsoft::Terminal::Control::implementation
{
TermControlAutomationPeer::TermControlAutomationPeer(TermControl* owner,
TermControlAutomationPeer::TermControlAutomationPeer(winrt::com_ptr<TermControl> owner,
const Core::Padding padding,
Control::InteractivityAutomationPeer impl) :
TermControlAutomationPeerT<TermControlAutomationPeer>(*owner), // pass owner to FrameworkElementAutomationPeer
TermControlAutomationPeerT<TermControlAutomationPeer>(*owner.get()), // pass owner to FrameworkElementAutomationPeer
_termControl{ owner },
_contentAutomationPeer{ impl }
{
Expand Down Expand Up @@ -134,8 +134,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
dispatcher.RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [weakThis{ get_weak() }]() {
if (auto strongThis{ weakThis.get() })
{
// The event that is raised when the text selection is modified.
strongThis->RaiseAutomationEvent(AutomationEvents::TextPatternOnTextSelectionChanged);
if (auto control{ strongThis->_termControl.get() })
{
// The event that is raised when the text selection is modified.
strongThis->RaiseAutomationEvent(AutomationEvents::TextPatternOnTextSelectionChanged);
}
}
});
}
Expand All @@ -157,8 +160,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
dispatcher.RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [weakThis{ get_weak() }]() {
if (auto strongThis{ weakThis.get() })
{
// The event that is raised when textual content is modified.
strongThis->RaiseAutomationEvent(AutomationEvents::TextPatternOnTextChanged);
if (auto control{ strongThis->_termControl.get() })
{
// The event that is raised when textual content is modified.
strongThis->RaiseAutomationEvent(AutomationEvents::TextPatternOnTextChanged);
}
}
});
}
Expand All @@ -180,13 +186,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
dispatcher.RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [weakThis{ get_weak() }]() {
if (auto strongThis{ weakThis.get() })
{
// The event that is raised when the text was changed in an edit control.
// Do NOT fire a TextEditTextChanged. Generally, an app on the other side
// will expect more information. Though you can dispatch that event
// on its own, it may result in a nullptr exception on the other side
// because no additional information was provided. Crashing the screen
// reader.
strongThis->RaiseAutomationEvent(AutomationEvents::TextPatternOnTextSelectionChanged);
if (auto control{ strongThis->_termControl.get() })
{
// The event that is raised when the text was changed in an edit control.
// Do NOT fire a TextEditTextChanged. Generally, an app on the other side
// will expect more information. Though you can dispatch that event
// on its own, it may result in a nullptr exception on the other side
// because no additional information was provided. Crashing the screen
// reader.
strongThis->RaiseAutomationEvent(AutomationEvents::TextPatternOnTextSelectionChanged);
}
}
});
}
Expand Down Expand Up @@ -236,14 +245,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
dispatcher.RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [weakThis{ get_weak() }, sanitizedCopy{ hstring{ sanitized } }]() {
if (auto strongThis{ weakThis.get() })
{
try
if (auto control{ strongThis->_termControl.get() })
{
strongThis->RaiseNotificationEvent(AutomationNotificationKind::ActionCompleted,
AutomationNotificationProcessing::All,
sanitizedCopy,
L"TerminalTextOutput");
try
{
strongThis->RaiseNotificationEvent(AutomationNotificationKind::ActionCompleted,
AutomationNotificationProcessing::All,
sanitizedCopy,
L"TerminalTextOutput");
}
CATCH_LOG();
}
CATCH_LOG();
}
});
}
Expand Down Expand Up @@ -284,17 +296,29 @@ namespace winrt::Microsoft::Terminal::Control::implementation
hstring TermControlAutomationPeer::GetNameCore() const
{
// fallback to title if profile name is empty
auto profileName = _termControl->GetProfileName();
if (profileName.empty())
if (auto control{ _termControl.get() })
{
return _termControl->Title();
const auto profileName = control->GetProfileName();
if (profileName.empty())
{
return control->Title();
}
else
{
return profileName;
}
}
return profileName;

return L"";
}

hstring TermControlAutomationPeer::GetHelpTextCore() const
{
return _termControl->Title();
if (const auto control{ _termControl.get() })
{
return control->Title();
}
return L"";
}

AutomationLiveSetting TermControlAutomationPeer::GetLiveSettingCore() const
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalControl/TermControlAutomationPeer.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
::Microsoft::Console::Types::IUiaEventDispatcher
{
public:
TermControlAutomationPeer(Microsoft::Terminal::Control::implementation::TermControl* owner,
TermControlAutomationPeer(winrt::com_ptr<Microsoft::Terminal::Control::implementation::TermControl> owner,
const Core::Padding padding,
Control::InteractivityAutomationPeer implementation);

Expand Down Expand Up @@ -78,7 +78,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
#pragma endregion

private:
winrt::Microsoft::Terminal::Control::implementation::TermControl* _termControl;
winrt::weak_ref<Microsoft::Terminal::Control::implementation::TermControl> _termControl;
Control::InteractivityAutomationPeer _contentAutomationPeer;
std::deque<wchar_t> _keyEvents;
};
Expand Down

0 comments on commit 6027a36

Please sign in to comment.