From 521a300c174a835289c819d65f39547e9fc36de0 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 23 Jan 2024 18:00:27 +0100 Subject: [PATCH] Avoid timer ticks on frozen windows (#16587) At the time of writing, closing the last tab of a window inexplicably doesn't lead to the destruction of the remaining TermControl instance. On top of that, on Win10 we don't destroy window threads due to bugs in DesktopWindowXamlSource. In other words, we leak TermControl instances. Additionally, the XAML timer class is "self-referential". Releasing all references to an instance will not stop the timer. Only calling Stop() explicitly will achieve that. The result is that the message loop of a frozen window thread has so far received 1-2 messages per second due to the blink timer not being stopped. This may have filled the message queue and lead to bugs as described in #16332 where keyboard input stopped working. --- src/cascadia/LocalTests_TerminalApp/pch.h | 2 + src/cascadia/TerminalApp/ColorHelper.cpp | 2 - src/cascadia/TerminalApp/ColorHelper.h | 3 +- .../TerminalApp/TerminalAppLib.vcxproj | 4 +- src/cascadia/TerminalApp/TerminalTab.cpp | 18 ++-- src/cascadia/TerminalApp/TerminalTab.h | 2 +- src/cascadia/TerminalApp/Toast.h | 2 +- src/cascadia/TerminalApp/pch.h | 2 + src/cascadia/TerminalControl/TermControl.cpp | 75 +++++++-------- src/cascadia/TerminalControl/TermControl.h | 8 +- src/cascadia/TerminalControl/pch.h | 3 +- src/cascadia/WinRTUtils/WinRTUtils.vcxproj | 3 +- .../WinRTUtils/WinRTUtils.vcxproj.filters | 8 +- .../WinRTUtils/inc/SafeDispatcherTimer.h | 92 +++++++++++++++++++ src/cascadia/WindowsTerminal/AppHost.cpp | 13 +-- src/cascadia/WindowsTerminal/AppHost.h | 5 +- src/cascadia/WindowsTerminal/pch.h | 6 +- 17 files changed, 163 insertions(+), 85 deletions(-) create mode 100644 src/cascadia/WinRTUtils/inc/SafeDispatcherTimer.h diff --git a/src/cascadia/LocalTests_TerminalApp/pch.h b/src/cascadia/LocalTests_TerminalApp/pch.h index f82561888b1..25df5f47e9e 100644 --- a/src/cascadia/LocalTests_TerminalApp/pch.h +++ b/src/cascadia/LocalTests_TerminalApp/pch.h @@ -68,6 +68,8 @@ Author(s): // Manually include til after we include Windows.Foundation to give it winrt superpowers #include "til.h" +#include + // Common includes for most tests: #include "../../inc/conattrs.hpp" #include "../../types/inc/utils.hpp" diff --git a/src/cascadia/TerminalApp/ColorHelper.cpp b/src/cascadia/TerminalApp/ColorHelper.cpp index 7ed3f8a260c..c2dd4c2303c 100644 --- a/src/cascadia/TerminalApp/ColorHelper.cpp +++ b/src/cascadia/TerminalApp/ColorHelper.cpp @@ -1,6 +1,4 @@ -#include "pch.h" #include "ColorHelper.h" -#include using namespace winrt::TerminalApp; diff --git a/src/cascadia/TerminalApp/ColorHelper.h b/src/cascadia/TerminalApp/ColorHelper.h index cbe6582c8d1..e4ab3f42b77 100644 --- a/src/cascadia/TerminalApp/ColorHelper.h +++ b/src/cascadia/TerminalApp/ColorHelper.h @@ -1,7 +1,6 @@ #pragma once -#include "pch.h" -#include +#include namespace winrt::TerminalApp { diff --git a/src/cascadia/TerminalApp/TerminalAppLib.vcxproj b/src/cascadia/TerminalApp/TerminalAppLib.vcxproj index 2b933ddfe91..6e1cd5aaaea 100644 --- a/src/cascadia/TerminalApp/TerminalAppLib.vcxproj +++ b/src/cascadia/TerminalApp/TerminalAppLib.vcxproj @@ -237,7 +237,9 @@ - + + NotUsing + Create diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index 06121ab945d..08c51cb730c 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -121,12 +121,7 @@ namespace winrt::TerminalApp::implementation void TerminalTab::_BellIndicatorTimerTick(const Windows::Foundation::IInspectable& /*sender*/, const Windows::Foundation::IInspectable& /*e*/) { ShowBellIndicator(false); - // Just do a sanity check that the timer still exists before we stop it - if (_bellIndicatorTimer.has_value()) - { - _bellIndicatorTimer->Stop(); - _bellIndicatorTimer = std::nullopt; - } + _bellIndicatorTimer.Stop(); } // Method Description: @@ -356,14 +351,13 @@ namespace winrt::TerminalApp::implementation { ASSERT_UI_THREAD(); - if (!_bellIndicatorTimer.has_value()) + if (!_bellIndicatorTimer) { - DispatcherTimer bellIndicatorTimer; - bellIndicatorTimer.Interval(std::chrono::milliseconds(2000)); - bellIndicatorTimer.Tick({ get_weak(), &TerminalTab::_BellIndicatorTimerTick }); - bellIndicatorTimer.Start(); - _bellIndicatorTimer.emplace(std::move(bellIndicatorTimer)); + _bellIndicatorTimer.Interval(std::chrono::milliseconds(2000)); + _bellIndicatorTimer.Tick({ get_weak(), &TerminalTab::_BellIndicatorTimerTick }); } + + _bellIndicatorTimer.Start(); } // Method Description: diff --git a/src/cascadia/TerminalApp/TerminalTab.h b/src/cascadia/TerminalApp/TerminalTab.h index fe6ee7d92a3..d3b03f426fd 100644 --- a/src/cascadia/TerminalApp/TerminalTab.h +++ b/src/cascadia/TerminalApp/TerminalTab.h @@ -152,7 +152,7 @@ namespace winrt::TerminalApp::implementation void _Setup(); - std::optional _bellIndicatorTimer; + SafeDispatcherTimer _bellIndicatorTimer; void _BellIndicatorTimerTick(const Windows::Foundation::IInspectable& sender, const Windows::Foundation::IInspectable& e); void _MakeTabViewItem() override; diff --git a/src/cascadia/TerminalApp/Toast.h b/src/cascadia/TerminalApp/Toast.h index 40d3df7cf55..63b2b5a0770 100644 --- a/src/cascadia/TerminalApp/Toast.h +++ b/src/cascadia/TerminalApp/Toast.h @@ -34,5 +34,5 @@ class Toast : public std::enable_shared_from_this private: winrt::Microsoft::UI::Xaml::Controls::TeachingTip _tip; - winrt::Windows::UI::Xaml::DispatcherTimer _timer; + SafeDispatcherTimer _timer; }; diff --git a/src/cascadia/TerminalApp/pch.h b/src/cascadia/TerminalApp/pch.h index 01811ad62f6..8d0338108e3 100644 --- a/src/cascadia/TerminalApp/pch.h +++ b/src/cascadia/TerminalApp/pch.h @@ -83,6 +83,8 @@ TRACELOGGING_DECLARE_PROVIDER(g_hTerminalAppProvider); // Manually include til after we include Windows.Foundation to give it winrt superpowers #include "til.h" +#include + #include #include // must go after the CoreDispatcher type is defined diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 4c59d027a52..2437be05452 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -57,10 +57,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _isInternalScrollBarUpdate{ false }, _autoScrollVelocity{ 0 }, _autoScrollingPointerPoint{ std::nullopt }, - _autoScrollTimer{}, _lastAutoScrollUpdateTime{ std::nullopt }, - _cursorTimer{}, - _blinkTimer{}, _searchBox{ nullptr } { InitializeComponent(); @@ -1087,10 +1084,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (blinkTime != INFINITE) { // Create a timer - DispatcherTimer cursorTimer; - cursorTimer.Interval(std::chrono::milliseconds(blinkTime)); - cursorTimer.Tick({ get_weak(), &TermControl::_CursorTimerTick }); - _cursorTimer.emplace(std::move(cursorTimer)); + _cursorTimer.Interval(std::chrono::milliseconds(blinkTime)); + _cursorTimer.Tick({ get_weak(), &TermControl::_CursorTimerTick }); // As of GH#6586, don't start the cursor timer immediately, and // don't show the cursor initially. We'll show the cursor and start // the timer when the control is first focused. @@ -1105,13 +1100,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation _core.CursorOn(_focused || _displayCursorWhileBlurred()); if (_displayCursorWhileBlurred()) { - _cursorTimer->Start(); + _cursorTimer.Start(); } } else { - // The user has disabled cursor blinking - _cursorTimer = std::nullopt; + _cursorTimer.Destroy(); } // Set up blinking attributes @@ -1120,16 +1114,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (animationsEnabled && blinkTime != INFINITE) { // Create a timer - DispatcherTimer blinkTimer; - blinkTimer.Interval(std::chrono::milliseconds(blinkTime)); - blinkTimer.Tick({ get_weak(), &TermControl::_BlinkTimerTick }); - blinkTimer.Start(); - _blinkTimer.emplace(std::move(blinkTimer)); + _blinkTimer.Interval(std::chrono::milliseconds(blinkTime)); + _blinkTimer.Tick({ get_weak(), &TermControl::_BlinkTimerTick }); + _blinkTimer.Start(); } else { // The user has disabled blinking - _blinkTimer = std::nullopt; + _blinkTimer.Destroy(); } // Now that the renderer is set up, update the appearance for initialization @@ -1498,7 +1490,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Manually show the cursor when a key is pressed. Restarting // the timer prevents flickering. _core.CursorOn(_core.SelectionMode() != SelectionInteractionMode::Mark); - _cursorTimer->Start(); + _cursorTimer.Start(); } return handled; @@ -1973,12 +1965,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // When the terminal focuses, show the cursor immediately _core.CursorOn(_core.SelectionMode() != SelectionInteractionMode::Mark); - _cursorTimer->Start(); + _cursorTimer.Start(); } if (_blinkTimer) { - _blinkTimer->Start(); + _blinkTimer.Start(); } // Only update the appearance here if an unfocused config exists - if an @@ -2021,13 +2013,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (_cursorTimer && !_displayCursorWhileBlurred()) { - _cursorTimer->Stop(); + _cursorTimer.Stop(); _core.CursorOn(false); } if (_blinkTimer) { - _blinkTimer->Stop(); + _blinkTimer.Stop(); } // Check if there is an unfocused config we should set the appearance to @@ -2278,7 +2270,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Disconnect the TSF input control so it doesn't receive EditContext events. TSFInputControl().Close(); + + // At the time of writing, closing the last tab of a window inexplicably + // does not lead to the destruction of the remaining TermControl instance(s). + // On Win10 we don't destroy window threads due to bugs in DesktopWindowXamlSource. + // In turn, we leak TermControl instances. This results in constant HWND messages + // while the thread is supposed to be idle. Stop these timers avoids this. _autoScrollTimer.Stop(); + _bellLightTimer.Stop(); + _cursorTimer.Stop(); + _blinkTimer.Stop(); if (!_detached) { @@ -3129,20 +3130,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation _bellDarkAnimation.Duration(winrt::Windows::Foundation::TimeSpan(std::chrono::milliseconds(TerminalWarningBellInterval))); } - // Similar to the animation, only initialize the timer here - if (!_bellLightTimer) - { - _bellLightTimer = {}; - _bellLightTimer.Interval(std::chrono::milliseconds(TerminalWarningBellInterval)); - _bellLightTimer.Tick({ get_weak(), &TermControl::_BellLightOff }); - } - Windows::Foundation::Numerics::float2 zeroSize{ 0, 0 }; // If the grid has 0 size or if the bell timer is // already active, do nothing if (RootGrid().ActualSize() != zeroSize && !_bellLightTimer.IsEnabled()) { - // Start the timer, when the timer ticks we switch off the light + _bellLightTimer.Interval(std::chrono::milliseconds(TerminalWarningBellInterval)); + _bellLightTimer.Tick({ get_weak(), &TermControl::_BellLightOff }); _bellLightTimer.Start(); // Switch on the light and animate the intensity to fade out @@ -3162,15 +3156,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation void TermControl::_BellLightOff(const Windows::Foundation::IInspectable& /* sender */, const Windows::Foundation::IInspectable& /* e */) { - if (_bellLightTimer) - { - // Stop the timer and switch off the light - _bellLightTimer.Stop(); + // Stop the timer and switch off the light + _bellLightTimer.Stop(); - if (!_IsClosing()) - { - VisualBellLight::SetIsTarget(RootGrid(), false); - } + if (!_IsClosing()) + { + VisualBellLight::SetIsTarget(RootGrid(), false); } } @@ -3729,9 +3720,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // If we should be ALWAYS displaying the cursor, turn it on and start blinking. _core.CursorOn(true); - if (_cursorTimer.has_value()) + if (_cursorTimer) { - _cursorTimer->Start(); + _cursorTimer.Start(); } } else @@ -3740,9 +3731,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation // blinking. (if we're focused, then we're already doing the right // thing) const auto focused = FocusState() != FocusState::Unfocused; - if (!focused && _cursorTimer.has_value()) + if (!focused && _cursorTimer) { - _cursorTimer->Stop(); + _cursorTimer.Stop(); } _core.CursorOn(focused); } diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 441ca5f5763..20449f98129 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -236,16 +236,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation // viewport. View is then scrolled to 'follow' the cursor. double _autoScrollVelocity; std::optional _autoScrollingPointerPoint; - Windows::UI::Xaml::DispatcherTimer _autoScrollTimer; + SafeDispatcherTimer _autoScrollTimer; std::optional _lastAutoScrollUpdateTime; bool _pointerPressedInBounds{ false }; winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation{ nullptr }; winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellDarkAnimation{ nullptr }; - Windows::UI::Xaml::DispatcherTimer _bellLightTimer{ nullptr }; + SafeDispatcherTimer _bellLightTimer; - std::optional _cursorTimer; - std::optional _blinkTimer; + SafeDispatcherTimer _cursorTimer; + SafeDispatcherTimer _blinkTimer; winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker; bool _showMarksInScrollbar{ false }; diff --git a/src/cascadia/TerminalControl/pch.h b/src/cascadia/TerminalControl/pch.h index 98f905ef088..f7be9ae1eac 100644 --- a/src/cascadia/TerminalControl/pch.h +++ b/src/cascadia/TerminalControl/pch.h @@ -73,7 +73,8 @@ TRACELOGGING_DECLARE_PROVIDER(g_hTerminalControlProvider); #include #include -#include "ThrottledFunc.h" +#include +#include #include #include // must go after the CoreDispatcher type is defined diff --git a/src/cascadia/WinRTUtils/WinRTUtils.vcxproj b/src/cascadia/WinRTUtils/WinRTUtils.vcxproj index 959daaddf8f..13052c590be 100644 --- a/src/cascadia/WinRTUtils/WinRTUtils.vcxproj +++ b/src/cascadia/WinRTUtils/WinRTUtils.vcxproj @@ -19,6 +19,7 @@ + @@ -52,4 +53,4 @@ - + \ No newline at end of file diff --git a/src/cascadia/WinRTUtils/WinRTUtils.vcxproj.filters b/src/cascadia/WinRTUtils/WinRTUtils.vcxproj.filters index 5e47f13f57a..a1d17001d20 100644 --- a/src/cascadia/WinRTUtils/WinRTUtils.vcxproj.filters +++ b/src/cascadia/WinRTUtils/WinRTUtils.vcxproj.filters @@ -2,21 +2,21 @@ + + - + + - - - diff --git a/src/cascadia/WinRTUtils/inc/SafeDispatcherTimer.h b/src/cascadia/WinRTUtils/inc/SafeDispatcherTimer.h new file mode 100644 index 00000000000..1a61bb254b6 --- /dev/null +++ b/src/cascadia/WinRTUtils/inc/SafeDispatcherTimer.h @@ -0,0 +1,92 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#pragma once + +// Par for the course, the XAML timer class is "self-referential". Releasing all references +// to an instance will not stop the timer. Only calling Stop() explicitly will achieve that. +struct SafeDispatcherTimer +{ + SafeDispatcherTimer() = default; + SafeDispatcherTimer(SafeDispatcherTimer const&) = delete; + SafeDispatcherTimer& operator=(SafeDispatcherTimer const&) = delete; + SafeDispatcherTimer(SafeDispatcherTimer&&) = delete; + SafeDispatcherTimer& operator=(SafeDispatcherTimer&&) = delete; + + ~SafeDispatcherTimer() + { + Destroy(); + } + + explicit operator bool() const noexcept + { + return _timer != nullptr; + } + + winrt::Windows::Foundation::TimeSpan Interval() + { + return _getTimer().Interval(); + } + + void Interval(winrt::Windows::Foundation::TimeSpan const& value) + { + _getTimer().Interval(value); + } + + bool IsEnabled() + { + return _timer && _timer.IsEnabled(); + } + + void Tick(winrt::Windows::Foundation::EventHandler const& handler) + { + auto& timer = _getTimer(); + if (_token) + { + timer.Tick(_token); + } + _token = timer.Tick(handler); + } + + void Start() + { + _getTimer().Start(); + } + + void Stop() const + { + if (_timer) + { + _timer.Stop(); + } + } + + void Destroy() + { + if (!_timer) + { + return; + } + + _timer.Stop(); + if (_token) + { + _timer.Tick(_token); + } + _timer = nullptr; + _token = {}; + } + +private: + ::winrt::Windows::UI::Xaml::DispatcherTimer& _getTimer() + { + if (!_timer) + { + _timer = ::winrt::Windows::UI::Xaml::DispatcherTimer{}; + } + return _timer; + } + + ::winrt::Windows::UI::Xaml::DispatcherTimer _timer{ nullptr }; + winrt::event_token _token; +}; diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 7cae9959998..3168011a491 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -438,10 +438,7 @@ void AppHost::Close() // After calling _window->Close() we should avoid creating more WinUI related actions. // I suspect WinUI wouldn't like that very much. As such unregister all event handlers first. _revokers = {}; - if (_frameTimer) - { - _frameTimer.Tick(_frameTimerToken); - } + _frameTimer.Destroy(); _showHideWindowThrottler.reset(); _revokeWindowCallbacks(); @@ -1190,12 +1187,8 @@ void AppHost::_startFrameTimer() // _updateFrameColor, which will actually handle setting the colors. If we // already have a timer, just start that one. - if (_frameTimer == nullptr) - { - _frameTimer = winrt::Windows::UI::Xaml::DispatcherTimer(); - _frameTimer.Interval(FrameUpdateInterval); - _frameTimerToken = _frameTimer.Tick({ this, &AppHost::_updateFrameColor }); - } + _frameTimer.Tick({ this, &AppHost::_updateFrameColor }); + _frameTimer.Interval(FrameUpdateInterval); _frameTimer.Start(); } diff --git a/src/cascadia/WindowsTerminal/AppHost.h b/src/cascadia/WindowsTerminal/AppHost.h index fd912aee67d..992a4402874 100644 --- a/src/cascadia/WindowsTerminal/AppHost.h +++ b/src/cascadia/WindowsTerminal/AppHost.h @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. +#pragma once + #include "pch.h" #include "NonClientIslandWindow.h" #include "NotificationIcon.h" @@ -56,7 +58,7 @@ class AppHost : public std::enable_shared_from_this std::shared_ptr> _showHideWindowThrottler; std::chrono::time_point _started; - winrt::Windows::UI::Xaml::DispatcherTimer _frameTimer{ nullptr }; + SafeDispatcherTimer _frameTimer; uint32_t _launchShowWindowCommand{ SW_NORMAL }; @@ -165,7 +167,6 @@ class AppHost : public std::enable_shared_from_this void _updateFrameColor(const winrt::Windows::Foundation::IInspectable&, const winrt::Windows::Foundation::IInspectable&); winrt::event_token _GetWindowLayoutRequestedToken; - winrt::event_token _frameTimerToken; // Helper struct. By putting these all into one struct, we can revoke them // all at once, by assigning _revokers to a fresh Revokers instance. That'll diff --git a/src/cascadia/WindowsTerminal/pch.h b/src/cascadia/WindowsTerminal/pch.h index eafe88dfb6c..5011dbeba61 100644 --- a/src/cascadia/WindowsTerminal/pch.h +++ b/src/cascadia/WindowsTerminal/pch.h @@ -48,7 +48,7 @@ Module Name: #include // Needed just for XamlIslands to work at all: -#include +#include #include #include #include @@ -63,7 +63,7 @@ Module Name: #include #include #include -#include +#include #include #include #include @@ -91,5 +91,7 @@ TRACELOGGING_DECLARE_PROVIDER(g_hWindowsTerminalProvider); #include "til.h" #include "til/mutex.h" +#include + #include #include // must go after the CoreDispatcher type is defined