maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Hide the commandline on a resize to prevent a crash when snapping the window (#5620)

Browse source 
Hide any commandline (cooked read) we have before we begin a resize, and
show it again after the resize. 

## References

* I found #5618 while I was working on this.

## PR Checklist
* [x] Closes #1856
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Basically, during a resize, we try to restore the viewport position
correctly, and part of that checks where the current commandline ends.
However, when we do that, the commandline's _current_ state still
reflects the _old_ buffer size, so resizing to be smaller can cause us
to throw an exception, when we find that the commandline doesn't fit in
the new viewport cleanly.

By hiding it, then redrawing it, we avoid this problem entirely. We
don't need to perform the check on the old commandline contents (since
they'll be empty), and we'll redraw it just fine for the new buffer size

## Validation Steps Performed
* ran tests
* checked resizing, snapping in conhost with a cooked read
* checked resizing, snapping in the Terminal with a cooked read
This commit is contained in:
Mike Griese 2020-04-29 18:47:56 -05:00 committed by GitHub
parent ea02128f11
commit 10fa3108e1
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 96 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

112
src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file

@ -196,10 +196,15 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
TEST_METHOD(ResizeRepaintVimExeBuffer);
TEST_METHOD(TestResizeWithCookedRead);
private:
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<TextBuffer*, TextBuffer*> _performResize(const til::size& newSize);
std::deque<std::string> expectedOutput;
std::unique_ptr<Microsoft::Console::Render::VtEngine> _pVtRenderEngine;
std::unique_ptr<CommonState> m_state;
@ -271,6 +276,19 @@ void ConptyRoundtripTests::_resizeConpty(const unsigned short sx,
}
}
[[nodiscard]] std::tuple<TextBuffer*, TextBuffer*> 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<unsigned short>(), newSize.height<unsigned short>());
// 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(
@ -844,24 +862,15 @@ void ConptyRoundtripTests::TestResizeHeight()
verifyHostData(*hostTb);
verifyTermData(*termTb);
const COORD newViewportSize{
::base::saturated_cast<short>(TerminalViewWidth + dx),
::base::saturated_cast<short>(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
@ -889,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
@ -2522,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<short>(TerminalViewWidth - 1),
::base::saturated_cast<short>(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());
@ -2552,3 +2551,64 @@ 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 the input line "
L"in cmd.exe) would cause the conpty to crash.");
// Resizing with a COOKED_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();
// 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());
// After we resize, make sure to get the new textBuffers
std::tie(hostTb, termTb) = _performResize({ TerminalViewWidth + dx,
TerminalViewHeight + dy });
Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());
// By simply reaching the end of this test, we know that we didn't crash. Hooray!
}

8
src/host/getset.cpp
View file

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

4
src/inc/test/CommonState.hpp
View file

@ -124,7 +124,7 @@ public:
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 @@ public:
0,
nullptr,
L"",
{});
initialData);
gci.SetCookedReadData(readData);
}