From 6a26fd68c41c860a851f1044291a9c290d510b8f Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 12 May 2023 22:11:22 +0200 Subject: [PATCH] Fix race conditions in ControlCore (#15334) `ControlCore` contained two bugs: * Race condition on access of the 3 throttled funcs which may now be `reset()` during tear out * The `ScrollPositionChanged` event emitter was written incorrectly and would emit the event from the background thread without throttling during tear out --- src/cascadia/TerminalControl/ControlCore.cpp | 83 ++++++++++---------- src/cascadia/TerminalControl/ControlCore.h | 13 ++- src/cascadia/TerminalControl/ControlCore.idl | 12 +-- src/cascadia/UnitTests_Control/pch.h | 1 + 4 files changed, 57 insertions(+), 52 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 24e1caa5fc..4aabcfa141 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -82,9 +82,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation _actualFont{ DEFAULT_FONT_FACE, 0, DEFAULT_FONT_WEIGHT, { 0, DEFAULT_FONT_SIZE }, CP_UTF8, false } { _settings = winrt::make_self(settings, unfocusedAppearance); - _terminal = std::make_shared<::Microsoft::Terminal::Core::Terminal>(); + _setupDispatcherAndCallbacks(); + Connection(connection); _terminal->SetWriteInputCallback([this](std::wstring_view wstr) { @@ -137,17 +138,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation _renderer->SetBackgroundColorChangedCallback([this]() { _rendererBackgroundColorChanged(); }); _renderer->SetFrameColorChangedCallback([this]() { _rendererTabColorChanged(); }); - - _renderer->SetRendererEnteredErrorStateCallback([weakThis = get_weak()]() { - if (auto strongThis{ weakThis.get() }) - { - strongThis->_RendererEnteredErrorStateHandlers(*strongThis, nullptr); - } - }); + _renderer->SetRendererEnteredErrorStateCallback([this]() { _RendererEnteredErrorStateHandlers(nullptr, nullptr); }); THROW_IF_FAILED(localPointerToThread->Initialize(_renderer.get())); } - _setupDispatcherAndCallbacks(); UpdateSettings(settings, unfocusedAppearance); } @@ -179,7 +173,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation // need to hop across the process boundary every time text is output. // We can throttle this to once every 8ms, which will get us out of // the way of the main output & rendering threads. - _tsfTryRedrawCanvas = std::make_shared>( + const auto shared = _shared.lock(); + shared->tsfTryRedrawCanvas = std::make_shared>( _dispatcher, TsfRedrawInterval, [weakThis = get_weak()]() { @@ -191,7 +186,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // NOTE: Calling UpdatePatternLocations from a background // thread is a workaround for us to hit GH#12607 less often. - _updatePatternLocations = std::make_unique>( + shared->updatePatternLocations = std::make_unique>( UpdatePatternLocationsInterval, [weakTerminal = std::weak_ptr{ _terminal }]() { if (const auto t = weakTerminal.lock()) @@ -201,7 +196,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } }); - _updateScrollBar = std::make_shared>( + shared->updateScrollBar = std::make_shared>( _dispatcher, ScrollBarUpdateInterval, [weakThis = get_weak()](const auto& update) { @@ -231,9 +226,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Clear out any throttled funcs that we had wired up to run on this UI // thread. These will be recreated in _setupDispatcherAndCallbacks, when // we're re-attached to a new control (on a possibly new UI thread). - _tsfTryRedrawCanvas.reset(); - _updatePatternLocations.reset(); - _updateScrollBar.reset(); + const auto shared = _shared.lock(); + shared->tsfTryRedrawCanvas.reset(); + shared->updatePatternLocations.reset(); + shared->updateScrollBar.reset(); } void ControlCore::AttachToNewControl(const Microsoft::Terminal::Control::IKeyBindings& keyBindings) @@ -318,7 +314,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // scope for terminalLock auto terminalLock = _terminal->LockForWriting(); - if (_initializedTerminal) + if (_initializedTerminal.load(std::memory_order_relaxed)) { return false; } @@ -403,7 +399,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation THROW_IF_FAILED(_renderEngine->Enable()); - _initializedTerminal = true; + _initializedTerminal.store(true, std::memory_order_relaxed); } // scope for TerminalLock // Start the connection outside of lock, because it could @@ -423,7 +419,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - void ControlCore::EnablePainting() { - if (_initializedTerminal) + if (_initializedTerminal.load(std::memory_order_relaxed)) { _renderer->EnablePainting(); } @@ -656,9 +652,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation // itself - it was initiated by the mouse wheel, or the scrollbar. _terminal->UserScrollViewport(viewTop); - if (_updatePatternLocations) + const auto shared = _shared.lock_shared(); + if (shared->updatePatternLocations) { - (*_updatePatternLocations)(); + (*shared->updatePatternLocations)(); } } @@ -825,7 +822,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Update the terminal core with its new Core settings _terminal->UpdateSettings(*_settings); - if (!_initializedTerminal) + if (!_initializedTerminal.load(std::memory_order_relaxed)) { // If we haven't initialized, there's no point in continuing. // Initialization will handle the renderer settings. @@ -1434,10 +1431,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation const int viewHeight, const int bufferSize) { - if (!_initializedTerminal) + if (!_initializedTerminal.load(std::memory_order_relaxed)) { return; } + // Clear the regex pattern tree so the renderer does not try to render them while scrolling // We're **NOT** taking the lock here unlike _scrollbarChangeHandler because // we are already under lock (since this usually happens as a result of writing). @@ -1448,20 +1446,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto update{ winrt::make(viewTop, viewHeight, bufferSize) }; - if (!_inUnitTests && _updateScrollBar) - { - _updateScrollBar->Run(update); - } - else + + if (_inUnitTests) [[unlikely]] { _ScrollPositionChangedHandlers(*this, update); } - - // Additionally, start the throttled update of where our links are. - - if (_updatePatternLocations) + else { - (*_updatePatternLocations)(); + const auto shared = _shared.lock_shared(); + if (shared->updateScrollBar) + { + shared->updateScrollBar->Run(update); + } } } @@ -1469,9 +1465,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // When the buffer's cursor moves, start the throttled func to // eventually dispatch a CursorPositionChanged event. - if (_tsfTryRedrawCanvas) + const auto shared = _shared.lock_shared(); + if (shared->tsfTryRedrawCanvas) { - _tsfTryRedrawCanvas->Run(); + shared->tsfTryRedrawCanvas->Run(); } } @@ -1482,11 +1479,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation void ControlCore::_terminalShowWindowChanged(bool showOrHide) { - if (_initializedTerminal) - { - auto showWindow = winrt::make_self(showOrHide); - _ShowWindowChangedHandlers(*this, *showWindow); - } + auto showWindow = winrt::make_self(showOrHide); + _ShowWindowChangedHandlers(*this, *showWindow); } // Method Description: @@ -1617,6 +1611,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto weakThis{ get_weak() }; + // Concurrent read of _dispatcher is safe, because Detach() calls WaitForPaintCompletionAndDisable() + // which blocks until this call returns. _dispatcher will only be changed afterwards. co_await wil::resume_foreground(_dispatcher); if (auto core{ weakThis.get() }) @@ -1686,7 +1682,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation Core::Point ControlCore::CursorPosition() const { // If we haven't been initialized yet, then fake it. - if (!_initializedTerminal) + if (!_initializedTerminal.load(std::memory_order_relaxed)) { return { 0, 0 }; } @@ -1805,9 +1801,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation _terminal->Write(hstr); // Start the throttled update of where our hyperlinks are. - if (_updatePatternLocations) + const auto shared = _shared.lock_shared(); + if (shared->updatePatternLocations) { - (*_updatePatternLocations)(); + (*shared->updatePatternLocations)(); } } catch (...) @@ -2016,7 +2013,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - void ControlCore::WindowVisibilityChanged(const bool showOrHide) { - if (_initializedTerminal) + if (_initializedTerminal.load(std::memory_order_relaxed)) { // show is true, hide is false if (auto conpty{ _connection.try_as() }) diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index d90ef24e93..f444b81e79 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -266,7 +266,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation // clang-format on private: - bool _initializedTerminal{ false }; + struct SharedState + { + std::shared_ptr> tsfTryRedrawCanvas; + std::unique_ptr> updatePatternLocations; + std::shared_ptr> updateScrollBar; + }; + + std::atomic _initializedTerminal{ false }; bool _closing{ false }; TerminalConnection::ITerminalConnection _connection{ nullptr }; @@ -313,9 +320,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation uint64_t _owningHwnd{ 0 }; winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr }; - std::shared_ptr> _tsfTryRedrawCanvas; - std::unique_ptr> _updatePatternLocations; - std::shared_ptr> _updateScrollBar; + til::shared_mutex _shared; til::point _contextMenuBufferPosition{ 0, 0 }; diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl index 3b6fe07160..fdd7fa53b2 100644 --- a/src/cascadia/TerminalControl/ControlCore.idl +++ b/src/cascadia/TerminalControl/ControlCore.idl @@ -149,26 +149,28 @@ namespace Microsoft.Terminal.Control Boolean ShouldShowSelectCommand(); Boolean ShouldShowSelectOutput(); - event FontSizeChangedEventArgs FontSizeChanged; - + // These events are called from some background thread event Windows.Foundation.TypedEventHandler CopyToClipboard; event Windows.Foundation.TypedEventHandler TitleChanged; event Windows.Foundation.TypedEventHandler WarningBell; event Windows.Foundation.TypedEventHandler TabColorChanged; event Windows.Foundation.TypedEventHandler BackgroundColorChanged; + event Windows.Foundation.TypedEventHandler TaskbarProgressChanged; + event Windows.Foundation.TypedEventHandler RendererEnteredErrorState; + event Windows.Foundation.TypedEventHandler ShowWindowChanged; + + // These events are always called from the UI thread (bugs aside) + event FontSizeChangedEventArgs FontSizeChanged; event Windows.Foundation.TypedEventHandler ScrollPositionChanged; event Windows.Foundation.TypedEventHandler CursorPositionChanged; - event Windows.Foundation.TypedEventHandler TaskbarProgressChanged; event Windows.Foundation.TypedEventHandler ConnectionStateChanged; event Windows.Foundation.TypedEventHandler HoveredHyperlinkChanged; - event Windows.Foundation.TypedEventHandler RendererEnteredErrorState; event Windows.Foundation.TypedEventHandler SwapChainChanged; event Windows.Foundation.TypedEventHandler RendererWarning; event Windows.Foundation.TypedEventHandler RaiseNotice; event Windows.Foundation.TypedEventHandler TransparencyChanged; event Windows.Foundation.TypedEventHandler ReceivedOutput; event Windows.Foundation.TypedEventHandler FoundMatch; - event Windows.Foundation.TypedEventHandler ShowWindowChanged; event Windows.Foundation.TypedEventHandler UpdateSelectionMarkers; event Windows.Foundation.TypedEventHandler OpenHyperlink; event Windows.Foundation.TypedEventHandler CloseTerminalRequested; diff --git a/src/cascadia/UnitTests_Control/pch.h b/src/cascadia/UnitTests_Control/pch.h index 6a8d0be3ee..4833c11df0 100644 --- a/src/cascadia/UnitTests_Control/pch.h +++ b/src/cascadia/UnitTests_Control/pch.h @@ -46,6 +46,7 @@ Licensed under the MIT license. // Manually include til after we include Windows.Foundation to give it winrt superpowers #include "til.h" +#include #include #include "ThrottledFunc.h"