Skip to content

Commit

Permalink
Don't crash when restore-down'ing the alt buffer (#2666)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

When a user had "Disable Scroll Forward" enabled and switched to the alt buffer and maximized the console, then restored down, we'd crash. Now we don't.

## References

## PR Checklist
* [x] Closes #1206
* [x] I work here
* [x] Tests added/passed

## Detailed Description of the Pull Request / Additional comments

The problem is that we'd previously try to "anchor" the viewport to the virtual bottom when resizing like this. This would also cause us to move the top of the viewport down, into the buffer. However, if the alt buffer is getting smaller, we don't want to do this - if we anchor to the old _virtualBottom, the bottom of the viewport will actually be outside the current buffer.

This could theoretically happen with the main buffer too, but it's much easier to repro with the alt buffer.

(cherry picked from commit c58033c)
  • Loading branch information
zadjii-msft authored and DHowett committed Sep 9, 2019
1 parent 511ce6a commit 1284fca
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 2 deletions.
7 changes: 5 additions & 2 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1231,10 +1231,13 @@ void SCREEN_INFORMATION::_InternalSetViewportSize(const COORD* const pcoordSize,

// See MSFT:19917443
// If we're in terminal scrolling mode, and we've changed the height of the
// viewport, the new viewport's bottom to the _virtualBottom
// viewport, the new viewport's bottom to the _virtualBottom.
// GH#1206 - Only do this if the viewport is _growing_ in height. This can
// cause unexpected behavior if we try to anchor the _virtualBottom to a
// position that will be greater than the height of the buffer.
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto newViewport = Viewport::FromInclusive(srNewViewport);
if (gci.IsTerminalScrolling() && newViewport.Height() != _viewport.Height())
if (gci.IsTerminalScrolling() && newViewport.Height() >= _viewport.Height())
{
const short newTop = static_cast<short>(std::max(0, _virtualBottom - (newViewport.Height() - 1)));

Expand Down
66 changes: 66 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ class ScreenBufferTests
TEST_METHOD(SetOriginMode);

TEST_METHOD(HardResetBuffer);

TEST_METHOD(RestoreDownAltBufferWithTerminalScrolling);
};

void ScreenBufferTests::SingleAlternateBufferCreationTest()
Expand Down Expand Up @@ -3581,3 +3583,67 @@ void ScreenBufferTests::HardResetBuffer()
VERIFY_ARE_EQUAL(COORD({ 0, 0 }), viewport.Origin());
VERIFY_ARE_EQUAL(COORD({ 0, 0 }), cursor.GetPosition());
}

void ScreenBufferTests::RestoreDownAltBufferWithTerminalScrolling()
{
// This is a test for microsoft/terminal#1206. Refer to that issue for more
// context

CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.SetTerminalScrolling(true);
gci.LockConsole(); // Lock must be taken to manipulate buffer.
auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });

auto& siMain = gci.GetActiveOutputBuffer();
COORD const coordFontSize = siMain.GetScreenFontSize();
siMain._virtualBottom = siMain._viewport.BottomInclusive();

auto originalView = siMain._viewport;

VERIFY_IS_NULL(siMain._psiMainBuffer);
VERIFY_IS_NULL(siMain._psiAlternateBuffer);

Log::Comment(L"Create an alternate buffer");
if (VERIFY_IS_TRUE(NT_SUCCESS(siMain.UseAlternateScreenBuffer())))
{
VERIFY_IS_NOT_NULL(siMain._psiAlternateBuffer);
auto& altBuffer = *siMain._psiAlternateBuffer;
VERIFY_ARE_EQUAL(0, altBuffer._viewport.Top());
VERIFY_ARE_EQUAL(altBuffer._viewport.BottomInclusive(), altBuffer._virtualBottom);

const COORD originalSize = originalView.Dimensions();
const COORD doubledSize = { originalSize.X * 2, originalSize.Y * 2 };

// Create some RECTs, which are dimensions in pixels, because
// ProcessResizeWindow needs to work on rects in screen _pixel_
// dimensions, not character sizes.
RECT originalClientRect{ 0 }, maximizedClientRect{ 0 };

originalClientRect.right = originalSize.X * coordFontSize.X;
originalClientRect.bottom = originalSize.Y * coordFontSize.Y;

maximizedClientRect.right = doubledSize.X * coordFontSize.X;
maximizedClientRect.bottom = doubledSize.Y * coordFontSize.Y;

Log::Comment(NoThrowString().Format(
L"Emulate a maximize"));
// Note that just calling _InternalSetViewportSize does not hit the
// exceptional case here. There's other logic farther down the stack
// that triggers it.
altBuffer.ProcessResizeWindow(&maximizedClientRect, &originalClientRect);

VERIFY_ARE_EQUAL(0, altBuffer._viewport.Top());
VERIFY_ARE_EQUAL(altBuffer._viewport.BottomInclusive(), altBuffer._virtualBottom);

Log::Comment(NoThrowString().Format(
L"Emulate a restore down"));

altBuffer.ProcessResizeWindow(&originalClientRect, &maximizedClientRect);

// Before the bugfix, this would fail, with the top being roughly 80,
// halfway into the buffer, with the bottom being anchored to the old
// size.
VERIFY_ARE_EQUAL(0, altBuffer._viewport.Top());
VERIFY_ARE_EQUAL(altBuffer._viewport.BottomInclusive(), altBuffer._virtualBottom);
}
}

0 comments on commit 1284fca

Please sign in to comment.