Skip to content

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

Merged
3 commits merged intomasterfrom
dev/migrie/b/i-have-no-idea-what-this-is
Apr 29, 2020
Merged

Clamp the new rows scrolling value to a positive number#5630
3 commits merged intomasterfrom
dev/migrie/b/i-have-no-idea-what-this-is

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 28, 2020 •
edited
Loading

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

DHowett-MSFT commented Apr 28, 2020

Guess what! You got it.

image

image

image

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

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Apr 28, 2020

Fixes #5540

@zadjii-msft
Copy link
Member Author

zadjii-msft commented Apr 29, 2020

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.

DHowett-MSFT
DHowett-MSFT approved these changes Apr 29, 2020
View reviewed changes
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

zadjii-msft commented Apr 29, 2020

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

@ghost ghost mentioned this pull request May 5, 2020
Closed
@beviu beviu mentioned this pull request Jun 5, 2020
Merged
5 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

+1 more reviewer

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Scroll Sometimes Stops Before Reaching Bottom of Terminal

3 participants

@zadjii-msft @DHowett-MSFT @carlos-zamora