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

BUG: Minimizing Terminal Crash #1795

Closed
evlli opened this issue Jul 3, 2019 · 6 comments · Fixed by #2149
Closed

BUG: Minimizing Terminal Crash #1795

evlli opened this issue Jul 3, 2019 · 6 comments · Fixed by #2149
Assignees
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone

Comments

@evlli
Copy link

evlli commented Jul 3, 2019

Environment

Windows build number: 10.0.18362.175
Windows Terminal version: 0.2.1831.0

Steps to reproduce

Execute a command that keeps outputting something for a while.
for example you can user apt list or ls in a directory with a lot of content.

Then, while the command is running, double click on the titlebar or click the minimize? button.

Expected behavior

The behavior I would have expected was for it to pause or keep running

Actual behavior

The Terminal Freezes and crashes after approximately 1,5 seconds
Other Terminal windows are unaffected

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 3, 2019
@zadjii-msft
Copy link
Member

I'm not sure if it's the same crash, but while trying to repro this, I managed to get an exception while restoring down from maximize while the app was outputting.

...
10 (Inline Function) --------`-------- conhost!wil::details::in1diag3::Throw_HrIf+0x17 [c:\ba\2009\s\dep\wil\include\wil\result_macros.h @ 5039] 
11 0000008b`002ff6d0 00007ff6`ceb4e2e5 conhost!TextBufferCellIterator::TextBufferCellIterator+0x281 [c:\ba\2009\s\src\buffer\out\textbuffercelliterator.cpp @ 43] 
12 (Inline Function) --------`-------- conhost!TextBuffer::GetCellDataAt+0x19 [c:\ba\2009\s\src\buffer\out\textbuffer.cpp @ 169] 
13 0000008b`002ff740 00007ff6`ceb4cefa conhost!Microsoft::Console::Render::Renderer::_PaintBufferOutput+0x1c5 [c:\ba\2009\s\src\renderer\base\renderer.cpp @ 573] 
14 0000008b`002ff8b0 00007ff6`ceb4ccca conhost!Microsoft::Console::Render::Renderer::_PaintFrameForEngine+0x1ea [c:\ba\2009\s\src\renderer\base\renderer.cpp @ 112] 
15 0000008b`002ff9a0 00007ff6`ceb4f6a2 conhost!Microsoft::Console::Render::Renderer::PaintFrame+0x6a [c:\ba\2009\s\src\renderer\base\renderer.cpp @ 65] 
16 (Inline Function) --------`-------- conhost!Microsoft::Console::Render::RenderThread::_ThreadProc+0x40 [c:\ba\2009\s\src\renderer\base\thread.cpp @ 165] 
17 0000008b`002ff9d0 00007ffd`26ef18a4 conhost!Microsoft::Console::Render::RenderThread::s_ThreadProc+0x52

Looked like it was trying to draw one of the lines that's now outside the viewport. Perhaps we're not locking the terminal properly for certain resize types? Or we've moved the viewport to a bad position on restore down? Fortunately, the repro is easy enough, so we just need to dig in more.

Thanks!

@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. Issue-Bug It either shouldn't be doing this or needs an investigation. labels Jul 3, 2019
@zadjii-msft zadjii-msft added this to the Terminal v0.3 - Preview 2 milestone Jul 3, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 3, 2019
@DHowett-MSFT DHowett-MSFT added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 8, 2019
@zadjii-msft zadjii-msft self-assigned this Jul 26, 2019
@zadjii-msft zadjii-msft added Product-Conpty For console issues specifically related to conpty and removed Help Wanted We encourage anyone to jump in on these. Product-Terminal The new Windows Terminal. labels Jul 26, 2019
@zadjii-msft
Copy link
Member

Leaving notes here for me after the weekend:

Trying to repro this, the terminal moves between [118x26]<->[207x61]

When we crash, the conpty's buffer has 61 rows with 118 chars each. We're trying to render row 60, and we want all 207 characters from it. Seems to imply that the buffer's width wasn't resized, but the height was.

That's not right.

@zadjii-msft
Copy link
Member

Okay debugging more into this. For my repro, the resize is from [207x61] -> [118x26]

When we resize in ConPty, we call both SetConsoleScreenBufferInfoEx and if that succeeds, SetConsoleWindowInfo. In SetConsoleScreenBufferInfoEx, we'll perform 2 ResizeWithReflow operations(!).

  • The first will resize from [207x61] -> [118x61], changing the width of all the rows.
  • The second will resize from [118x61] -> [118x26], changing the height of the buffer. The second resize is triggered by the ScreenInfo::SetViewportSize call near the bottom ofSetConsoleScreenBufferInfoEx.
    • During this resize, we'll copy the first 26 rows (up to iOldRow = 25) just fine. However, the problem comes when we try to newline the cursor. When that happens on the 25th row, we'll attempt to circle the buffer, to shift all the buffer contents up a row, and give us a new row to work with. Hypothetically, this will happen 35 more times, as we copy all the old lines into the new buffer.
    • HOWEVER, as we call TextBuffer::IncrementCircularBuffer, we'll also tell the renderer that it should give the render engines a chance to paint, before any old text was lost. This is helpful when a newline is normally output to the terminal, but isn't really necessary to do during a resize.
    • The renderer will ask the VT engine if it wants to paint to handle the buffer circling. The VT engine will always say "yes".
    • The renderer will then immediately render a new frame for the VT engine. Somewhere during this frame, the vt engine will throw an exception.
    • The exception will get caught way back up in SetConsoleScreenBufferInfoEx. The rest of the buffer will fail to get copied, and even worse, we won't even move the [118x26] text buffer into the screen buffer, leaving the screen buffer with a incorrectly sized buffer.

Later on, we'll try and render a new frame, and try to render the 60th line of the buffer, with a width of 207 chars. The buffer does incorrectly still have 61 lines, but it only has 118 chars per row, so the renderer throws an exception that's not caught here, crashing conpty. Conpty crashing causes the terminal to crash.

Now we just need to come up with the right solution for handling this resize. Very simply, we could maybe tell conpty to not paint at all during a resize. Might be too risky a change for 0.3, however.

@mcpiroman
Copy link
Contributor

mcpiroman commented Jul 29, 2019

@zadjii-msft Maybe, kind of, #1946? It got lost somewhere in the swarm of issues few weeks ago :(

@zadjii-msft
Copy link
Member

@mcpiroman It's probably related to the larger class of "conpty resizing issues". I think it might be able to improve the QOL for the terminal by fixing this particular bug now, while a more complete design of the right way to handle conpty resizing is a while off. I do still owe you a response on that other issue though.

zadjii-msft added a commit that referenced this issue Jul 29, 2019
  When we resize the conpty buffer, we'll call into
  SCREEN_INFORMATION::_InternalSetViewportSize, which will try to keep the
  commandline visible.
  However, we don't even want the commandline visible while that's happening.
  We should hide it and reflow the start of the commandline.

  I'm actually not sure this is the fix for #1095. #1095 uses WSL, which isn't
  going to be COOKED_READing.

  Aditionally, the commandline can end up in some weird places after this. Like
  when restoring down with a commandline wider than the restored width, the
  commandline will appear one line below where it should. That might be a side
  effect of the rest of the changes I'm prototyping (for #1795)
zadjii-msft added a commit that referenced this issue Jul 29, 2019
…operation

  This fixes #1795, and shined quite a bit of light on the whole conpty resize process.
@ghost ghost added the In-PR This issue has a related PR label Jul 30, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 30, 2019
zadjii-msft added a commit that referenced this issue Jul 30, 2019
* 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.
@ghost
Copy link

ghost commented Aug 3, 2019

🎉This issue was addressed in #2149, which has now been successfully released as Windows Terminal Preview v0.3.2142.0.:tada:

Handy links:

DHowett-MSFT pushed a commit that referenced this issue Aug 6, 2019
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants