From 6fc0978ddb41a7276aa965dba70df49a13d2ab56 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Tue, 11 Jun 2019 12:37:20 -0700 Subject: [PATCH] Properly manage teardown state in TermControl (#1199) * Properly manage teardown state in TermControl This commit introduces a few automatic event revokers and implements staged Close for TermControl's constituent components. * Only read the focused control title if there is one Fixes #1198 Fixes #1188 --- src/cascadia/TerminalApp/App.cpp | 5 +- src/cascadia/TerminalControl/TermControl.cpp | 69 +++++++++++--------- src/cascadia/TerminalControl/TermControl.h | 10 ++- 3 files changed, 51 insertions(+), 33 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index 556565f5f..c19574698 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -1036,7 +1036,10 @@ namespace winrt::TerminalApp::implementation { try { - return _GetFocusedControl().Title(); + if (auto focusedControl{ _GetFocusedControl() }) + { + return focusedControl.Title(); + } } CATCH_LOG(); } diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index efb29141a..1575434f0 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -80,12 +80,12 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Create the SwapChainPanel that will display our content Controls::SwapChainPanel swapChainPanel; - swapChainPanel.SizeChanged({ this, &TermControl::_SwapChainSizeChanged }); - swapChainPanel.CompositionScaleChanged({ this, &TermControl::_SwapChainScaleChanged }); + _sizeChangedRevoker = swapChainPanel.SizeChanged(winrt::auto_revoke, { this, &TermControl::_SwapChainSizeChanged }); + _compositionScaleChangedRevoker = swapChainPanel.CompositionScaleChanged(winrt::auto_revoke, { this, &TermControl::_SwapChainScaleChanged }); // Initialize the terminal only once the swapchainpanel is loaded - that // way, we'll be able to query the real pixel size it got on layout - swapChainPanel.Loaded([this] (auto /*s*/, auto /*e*/){ + _loadedRevoker = swapChainPanel.Loaded(winrt::auto_revoke, [this] (auto /*s*/, auto /*e*/){ _InitializeTerminal(); }); @@ -355,30 +355,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation TermControl::~TermControl() { - _closing = true; - // Don't let anyone else do something to the buffer. - auto lock = _terminal->LockForWriting(); - - // Clear out the cursor timer, so it doesn't trigger again on us once we're destructed. - if (_cursorTimer) - { - _cursorTimer.value().Stop(); - _cursorTimer = std::nullopt; - } - - if (_connection) - { - _connection.Close(); - } - - if (_renderer) - { - _renderer->TriggerTeardown(); - } - - _swapChainPanel = nullptr; - _root = nullptr; - _connection = nullptr; + Close(); } UIElement TermControl::GetRoot() @@ -569,8 +546,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _cursorTimer = std::nullopt; } - _controlRoot.GotFocus({ this, &TermControl::_GotFocusHandler }); - _controlRoot.LostFocus({ this, &TermControl::_LostFocusHandler }); + _gotFocusRevoker = _controlRoot.GotFocus(winrt::auto_revoke, { this, &TermControl::_GotFocusHandler }); + _lostFocusRevoker = _controlRoot.LostFocus(winrt::auto_revoke, { this, &TermControl::_LostFocusHandler }); // Focus the control here. If we do it up above (in _Create_), then the // focus won't actually get passed to us. I believe this is because @@ -1194,9 +1171,39 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void TermControl::Close() { - if (!_closing) + if (!_closing.exchange(true)) { - this->~TermControl(); + // Stop accepting new output before we disconnect everything. + _connection.TerminalOutput(_connectionOutputEventToken); + + // Clear out the cursor timer, so it doesn't trigger again on us once we're destructed. + if (auto localCursorTimer{std::exchange(_cursorTimer, std::nullopt)}) + { + localCursorTimer->Stop(); + // cursorTimer timer, now stopped, is destroyed. + } + + if (auto localConnection{std::exchange(_connection, nullptr)}) + { + localConnection.Close(); + // connection is destroyed. + } + + if (auto localRenderEngine{std::exchange(_renderEngine, nullptr)}) + { + if (auto localRenderer{std::exchange(_renderer, nullptr)}) + { + localRenderer->TriggerTeardown(); + // renderer is destroyed + } + // renderEngine is destroyed + } + + if (auto localTerminal{std::exchange(_terminal, nullptr)}) + { + _initializedTerminal = false; + // terminal is destroyed. + } } } diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 5abe60e20..6b62b38fc 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -78,7 +78,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation Settings::IControlSettings _settings; bool _focused; - bool _closing; + std::atomic _closing; FontInfoDesired _desiredFont; FontInfo _actualFont; @@ -94,6 +94,14 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // viewport via touch input. std::optional _touchAnchor; + // Event revokers -- we need to deregister ourselves before we die, + // lest we get callbacks afterwards. + winrt::Windows::UI::Xaml::Controls::Control::SizeChanged_revoker _sizeChangedRevoker; + winrt::Windows::UI::Xaml::Controls::SwapChainPanel::CompositionScaleChanged_revoker _compositionScaleChangedRevoker; + winrt::Windows::UI::Xaml::Controls::SwapChainPanel::Loaded_revoker _loadedRevoker; + winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker; + winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker; + void _Create(); void _ApplyUISettings(); void _InitializeBackgroundBrush();