Skip to content

Don't crash when restor-down'ing the alt buffer#2666

Merged
zadjii-msft merged 4 commits intomasterfrom
dev/migrie/b/1206-restore-alt-crash
Sep 5, 2019
Merged

Don't crash when restor-down'ing the alt buffer#2666
zadjii-msft merged 4 commits intomasterfrom
dev/migrie/b/1206-restore-alt-crash

Conversation

@zadjii-msft
Copy link
Member

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

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.

Validation Steps Performed

Added tests.
Tried the original repro. Really went wild on the resizing.

@zadjii-msft zadjii-msft added the Product-Conhost For issues in the Console codebase label Sep 5, 2019
@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Sep 5, 2019
@ghost ghost requested review from carlos-zamora and miniksa September 5, 2019 15:51
@zadjii-msft zadjii-msft merged commit c58033c into master Sep 5, 2019
@zadjii-msft zadjii-msft deleted the dev/migrie/b/1206-restore-alt-crash branch September 5, 2019 20:37
DHowett-MSFT pushed a commit that referenced this pull request Sep 23, 2019
## 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)
@ghost
Copy link

ghost commented Sep 24, 2019

🎉Windows Terminal Preview v0.5.2661.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost pushed a commit that referenced this pull request May 2, 2022
The "virtual bottom" marks the last line of the mutable viewport area,
which is the part of the buffer that VT sequences can write to. This
region should typically only move downwards as new lines are added to
the buffer, but there were a number of cases where it was incorrectly
being moved up, or moved down further than necessary. This PR attempts
to fix that.

There was an earlier, unsuccessful attempt to fix this in PR #9770 which
was later reverted (issue #9872 was the reason it had to be reverted).
PRs #2666, #2705, and #5317 were fixes for related virtual viewport
problems, some of which have either been extended or superseded by this
PR.

`SetConsoleCursorPositionImpl` is one of the cases that actually does
need to move the virtual viewport upwards sometimes, in particular when
the cmd shell resets the buffer with a `CLS` command. But when this
operation "snaps" the viewport to the location of the cursor, it needs
to use the virtual viewport as the frame of reference. This was
partially addressed by PR #2705, but that only applied in
terminal-scrolling mode, so I've now applied that fix regardless of the
mode.

`SetViewportOrigin` takes a flag which determines whether it will also
move the virtual bottom to match the visible viewport. In some case this
is appropriate (`SetConsoleCursorPositionImpl` being one example), but
in other cases (e.g. when panning the viewport downwards in the
`AdjustCursorPosition` function), it should only be allowed to move
downwards. We can't just not set the update flag in those cases, because
that also determines whether or not the viewport would be clamped, and
we don't want change that. So what I've done is limit
`SetViewportOrigin` to only move the virtual bottom downwards, and added
an explicit `UpdateBottom` call in those places that may also require
upward movement.

`ResizeWindow` in the `ConhostInternalGetSet` class has a similar
problem to `SetConsoleCursorPositionImpl`, in that it's updating the
viewport to account for the new size, but if that visible viewport is
scrolled back or forward, it would end up placing the virtual viewport
in the wrong place. So again the solution here was to use the virtual
viewport as the frame of reference for the position. However, if the
viewport is being shrunk, this can still result in the cursor falling
below the bottom, so we need an additional check to adjust for that.
This can't be applied in pty mode, though, because that would break the
conpty resizing operation.

`_InternalSetViewportSize` comes into play when resizing the window
manually, and again the viewport after the resize can end up truncating
the virtual bottom if not handled correctly. This was partially
addressed in the original code by clamping the new viewport above the
virtual bottom under certain conditions, and only in terminal scrolling
mode. I've replaced that with a new algorithm which links the virtual
bottom to the visible viewport bottom if the two intersect, but
otherwise leaves it unchanged. This applies regardless of the scrolling
mode.

`ResizeWithReflow` is another sizing operation that can affect the
virtual bottom. This occurs when a change of the window width requires
the buffer to be reflowed, and we need to reposition the viewport in the
newly generated buffer. Previously we were just setting the virtual
bottom to align with the new visible viewport, but that could easily
result in the buffer truncation if the visible viewport was scrolled
back at the time. We now set the virtual bottom to the last non-space
row, or the cursor row (whichever is larger). There'll be edge cases
where this is probably not ideal, but it should still work reasonably
well.

`MakeCursorVisible` was another case where the virtual bottom was being
updated (when requested with a flag) via a `SetViewportOrigin` call.
When I checked all the places it was used, though, none of them actually
required that behavior, and doing so could result in the virtual bottom
being incorrectly positioned, even after `SetViewportOrigin` was limited
to moving the virtual bottom downwards. So I've now made it so that
`MakeCursorVisible` never updates the virtual bottom.

`SelectAll` in the `Selection` class was a similar case. It was calling
`SetViewportOrigin` with the `updateBottom` flag set when that really
wasn't necessary and could result in the virtual bottom being
incorrectly set. I've changed the flag to false now.

## Validation Steps Performed

I've manually confirmed that the test cases in issue #9754 are working
now, except for the one involving margins, which is bigger problem with
`AdjustCursorPosition` which will need to be addressed separately.

I've also double checked the test cases from several other virtual
bottom issues (#1206, #1222, #5302, and #9872), and confirmed that
they're still working correctly with these changes.

And I've added a few screen buffer tests in which I've tried to cover as
many of the problematic code paths as possible.

Closes #9754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maximizing/Restoring when the alt buffer is active crashes conhost

3 participants