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

Fix a crash on restore down #2149

Merged
merged 2 commits into from
Jul 30, 2019
Merged

Fix a crash on restore down #2149

merged 2 commits into from
Jul 30, 2019

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Jul 30, 2019 •
edited
Loading

Summary of the Pull Request

Fixes a crash that occur when changing the buffer height. During the resize, conpty would immediately request a repaint, which would throw an exception that would cause the resize to fail halfway through, and that would make the subsequent render fail.

This change prevents the VT Renderer from immediately requesting a paint during the resize.

References

Might also be related to #1095. I can't seem to get that to repro anymore with this change.

Might be related to #1856. We should verify.

This does however make another issue more apparent. When you restore down with a COOKED_READ input line (read:cmd.exe), the terminal can crash. I have a fix for that in d34a3be, which I wasn't sure if I should include in this PR or not, because I wasn't confident that was the right fix. I'm fairly certain there's another issue floating around tracking that bug. We could either merge that commit/branch into this one, or wait and save it for a separate one.

PR Checklist

@miniksa
Copy link
Member

miniksa commented Jul 30, 2019

For your tentative COOKED_READ fix, that is sort of how the v1 console used to do it when resizing anything. It would hide the cooked line, do the work, then redraw it in the new position. So I'm not really opposed to that.

My thought would be... should it belong to SCREEN_INFORMATION::SetViewportSize instead, though, because I bet other folks resizing it probably also have the same problem if it's true here.

miniksa
miniksa requested changes Jul 30, 2019
View reviewed changes
src/host/VtIo.cpp Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 30, 2019
@DHowett-MSFT
Copy link
Contributor

Is this something we should cherry-pick into 20H1?

@zadjii-msft
Copy link
Member Author

@DHowett-MSFT Probably.

@miniksa Okay that makes sense. I'll make another PR for that one, since this one has a pretty specific scope, as does the other.

@zadjii-msft
Copy link
Member Author

@DHowett-MSFT Oh you said 20H1, not Vb. Eh, feels too risky for 20H1.

@zadjii-msft zadjii-msft mentioned this pull request Jul 30, 2019
Open
@zadjii-msft
Copy link
Member Author

I was wrong this should be in 20H1 (which is Vb). Our codenames are confusing.

@zadjii-msft zadjii-msft requested a review from miniksa July 30, 2019 21:33
zadjii-msft added a commit that referenced this pull request Jul 30, 2019
@zadjii-msft
Start trying to do a raii thing
316bb93 
  This looks super weird though. There's gotta be a better way (and there is Kevin)

  Related to #2162, #2149
@zadjii-msft zadjii-msft merged commit 7abcc35 into master Jul 30, 2019
@zadjii-msft zadjii-msft deleted the dev/migrie/b/1795-restore-down-crash branch July 30, 2019 22:03
@ghost
Copy link

ghost commented Aug 3, 2019

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

Handy links:

@ghost ghost mentioned this pull request Aug 3, 2019
Closed
DHowett-MSFT pushed a commit that referenced this pull request Aug 6, 2019
@zadjii-msft @DHowett
Fix a crash on restore down (#2149)
3385f8c 
* Don't trigger a frame due to circling when in the middle of a resize operation

  This fixes #1795, and shined quite a bit of light on the whole conpty resize process.

* Move the Begin/End to ResizeScreenBuffer, to catch more cases.

(cherry picked from commit 7abcc35)
This was referenced Aug 8, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa approved these changes

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

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Minimizing Terminal Crash
3 participants
@zadjii-msft @miniksa @DHowett-MSFT