From 661fde59375b52e2c67b7c9483cc2358508484c2 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Mon, 17 May 2021 14:22:22 -0500 Subject: [PATCH] Fix a number of shutdown crashes in TermControl (#10115) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. The TSFInputControl may get a layout event after it has been removed from service (and no longer has a XAML tree) * Two fixes: * first, guard the layour updater from accessing detached xaml objects * second, shut down all pending throttled functions during close (not destruction!¹) 2. The TermControlAutomationPeer may be destructed before its events fire. 3. The TermControlAutomationPeer may receive a notification after it has been detached from XAML (and therefore has no dispatcher). ¹ Close happens before the control is removed from the XAML tree; destruction happens some time later. We must detach all UI-bound events in Close so that they don't fire between when we detach and when we destruct. Fixes MSFT-32496693 Fixes MSFT-32496158 Fixes MSFT-32509759 Fixes MSFT-32871913 --- .../TerminalControl/TSFInputControl.cpp | 4 +- src/cascadia/TerminalControl/TermControl.cpp | 12 ++++- src/cascadia/TerminalControl/TermControl.h | 2 + .../TermControlAutomationPeer.cpp | 52 ++++++++++++++----- 4 files changed, 54 insertions(+), 16 deletions(-) diff --git a/src/cascadia/TerminalControl/TSFInputControl.cpp b/src/cascadia/TerminalControl/TSFInputControl.cpp index 23bd9bcbec2..54c32a27c7b 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.cpp +++ b/src/cascadia/TerminalControl/TSFInputControl.cpp @@ -134,8 +134,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Return Value: // - void TSFInputControl::TryRedrawCanvas() + try { - if (!_focused) + if (!_focused || !Canvas()) { return; } @@ -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 diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index bfcb0479abd..a3a5ecf02a0 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -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; } @@ -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(); diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index fb23b17008e..bdcae13ab75 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -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; diff --git a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp index 80ab74ada9b..4cc814ba410 100644 --- a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp +++ b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp @@ -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); + } }); } @@ -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); + } }); } @@ -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); + } }); }