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.

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

Fix a number of shutdown crashes in TermControl #10115

Merged
merged 1 commit into from
May 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion src/cascadia/TerminalControl/TSFInputControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// Return Value:
// - <none>
void TSFInputControl::TryRedrawCanvas()
try
{
if (!_focused)
if (!_focused || !Canvas())
{
return;
}
Expand Down Expand Up @@ -164,6 +165,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

_RedrawCanvas();
}
CATCH_LOG()

// Method Description:
// - Redraw the Canvas and update the current Text Bounds and Control Bounds for
Expand Down
12 changes: 11 additions & 1 deletion src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

_uiaEngine = std::make_unique<::Microsoft::Console::Render::UiaEngine>(autoPeer.get());
_renderer->AddRenderEngine(_uiaEngine.get());
return *autoPeer;
_automationPeer = *autoPeer;
return _automationPeer;
}
return nullptr;
}
Expand Down Expand Up @@ -2627,6 +2628,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_connection.TerminalOutput(_connectionOutputEventToken);
_connectionStateChangedRevoker.revoke();

// These four throttled functions are triggered by terminal output and interact with the UI.
// Since Close() is the point after which we are removed from the UI, but before the destructor
// has run, we should disconnect them *right now*. If we don't, they may fire between the
// throttle delay (from the final output) and the dtor.
_tsfTryRedrawCanvas.reset();
_updatePatternLocations.reset();
_updateScrollBar.reset();
_playWarningBell.reset();

TSFInputControl().Close(); // Disconnect the TSF input control so it doesn't receive EditContext events.
_autoScrollTimer.Stop();

Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// we must ensure the _renderer is deallocated first.
// (C++ class members are destroyed in reverse order.)
std::unique_ptr<::Microsoft::Console::Render::DxEngine> _renderEngine;
// (further, the TermControlAutomationPeer must be destructed after _uiaEngine!)
winrt::Windows::UI::Xaml::Automation::Peers::AutomationPeer _automationPeer{ nullptr };
std::unique_ptr<::Microsoft::Console::Render::UiaEngine> _uiaEngine;
std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer;

Expand Down
52 changes: 38 additions & 14 deletions src/cascadia/TerminalControl/TermControlAutomationPeer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,17 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
void TermControlAutomationPeer::SignalSelectionChanged()
{
UiaTracing::Signal::SelectionChanged();
Dispatcher().RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [&]() {
// The event that is raised when the text selection is modified.
RaiseAutomationEvent(AutomationEvents::TextPatternOnTextSelectionChanged);
auto dispatcher{ Dispatcher() };
if (!dispatcher)
{
return;
}
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);
}
});
}

Expand All @@ -61,9 +69,17 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
void TermControlAutomationPeer::SignalTextChanged()
{
UiaTracing::Signal::TextChanged();
Dispatcher().RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [&]() {
// The event that is raised when textual content is modified.
RaiseAutomationEvent(AutomationEvents::TextPatternOnTextChanged);
auto dispatcher{ Dispatcher() };
if (!dispatcher)
{
return;
}
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);
}
});
}

Expand All @@ -76,14 +92,22 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
void TermControlAutomationPeer::SignalCursorChanged()
{
UiaTracing::Signal::CursorChanged();
Dispatcher().RunAsync(Windows::UI::Core::CoreDispatcherPriority::Normal, [&]() {
// 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.
RaiseAutomationEvent(AutomationEvents::TextPatternOnTextSelectionChanged);
auto dispatcher{ Dispatcher() };
if (!dispatcher)
{
return;
}
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);
}
});
}

Expand Down