From 7533b31cbd2f4fcf8f233ba62213377744f0ede3 Mon Sep 17 00:00:00 2001 From: Michael Ratanapintha <1039862+metathinker@users.noreply.github.com> Date: Fri, 17 May 2019 20:57:34 -0700 Subject: [PATCH] Fix #453: Setting historySize=32767 causes WindowsTerminal to hang (#843) * fix for historySize=32767 hang (except for historySize=0 case); tests still in progress * tests run and almost pass - failure is a real bug in my change * fixed bug that caused tests to fail, but it seems another bug causes the app to crash with a zero row count * fix the additional bug (at a higher layer) mentioned in previous commit description * Fix chk build assertion failures in new tests It seems C++/WinRT doesn't like it when you implement a Windows Runtime interface but then create instances of the implementing class with function-call lifetime (aka stack allocation). That makes sense given that WinRT objects are COM objects, but in my defense I was following this example where they are just fine instantiating the `App` object on the stack: https://docs.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/author-apis#if-youre-not-authoring-a-runtime-class * tabs to spaces * CR feedback * fix minor CR feedback (incorrect test log message) --- src/cascadia/TerminalControl/TermControl.cpp | 7 +- src/cascadia/TerminalCore/Terminal.cpp | 17 ++- .../ScreenSizeLimitsTest.cpp | 136 ++++++++++++++++++ .../UnitTests_TerminalCore/UnitTests.vcxproj | 6 +- src/inc/LibraryIncludes.h | 1 + tools/runut.cmd | 1 + 6 files changed, 158 insertions(+), 10 deletions(-) create mode 100644 src/cascadia/UnitTests_TerminalCore/ScreenSizeLimitsTest.cpp diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 628c81555e..e363205f7a 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1077,8 +1077,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation FontInfo actualFont = { fontFace, 0, 10, { 0, fontHeight }, CP_UTF8, false }; FontInfoDesired desiredFont = { actualFont }; - const auto cols = settings.InitialCols(); - const auto rows = settings.InitialRows(); + // If the settings have negative or zero row or column counts, ignore those counts. + // (The lower TerminalCore layer also has upper bounds as well, but at this layer + // we may eventually impose different ones depending on how many pixels we can address.) + const auto cols = std::max(settings.InitialCols(), 1); + const auto rows = std::max(settings.InitialRows(), 1); // Create a DX engine and initialize it with our font and DPI. We'll // then use it to measure how much space the requested rows and columns diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index bb66445d51..96972e9e51 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -18,7 +18,12 @@ using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; using namespace Microsoft::Console::VirtualTerminal; -std::wstring _KeyEventsToText(std::deque>& inEventsToWrite) +static constexpr short _ClampToShortMax(int value, short min) +{ + return static_cast(std::clamp(value, static_cast(min), SHRT_MAX)); +} + +static std::wstring _KeyEventsToText(std::deque>& inEventsToWrite) { std::wstring wstr = L""; for(auto& ev : inEventsToWrite) @@ -65,9 +70,9 @@ void Terminal::Create(COORD viewportSize, SHORT scrollbackLines, IRenderTarget& { _mutableViewport = Viewport::FromDimensions({ 0,0 }, viewportSize); _scrollbackLines = scrollbackLines; - COORD bufferSize { viewportSize.X, viewportSize.Y + scrollbackLines }; - TextAttribute attr{}; - UINT cursorSize = 12; + const COORD bufferSize { viewportSize.X, _ClampToShortMax(viewportSize.Y + scrollbackLines, 1) }; + const TextAttribute attr{}; + const UINT cursorSize = 12; _buffer = std::make_unique(bufferSize, attr, cursorSize, renderTarget); } @@ -79,9 +84,9 @@ void Terminal::Create(COORD viewportSize, SHORT scrollbackLines, IRenderTarget& void Terminal::CreateFromSettings(winrt::Microsoft::Terminal::Settings::ICoreSettings settings, Microsoft::Console::Render::IRenderTarget& renderTarget) { - const COORD viewportSize{ static_cast(settings.InitialCols()), static_cast(settings.InitialRows()) }; + const COORD viewportSize{ _ClampToShortMax(settings.InitialCols(), 1), _ClampToShortMax(settings.InitialRows(), 1) }; // TODO:MSFT:20642297 - Support infinite scrollback here, if HistorySize is -1 - Create(viewportSize, static_cast(settings.HistorySize()), renderTarget); + Create(viewportSize, _ClampToShortMax(settings.HistorySize(), 0), renderTarget); UpdateSettings(settings); } diff --git a/src/cascadia/UnitTests_TerminalCore/ScreenSizeLimitsTest.cpp b/src/cascadia/UnitTests_TerminalCore/ScreenSizeLimitsTest.cpp new file mode 100644 index 0000000000..514105fad3 --- /dev/null +++ b/src/cascadia/UnitTests_TerminalCore/ScreenSizeLimitsTest.cpp @@ -0,0 +1,136 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" +#include + +#include "DefaultSettings.h" +#include "../cascadia/TerminalCore/Terminal.hpp" +#include "../renderer/inc/DummyRenderTarget.hpp" +#include "consoletaeftemplates.hpp" + +#include "winrt/Microsoft.Terminal.Settings.h" + +using namespace winrt::Microsoft::Terminal::Settings; +using namespace Microsoft::Terminal::Core; + +namespace TerminalCoreUnitTests +{ +class MockTermSettings : public winrt::implements +{ +public: + MockTermSettings(int32_t historySize, int32_t initialRows, int32_t initialCols) : + _historySize(historySize), + _initialRows(initialRows), + _initialCols(initialCols) + { } + + // property getters - all implemented + int32_t HistorySize() { return _historySize; } + int32_t InitialRows() { return _initialRows; } + int32_t InitialCols() { return _initialCols; } + uint32_t DefaultForeground() { return COLOR_WHITE; } + uint32_t DefaultBackground() { return COLOR_BLACK; } + bool SnapOnInput() { return false; } + uint32_t CursorColor() { return COLOR_WHITE; } + CursorStyle CursorShape() const noexcept { return CursorStyle::Vintage; } + uint32_t CursorHeight() { return 42UL; } + + // other implemented methods + uint32_t GetColorTableEntry(int32_t) const { return 123; } + + // property setters - all unimplemented + void HistorySize(int32_t) { } + void InitialRows(int32_t) { } + void InitialCols(int32_t) { } + void DefaultForeground(uint32_t) { } + void DefaultBackground(uint32_t) { } + void SnapOnInput(bool) { } + void CursorColor(uint32_t) { } + void CursorShape(CursorStyle const&) noexcept { } + void CursorHeight(uint32_t) { } + + // other unimplemented methods + void SetColorTableEntry(int32_t /* index */, uint32_t /* value */) { } + +private: + int32_t _historySize; + int32_t _initialRows; + int32_t _initialCols; +}; + +#define WCS(x) WCSHELPER(x) +#define WCSHELPER(x) L#x + +class ScreenSizeLimitsTest +{ + TEST_CLASS(ScreenSizeLimitsTest); + + TEST_METHOD(ScreenWidthAndHeightAreClampedToBounds) + { + DummyRenderTarget emptyRenderTarget; + + // Negative values for initial visible row count or column count + // are clamped to 1. Too-large positive values are clamped to SHRT_MAX. + auto negativeColumnsSettings = winrt::make(10000, 9999999, -1234); + Terminal negativeColumnsTerminal; + negativeColumnsTerminal.CreateFromSettings(negativeColumnsSettings, emptyRenderTarget); + COORD actualDimensions = negativeColumnsTerminal.GetViewport().Dimensions(); + VERIFY_ARE_EQUAL(actualDimensions.Y, SHRT_MAX, L"Row count clamped to SHRT_MAX == " WCS(SHRT_MAX)); + VERIFY_ARE_EQUAL(actualDimensions.X, 1, L"Column count clamped to 1"); + + // Zero values are clamped to 1 as well. + auto zeroRowsSettings = winrt::make(10000, 0, 9999999); + Terminal zeroRowsTerminal; + zeroRowsTerminal.CreateFromSettings(zeroRowsSettings, emptyRenderTarget); + actualDimensions = zeroRowsTerminal.GetViewport().Dimensions(); + VERIFY_ARE_EQUAL(actualDimensions.Y, 1, L"Row count clamped to 1"); + VERIFY_ARE_EQUAL(actualDimensions.X, SHRT_MAX, L"Column count clamped to SHRT_MAX == " WCS(SHRT_MAX)); + } + + TEST_METHOD(ScrollbackHistorySizeIsClampedToBounds) + { + // What is actually clamped is the number of rows in the internal history buffer, + // which is the *sum* of the history size plus the number of rows + // actually visible on screen at the moment. + + const unsigned int visibleRowCount = 100; + DummyRenderTarget emptyRenderTarget; + + // Zero history size is acceptable. + auto noHistorySettings = winrt::make(0, visibleRowCount, 100); + Terminal noHistoryTerminal; + noHistoryTerminal.CreateFromSettings(noHistorySettings, emptyRenderTarget); + VERIFY_ARE_EQUAL(noHistoryTerminal.GetTextBuffer().TotalRowCount(), visibleRowCount, + L"History size of 0 is accepted"); + + // Negative history sizes are clamped to zero. + auto negativeHistorySizeSettings = winrt::make(-100, visibleRowCount, 100); + Terminal negativeHistorySizeTerminal; + negativeHistorySizeTerminal.CreateFromSettings(negativeHistorySizeSettings, emptyRenderTarget); + VERIFY_ARE_EQUAL(negativeHistorySizeTerminal.GetTextBuffer().TotalRowCount(), visibleRowCount, + L"Negative history size is clamped to 0"); + + // History size + initial visible rows == SHRT_MAX is acceptable. + auto maxHistorySizeSettings = winrt::make(SHRT_MAX - visibleRowCount, visibleRowCount, 100); + Terminal maxHistorySizeTerminal; + maxHistorySizeTerminal.CreateFromSettings(maxHistorySizeSettings, emptyRenderTarget); + VERIFY_ARE_EQUAL(maxHistorySizeTerminal.GetTextBuffer().TotalRowCount(), static_cast(SHRT_MAX), + L"History size == SHRT_MAX - initial row count is accepted"); + + // History size + initial visible rows == SHRT_MAX + 1 will be clamped slightly. + auto justTooBigHistorySizeSettings = winrt::make(SHRT_MAX - visibleRowCount + 1, visibleRowCount, 100); + Terminal justTooBigHistorySizeTerminal; + justTooBigHistorySizeTerminal.CreateFromSettings(justTooBigHistorySizeSettings, emptyRenderTarget); + VERIFY_ARE_EQUAL(justTooBigHistorySizeTerminal.GetTextBuffer().TotalRowCount(), static_cast(SHRT_MAX), + L"History size == 1 + SHRT_MAX - initial row count is clamped to SHRT_MAX - initial row count"); + + // Ridiculously large history sizes are also clamped. + auto farTooBigHistorySizeSettings = winrt::make(99999999, visibleRowCount, 100); + Terminal farTooBigHistorySizeTerminal; + farTooBigHistorySizeTerminal.CreateFromSettings(farTooBigHistorySizeSettings, emptyRenderTarget); + VERIFY_ARE_EQUAL(farTooBigHistorySizeTerminal.GetTextBuffer().TotalRowCount(), static_cast(SHRT_MAX), + L"History size that is far too large is clamped to SHRT_MAX - initial row count"); + } +}; +} diff --git a/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj b/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj index d01c7223cc..be07732931 100644 --- a/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj +++ b/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj @@ -1,7 +1,9 @@  - + + + Create @@ -36,7 +38,7 @@ - ..;$(SolutionDir)src\inc;$(SolutionDir)src\inc\test;%(AdditionalIncludeDirectories) + ..;$(SolutionDir)src\inc;$(SolutionDir)src\inc\test;$(WinRT_IncludePath)\..\cppwinrt\winrt;"$(OpenConsoleDir)\src\cascadia\TerminalSettings\Generated Files";%(AdditionalIncludeDirectories) precomp.h diff --git a/src/inc/LibraryIncludes.h b/src/inc/LibraryIncludes.h index e852ef621c..7470a4fa20 100644 --- a/src/inc/LibraryIncludes.h +++ b/src/inc/LibraryIncludes.h @@ -9,6 +9,7 @@ #pragma warning(disable: ALL_CPPCORECHECK_WARNINGS) // C +#include #include #include diff --git a/tools/runut.cmd b/tools/runut.cmd index 4aaf994979..d144b0eaf8 100644 --- a/tools/runut.cmd +++ b/tools/runut.cmd @@ -5,6 +5,7 @@ rem Run the console unit tests. call %TAEF% ^ %OPENCON%\bin\%PLATFORM%\%_LAST_BUILD_CONF%\Conhost.Unit.Tests.dll ^ %OPENCON%\bin\%PLATFORM%\%_LAST_BUILD_CONF%\TextBuffer.Unittests.dll ^ + %OPENCON%\bin\%PLATFORM%\%_LAST_BUILD_CONF%\Terminal.Core.Unit.Tests.dll ^ %OPENCON%\bin\%PLATFORM%\%_LAST_BUILD_CONF%\Conhost.Interactivity.Win32.Unit.Tests.dll ^ %OPENCON%\bin\%PLATFORM%\%_LAST_BUILD_CONF%\ConParser.Unit.Tests.dll ^ %OPENCON%\bin\%PLATFORM%\%_LAST_BUILD_CONF%\ConAdapter.Unit.Tests.dll ^