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.

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

Clamp the new rows scrolling value to a positive number #5630

Merged
3 commits merged into from
Apr 29, 2020

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 28, 2020

Summary of the Pull Request

This PR clamp the "new rows" scrolling value to a positive number. We can't create a negative number of new rows. It also adds a test.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

The origin of this bug is that as newlines are emitted, we'll accumulate an enormous scroll delta into a selection region, to the point of overflowing a SHORT. When the overflow occurs, the Terminal would fail to send a NotifyScroll() to the TermControl hosting it.

For this bug to repro, we need to:

  • Have a sufficiently large buffer, because each newline we'll accumulate a delta of (0, ~bufferHeight), so (bufferHeight^2 + bufferHeight) > SHRT_MAX
  • Have a selection

Validation Steps Performed

  • Dustin verified this actually
  • Created a new insane test case

@DHowett-MSFT
Copy link
Contributor

Guess what! You got it.

image

image

image

Because we're hugely negative, we fail to notifyscroll.

@DHowett-MSFT
Copy link
Contributor

Fixes #5540

@zadjii-msft
Copy link
Member Author

I'm working on a test for this, so hopefully we can catch this if it regresses. Since this is a Terminal-Renderer-DxEngine interaction that causes this, it's a little tricky to get the test into the right state. Hopefully I can get it figured out, otherwise we might just ship this without the test.

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Wow, and booting up a renderer isn't weird or bad or scary here?

@zadjii-msft
Copy link
Member Author

@DHowett-MSFT nah, we don't have a thread so it's fine, and the RoundtripTests already do something similar

@zadjii-msft zadjii-msft added AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal. labels Apr 29, 2020
@ghost
Copy link

ghost commented Apr 29, 2020

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 1ce86f8 into master Apr 29, 2020
@ghost ghost deleted the dev/migrie/b/i-have-no-idea-what-this-is branch April 29, 2020 19:29
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scroll Sometimes Stops Before Reaching Bottom of Terminal
3 participants