From 17bd8b57504a9761b65f2bfb384466ec61981693 Mon Sep 17 00:00:00 2001 From: Leon Liang <57155886+leonMSFT@users.noreply.github.com> Date: Wed, 29 Apr 2020 17:05:29 -0700 Subject: [PATCH] Grab the write lock before updating the font (#5654) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Based on the discussion in #5479, it seems that the crash is caused by a race condition due to not obtaining the write lock before calling `TriggerFontChange`. I'm not totally sure if my approach is the right one, but I've taken the lock out of `_RefreshSize` since it seems like all calls of `_RefreshSize` come after a `TriggerFontChange`/`UpdateFont`. Then I just made sure all calls of `TriggerFontChange`/`UpdateFont` are preceded with a `LockForWriting`. ## PR Checklist * [x] Closes #5479 * [x] CLA signed. * [x] Tests added/passed ## Validation Steps Performed Scrolling to change my font size does not kill the Terminal anymore! 🙌 --- src/cascadia/TerminalControl/TermControl.cpp | 28 ++++++++++++-------- src/cascadia/TerminalControl/TermControl.h | 4 +-- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index c75621f25d..94878332c5 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -224,13 +224,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Update the terminal core with its new Core settings _terminal->UpdateSettings(_settings); + auto lock = _terminal->LockForWriting(); + // Refresh our font with the renderer const auto actualFontOldSize = _actualFont.GetSize(); _UpdateFont(); const auto actualFontNewSize = _actualFont.GetSize(); if (actualFontNewSize != actualFontOldSize) { - _RefreshSize(); + _RefreshSizeUnderLock(); } } } @@ -1654,7 +1656,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // font changes or the DPI changes, as DPI changes will necessitate a // font change. This method will *not* change the buffer/viewport size // to account for the new glyph dimensions. Callers should make sure to - // appropriately call _DoResize after this method is called. + // appropriately call _DoResizeUnderLock after this method is called. + // - The write lock should be held when calling this method. // Arguments: // - initialUpdate: whether this font update should be considered as being // concerned with initialization process. Value forwarded to event handler. @@ -1684,6 +1687,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _actualFont = { fontFace, 0, 10, { 0, newSize }, CP_UTF8, false }; _desiredFont = { _actualFont }; + auto lock = _terminal->LockForWriting(); + // Refresh our font with the renderer _UpdateFont(); @@ -1691,7 +1696,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // NOT change the size of the window, because that can lead to more // problems (like what happens when you change the font size while the // window is maximized?) - _RefreshSize(); + _RefreshSizeUnderLock(); } CATCH_LOG(); } @@ -1730,7 +1735,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation foundationSize.Width *= currentEngineScale; foundationSize.Height *= currentEngineScale; - _DoResize(foundationSize.Width, foundationSize.Height); + _DoResizeUnderLock(foundationSize.Width, foundationSize.Height); } // Method Description: @@ -1782,12 +1787,14 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation const auto actualFontOldSize = _actualFont.GetSize(); + auto lock = _terminal->LockForWriting(); + _renderer->TriggerFontChange(::base::saturated_cast(dpi), _desiredFont, _actualFont); const auto actualFontNewSize = _actualFont.GetSize(); if (actualFontNewSize != actualFontOldSize) { - _RefreshSize(); + _RefreshSizeUnderLock(); } } } @@ -1830,14 +1837,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Method Description: // - Perform a resize for the current size of the swapchainpanel. If the // font size changed, we'll need to resize the buffer to fit the existing - // swapchain size. This helper will call _DoResize with the current size + // swapchain size. This helper will call _DoResizeUnderLock with the current size // of the swapchain, accounting for scaling due to DPI. // - Note that a DPI change will also trigger a font size change, and will call into here. + // - The write lock should be held when calling this method, we might be changing the buffer size in _DoResizeUnderLock. // Arguments: // - // Return Value: // - - void TermControl::_RefreshSize() + void TermControl::_RefreshSizeUnderLock() { const auto currentScaleX = SwapChainPanel().CompositionScaleX(); const auto currentScaleY = SwapChainPanel().CompositionScaleY(); @@ -1847,9 +1855,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation const auto widthInPixels = actualWidth * currentScaleX; const auto heightInPixels = actualHeight * currentScaleY; - // Grab the lock, because we might be changing the buffer size here. - auto lock = _terminal->LockForWriting(); - _DoResize(widthInPixels, heightInPixels); + _DoResizeUnderLock(widthInPixels, heightInPixels); } // Method Description: @@ -1860,7 +1866,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Arguments: // - newWidth: the new width of the swapchain, in pixels. // - newHeight: the new height of the swapchain, in pixels. - void TermControl::_DoResize(const double newWidth, const double newHeight) + void TermControl::_DoResizeUnderLock(const double newWidth, const double newHeight) { SIZE size; size.cx = static_cast(newWidth); diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 81a8fc85ce..c42f2246ab 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -197,8 +197,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _SendPastedTextToConnection(const std::wstring& wstr); void _SwapChainSizeChanged(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::SizeChangedEventArgs const& e); void _SwapChainScaleChanged(Windows::UI::Xaml::Controls::SwapChainPanel const& sender, Windows::Foundation::IInspectable const& args); - void _DoResize(const double newWidth, const double newHeight); - void _RefreshSize(); + void _DoResizeUnderLock(const double newWidth, const double newHeight); + void _RefreshSizeUnderLock(); void _TerminalTitleChanged(const std::wstring_view& wstr); winrt::fire_and_forget _TerminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize); winrt::fire_and_forget _TerminalCursorPositionChanged();