From 14fe652f3e94aa14cb08894f116751abda21fb63 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Apr 2020 08:14:18 -0500 Subject: [PATCH 1/3] This is the test for #1856 --- .../ConptyRoundtripTests.cpp | 70 +++++++++++++++++++ src/inc/test/CommonState.hpp | 4 +- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 748644ef2db..0b7e84da299 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -196,6 +196,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(ResizeRepaintVimExeBuffer); + TEST_METHOD(TestResizeWithCookedRead); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -2552,3 +2554,71 @@ void ConptyRoundtripTests::ResizeRepaintVimExeBuffer() Log::Comment(L"========== Checking the terminal buffer state (after) =========="); verifyBuffer(*termTb, term->_mutableViewport.ToInclusive()); } + +void ConptyRoundtripTests::TestResizeWithCookedRead() +{ + // see https://github.com/microsoft/terminal/issues/1856 + Log::Comment(L"This test checks a crash in conpty where resizing the " + L"window with any data in a cooked read (like thhe input line " + L"in cmd.exe) would cause the conpty to crash."); + + // Resizing with a COKKED_READ used to cause a crash in + // `Selection::s_GetInputLineBoundaries` north of + // `Selection::GetValidAreaBoundaries`. + // + // If this test completes successfully, then we know that we didn't crash. + + // The specific cases that repro the original crash are: + // * (0, -10) + // * (0, -1) + // * (0, 0) + // the rest of the cases are added here for completeness. + + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:dx", L"{-10, -1, 0, 1, -10}") + TEST_METHOD_PROPERTY(L"Data:dy", L"{-10, -1, 0, 1, 10}") + END_TEST_METHOD_PROPERTIES() + + INIT_TEST_PROPERTY(int, dx, L"The change in width of the buffer"); + INIT_TEST_PROPERTY(int, dy, L"The change in height of the buffer"); + + VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto* hostTb = &si.GetTextBuffer(); + auto* termTb = term->_buffer.get(); + + _flushFirstFrame(); + + _checkConptyOutput = false; + _logConpty = true; + + // Setup the cooked read data + m_state->PrepareReadHandle(); + m_state->PrepareCookedReadData({ "This is some cooked read data\0" }); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Resize the Terminal and conpty here =========="); + const COORD newViewportSize{ + ::base::saturated_cast(TerminalViewWidth + dx), + ::base::saturated_cast(TerminalViewHeight + dy) + }; + + auto resizeResult = term->UserResize(newViewportSize); + VERIFY_SUCCEEDED(resizeResult); + _resizeConpty(newViewportSize.X, newViewportSize.Y); + + // After we resize, make sure to get the new textBuffers + hostTb = &si.GetTextBuffer(); + termTb = term->_buffer.get(); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + // By simply reaching the +} diff --git a/src/inc/test/CommonState.hpp b/src/inc/test/CommonState.hpp index 976bc0b62f5..4c5fde73628 100644 --- a/src/inc/test/CommonState.hpp +++ b/src/inc/test/CommonState.hpp @@ -124,7 +124,7 @@ class CommonState delete gci.pInputBuffer; } - void PrepareCookedReadData() + void PrepareCookedReadData(const std::string_view initialData = {}) { CONSOLE_INFORMATION& gci = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation(); auto* readData = new COOKED_READ_DATA(gci.pInputBuffer, @@ -135,7 +135,7 @@ class CommonState 0, nullptr, L"", - {}); + initialData); gci.SetCookedReadData(readData); } From 0d6dba53c556bb107a32abbb5b1df461a13990d8 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Apr 2020 08:14:30 -0500 Subject: [PATCH 2/3] This is the fix for #1856 --- src/host/getset.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index c8d81f92670..0c96032d4aa 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -589,7 +589,15 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont if (NewSize.X != context.GetViewport().Width() || NewSize.Y != context.GetViewport().Height()) { + // GH#1856 - make sure to hide the commandline _before_ we execute + // the resize, and the re-display it after the resize. If we leave + // it displayed, we'll crash during the resize when we try to figure + // out if the bounds of the old commandline fit within the new + // window (it might not). + CommandLine& commandLine = CommandLine::Instance(); + commandLine.Hide(FALSE); context.SetViewportSize(&NewSize); + commandLine.Show(); IConsoleWindow* const pWindow = ServiceLocator::LocateConsoleWindow(); if (pWindow != nullptr) From f294defc3fc97400636659d7689623a543830112 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Apr 2020 09:33:06 -0500 Subject: [PATCH 3/3] add a resizing helper that will force you to do the right thing --- .../ConptyRoundtripTests.cpp | 74 ++++++++----------- 1 file changed, 32 insertions(+), 42 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 0b7e84da299..d4cd1ca33c4 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -202,6 +202,9 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); void _resizeConpty(const unsigned short sx, const unsigned short sy); + + [[nodiscard]] std::tuple _performResize(const til::size& newSize); + std::deque expectedOutput; std::unique_ptr _pVtRenderEngine; std::unique_ptr m_state; @@ -273,6 +276,19 @@ void ConptyRoundtripTests::_resizeConpty(const unsigned short sx, } } +[[nodiscard]] std::tuple ConptyRoundtripTests::_performResize(const til::size& newSize) +{ + Log::Comment(L"========== Resize the Terminal and conpty =========="); + + auto resizeResult = term->UserResize(newSize); + VERIFY_SUCCEEDED(resizeResult); + _resizeConpty(newSize.width(), newSize.height()); + + // After we resize, make sure to get the new textBuffers + return { &ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer().GetTextBuffer(), + term->_buffer.get() }; +} + void ConptyRoundtripTests::ConptyOutputTestCanary() { Log::Comment(NoThrowString().Format( @@ -846,24 +862,15 @@ void ConptyRoundtripTests::TestResizeHeight() verifyHostData(*hostTb); verifyTermData(*termTb); - const COORD newViewportSize{ - ::base::saturated_cast(TerminalViewWidth + dx), - ::base::saturated_cast(TerminalViewHeight + dy) - }; - - Log::Comment(NoThrowString().Format(L"Resize the Terminal and conpty here")); - auto resizeResult = term->UserResize(newViewportSize); - VERIFY_SUCCEEDED(resizeResult); - _resizeConpty(newViewportSize.X, newViewportSize.Y); - + const til::size newViewportSize{ TerminalViewWidth + dx, + TerminalViewHeight + dy }; // After we resize, make sure to get the new textBuffers - hostTb = &si.GetTextBuffer(); - termTb = term->_buffer.get(); + std::tie(hostTb, termTb) = _performResize(newViewportSize); // Conpty's doesn't have a scrollback, it's view's origin is always 0,0 const auto thirdHostView = si.GetViewport(); VERIFY_ARE_EQUAL(0, thirdHostView.Top()); - VERIFY_ARE_EQUAL(newViewportSize.Y, thirdHostView.BottomExclusive()); + VERIFY_ARE_EQUAL(newViewportSize.height(), thirdHostView.BottomExclusive()); // The Terminal should be stuck to the top of the viewport, unless dy<0, // rows=50. In that set of cases, we _didn't_ pin the top of the Terminal to @@ -891,7 +898,7 @@ void ConptyRoundtripTests::TestResizeHeight() // Conpty's doesn't have a scrollback, it's view's origin is always 0,0 const auto fourthHostView = si.GetViewport(); VERIFY_ARE_EQUAL(0, fourthHostView.Top()); - VERIFY_ARE_EQUAL(newViewportSize.Y, fourthHostView.BottomExclusive()); + VERIFY_ARE_EQUAL(newViewportSize.height(), fourthHostView.BottomExclusive()); // The Terminal should be stuck to the top of the viewport, unless dy<0, // rows=50. In that set of cases, we _didn't_ pin the top of the Terminal to @@ -2524,19 +2531,9 @@ void ConptyRoundtripTests::ResizeRepaintVimExeBuffer() Log::Comment(L"========== Checking the terminal buffer state (before) =========="); verifyBuffer(*termTb, term->_mutableViewport.ToInclusive()); - Log::Comment(L"========== Resize the Terminal and conpty here =========="); - const COORD newViewportSize{ - ::base::saturated_cast(TerminalViewWidth - 1), - ::base::saturated_cast(TerminalViewHeight) - }; - - auto resizeResult = term->UserResize(newViewportSize); - VERIFY_SUCCEEDED(resizeResult); - _resizeConpty(newViewportSize.X, newViewportSize.Y); - // After we resize, make sure to get the new textBuffers - hostTb = &si.GetTextBuffer(); - termTb = term->_buffer.get(); + std::tie(hostTb, termTb) = _performResize({ TerminalViewWidth - 1, + TerminalViewHeight }); Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); @@ -2559,10 +2556,10 @@ void ConptyRoundtripTests::TestResizeWithCookedRead() { // see https://github.com/microsoft/terminal/issues/1856 Log::Comment(L"This test checks a crash in conpty where resizing the " - L"window with any data in a cooked read (like thhe input line " + L"window with any data in a cooked read (like the input line " L"in cmd.exe) would cause the conpty to crash."); - // Resizing with a COKKED_READ used to cause a crash in + // Resizing with a COOKED_READ used to cause a crash in // `Selection::s_GetInputLineBoundaries` north of // `Selection::GetValidAreaBoundaries`. // @@ -2598,27 +2595,20 @@ void ConptyRoundtripTests::TestResizeWithCookedRead() // Setup the cooked read data m_state->PrepareReadHandle(); - m_state->PrepareCookedReadData({ "This is some cooked read data\0" }); + // TODO GH#5618: This string will get mangled, but we don't really care + // about the buffer contents in this test, so it doesn't really matter. + const std::string_view cookedReadContents{ "This is some cooked read data" }; + m_state->PrepareCookedReadData(cookedReadContents); Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); - Log::Comment(L"========== Resize the Terminal and conpty here =========="); - const COORD newViewportSize{ - ::base::saturated_cast(TerminalViewWidth + dx), - ::base::saturated_cast(TerminalViewHeight + dy) - }; - - auto resizeResult = term->UserResize(newViewportSize); - VERIFY_SUCCEEDED(resizeResult); - _resizeConpty(newViewportSize.X, newViewportSize.Y); - // After we resize, make sure to get the new textBuffers - hostTb = &si.GetTextBuffer(); - termTb = term->_buffer.get(); + std::tie(hostTb, termTb) = _performResize({ TerminalViewWidth + dx, + TerminalViewHeight + dy }); Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); - // By simply reaching the + // By simply reaching the end of this test, we know that we didn't crash. Hooray! }