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

@@ -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

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<std::unique_ptr<IInputEvent>>& inEventsToWrite)
+static constexpr short _ClampToShortMax(int value, short min)
+{
+    return static_cast<short>(std::clamp(value, static_cast<int>(min), SHRT_MAX));
+}
+
+static std::wstring _KeyEventsToText(std::deque<std::unique_ptr<IInputEvent>>& 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<TextBuffer>(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<short>(settings.InitialCols()), static_cast<short>(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<short>(settings.HistorySize()), renderTarget);
+    Create(viewportSize, _ClampToShortMax(settings.HistorySize(), 0), renderTarget);
 
     UpdateSettings(settings);
 }

src/cascadia/UnitTests_TerminalCore/ScreenSizeLimitsTest.cpp

@@ -0,0 +1,136 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+#include "precomp.h"
+#include <WexTestClass.h>
+
+#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<MockTermSettings, ICoreSettings>
+{
+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<MockTermSettings>(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<MockTermSettings>(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<MockTermSettings>(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<MockTermSettings>(-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<MockTermSettings>(SHRT_MAX - visibleRowCount, visibleRowCount, 100);
+        Terminal maxHistorySizeTerminal;
+        maxHistorySizeTerminal.CreateFromSettings(maxHistorySizeSettings, emptyRenderTarget);
+        VERIFY_ARE_EQUAL(maxHistorySizeTerminal.GetTextBuffer().TotalRowCount(), static_cast<unsigned int>(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<MockTermSettings>(SHRT_MAX - visibleRowCount + 1, visibleRowCount, 100);
+        Terminal justTooBigHistorySizeTerminal;
+        justTooBigHistorySizeTerminal.CreateFromSettings(justTooBigHistorySizeSettings, emptyRenderTarget);
+        VERIFY_ARE_EQUAL(justTooBigHistorySizeTerminal.GetTextBuffer().TotalRowCount(), static_cast<unsigned int>(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<MockTermSettings>(99999999, visibleRowCount, 100);
+        Terminal farTooBigHistorySizeTerminal;
+        farTooBigHistorySizeTerminal.CreateFromSettings(farTooBigHistorySizeSettings, emptyRenderTarget);
+        VERIFY_ARE_EQUAL(farTooBigHistorySizeTerminal.GetTextBuffer().TotalRowCount(), static_cast<unsigned int>(SHRT_MAX),
+            L"History size that is far too large is clamped to SHRT_MAX - initial row count");
+    }
+};
+}

src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj

@@ -1,7 +1,9 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project DefaultTargets="Build" ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
-  <Import Project="$(SolutionDir)src\common.build.pre.props" />
+  <Import Project="$(SolutionDir)\common.openconsole.props" Condition="'$(OpenConsoleDir)'==''" />
+  <Import Project="$(SolutionDir)\src\common.build.pre.props" />
   <ItemGroup>
+    <ClCompile Include="ScreenSizeLimitsTest.cpp" />
     <ClCompile Include="SelectionTest.cpp" />
     <ClCompile Include="precomp.cpp">
       <PrecompiledHeader>Create</PrecompiledHeader>
@@ -36,7 +38,7 @@
   </PropertyGroup>
   <ItemDefinitionGroup>
     <ClCompile>
-      <AdditionalIncludeDirectories>..;$(SolutionDir)src\inc;$(SolutionDir)src\inc\test;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <AdditionalIncludeDirectories>..;$(SolutionDir)src\inc;$(SolutionDir)src\inc\test;$(WinRT_IncludePath)\..\cppwinrt\winrt;"$(OpenConsoleDir)\src\cascadia\TerminalSettings\Generated Files";%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PrecompiledHeaderFile>precomp.h</PrecompiledHeaderFile>
     </ClCompile>
     <Link>

src/inc/LibraryIncludes.h

@@ -9,6 +9,7 @@
 #pragma warning(disable: ALL_CPPCORECHECK_WARNINGS)
 
 // C
+#include <climits>
 #include <cwchar>
 #include <cwctype>
 

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 ^