Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
3 commits merged into from
Apr 29, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14fe652
This is the test for #1856
zadjii-msft Apr 28, 2020
0d6dba5
This is the fix for #1856
zadjii-msft Apr 28, 2020
f294def
add a resizing helper that will force you to do the right thing
zadjii-msft Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 86 additions & 26 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand All @@ -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 changes: 8 additions & 0 deletions src/host/getset.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment on lines +592 to +600
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this have an impact on any traditional console interactive scenarios? I'm a bit worried about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh -- resizing goes through here before because it's coming off the pty's resize handle? Does a user-initiated window resize on conhost run through ApiRoutines too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty OK with this because the console host used to do this a ton in the past. It always hid the command line display before manipulating anything and then restored it when it was done. It's kinda sad that we have to do it again, but if it prevents a crash, I'm not that bothered by it.


IConsoleWindow* const pWindow = ServiceLocator::LocateConsoleWindow();
if (pWindow != nullptr)
Expand Down
4 changes: 2 additions & 2 deletions src/inc/test/CommonState.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -135,7 +135,7 @@ class CommonState
0,
nullptr,
L"",
{});
initialData);
gci.SetCookedReadData(readData);
}

Expand Down