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

Fix VtEngine hang when resizing while scrolling #15618

Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jun 28, 2023

This fixes a bug reported internally that occurs when resizing the
terminal while also scolling the contents. The easiest way to reproduce
it is to resize the terminal to 0 rows, but it's much more prominent
in a debug build where everything goes out of sync almost immediately.

The underlying issue is that VtEngine::_wrappedRow may contain an
offset that is outside of the viewport bounds, because reflowing and
scrolling aren't properly synchronized. The previous bitmap code
would then throw an exception for such invalid coordinates and cause
the internal VtEngine state to be broken. Once _wrappedRow got
to a negative value at least once, it would stay that way unless you're
scrolling up. If the contents are actively scrolling it would quickly
reach a negative value from which it can never recover. At that point
OpenConsole would enter a tight exception-throw-catch-retry loop
and Windows Terminal seemingly cease to show any content.

Validation Steps Performed

  • Resize WT to the minimal window size repeatedly
  • Doesn't hang ✅

@lhecker lhecker added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Jun 28, 2023
{
_bits.set(_rc.index_of(til::point{ rc.left, row }), rc.width(), true);
_bits.set(idx, width, true);
Copy link
Member Author

@lhecker lhecker Jun 28, 2023

Choose a reason for hiding this comment

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

I chose to change til::bitmap instead of VtEngine, because I think til::bitmap shouldn't throw for out of bounds coordinates.

Furthermore, fixing the reflow/scroll aspect of VtEngine might be a losing battle, because reading through the _wrappedRow implementation gave me the impression that it's not doing the right thing anyways. XtermEngine::ScrollFrame() in particular sets _wrappedRow to nullopt temporarily and then restores its initial value which is... weird? It's definitely a "spooky action from a distance" anti-pattern. 😅 ...and not very exception safe either. If we were to fix VtEngine I would instead opt to fix it fundamentally. I'm sure that whetever delay-wrap logic we got, we can solve some other way and more robustly too, for instance with out-of-bounds coordinates, etc. 🙂

@lhecker lhecker force-pushed the dev/lhecker/vtengine-resize-scroll branch from 92739ef to f55be3e Compare June 28, 2023 14:02
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 28, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot enabled auto-merge (squash) June 28, 2023 23:59
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the dev/lhecker/vtengine-resize-scroll branch June 29, 2023 20:49
DHowett pushed a commit that referenced this pull request Jul 17, 2023
This fixes a bug reported internally that occurs when resizing the
terminal while also scolling the contents. The easiest way to reproduce
it is to resize the terminal to 0 rows, but it's much more prominent
in a debug build where everything goes out of sync almost immediately.

The underlying issue is that `VtEngine::_wrappedRow` may contain an
offset that is outside of the viewport bounds, because reflowing and
scrolling aren't properly synchronized. The previous `bitmap` code
would then throw an exception for such invalid coordinates and cause
the internal `VtEngine` state to be broken. Once `_wrappedRow` got
to a negative value at least once, it would stay that way unless you're
scrolling up. If the contents are actively scrolling it would quickly
reach a negative value from which it can never recover. At that point
OpenConsole would enter a tight exception-throw-catch-retry loop
and Windows Terminal seemingly cease to show any content.

## Validation Steps Performed
* Resize WT to the minimal window size repeatedly
* Doesn't hang ✅

(cherry picked from commit 358e10b)
Service-Card-Id: 89739741
Service-Version: 1.17
DHowett pushed a commit that referenced this pull request Jul 27, 2023
This fixes a bug reported internally that occurs when resizing the
terminal while also scolling the contents. The easiest way to reproduce
it is to resize the terminal to 0 rows, but it's much more prominent
in a debug build where everything goes out of sync almost immediately.

The underlying issue is that `VtEngine::_wrappedRow` may contain an
offset that is outside of the viewport bounds, because reflowing and
scrolling aren't properly synchronized. The previous `bitmap` code
would then throw an exception for such invalid coordinates and cause
the internal `VtEngine` state to be broken. Once `_wrappedRow` got
to a negative value at least once, it would stay that way unless you're
scrolling up. If the contents are actively scrolling it would quickly
reach a negative value from which it can never recover. At that point
OpenConsole would enter a tight exception-throw-catch-retry loop
and Windows Terminal seemingly cease to show any content.

## Validation Steps Performed
* Resize WT to the minimal window size repeatedly
* Doesn't hang ✅

(cherry picked from commit 358e10b)
Service-Card-Id: 89739742
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants