Skip to content

Commit

Permalink
Fix resize crash in OpenConsole with AtlasEngine (#13015)
Browse files Browse the repository at this point in the history
`_api.cellCount` caches the `TextBuffer` size in AtlasEngine.
Calculating it based on the `_api.sizeInPixel` is incorrect as the
`TextBuffer` size doesn't necessarily have to be the size of the window.
This can occur when the window is resized, as the main thread is receiving its
`WM_SIZE` message and resizing the `TextBuffer` concurrently with the render
thread performing a render pass and AtlasEngine checking the `GetClientRect`.

In order to inform `AtlasEngine` about the initial buffer size, `Renderer`
was modified to also invoke `UpdateViewport()` on the first render cycle.

The only other user of `UpdateViewport()` is `VtEngine` which used to call
`InvalidateAll()` in these situations. In order to prevent the `InvalidateAll()`
call, `VtEngine::UpdateViewport()` was modified to suppress this.

## Validation Steps Performed
* Resizing wide characters doesn't crash the terminal anymore ✅
* The additional call to `UpdateViewport()` doesn't break VtEngine ✅
  • Loading branch information
lhecker committed May 3, 2022
1 parent c9468aa commit ad2358d
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 31 deletions.
5 changes: 3 additions & 2 deletions src/renderer/atlas/AtlasEngine.api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ constexpr HRESULT vec2_narrow(U x, U y, AtlasEngine::vec2<T>& out) noexcept

[[nodiscard]] HRESULT AtlasEngine::UpdateViewport(const SMALL_RECT srNewViewport) noexcept
{
_api.cellCount.x = gsl::narrow_cast<u16>(srNewViewport.Right - srNewViewport.Left + 1);
_api.cellCount.y = gsl::narrow_cast<u16>(srNewViewport.Bottom - srNewViewport.Top + 1);
WI_SetFlag(_api.invalidations, ApiInvalidations::Size);
return S_OK;
}

Expand Down Expand Up @@ -396,7 +399,6 @@ void AtlasEngine::SetWarningCallback(std::function<void(HRESULT)> pfn) noexcept
if (_api.sizeInPixel != newSize && newSize != u16x2{})
{
_api.sizeInPixel = newSize;
_api.cellCount = _api.sizeInPixel / _api.fontMetrics.cellSize;
WI_SetFlag(_api.invalidations, ApiInvalidations::Size);
}

Expand Down Expand Up @@ -536,7 +538,6 @@ void AtlasEngine::_updateFont(const wchar_t* faceName, const FontInfoDesired& fo

if (previousCellSize != _api.fontMetrics.cellSize)
{
_api.cellCount = _api.sizeInPixel / _api.fontMetrics.cellSize;
WI_SetFlag(_api.invalidations, ApiInvalidations::Size);
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/renderer/base/renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -390,12 +390,13 @@ bool Renderer::_CheckViewportAndScroll()
const auto srOldViewport = _viewport.ToInclusive();
const auto srNewViewport = _pData->GetViewport().ToInclusive();

if (srOldViewport == srNewViewport)
if (!_forceUpdateViewport && srOldViewport == srNewViewport)
{
return false;
}

_viewport = Viewport::FromInclusive(srNewViewport);
_forceUpdateViewport = false;

COORD coordDelta;
coordDelta.X = srOldViewport.Left - srNewViewport.Left;
Expand Down
1 change: 1 addition & 0 deletions src/renderer/base/renderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ namespace Microsoft::Console::Render
std::vector<SMALL_RECT> _previousSelection;
std::function<void()> _pfnRendererEnteredErrorState;
bool _destructing = false;
bool _forceUpdateViewport = true;

#ifdef UNIT_TESTING
friend class ConptyOutputTests;
Expand Down
57 changes: 29 additions & 28 deletions src/renderer/vt/state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,18 +246,41 @@ CATCH_RETURN();
[[nodiscard]] HRESULT VtEngine::UpdateViewport(const SMALL_RECT srNewViewport) noexcept
{
auto hr = S_OK;
const auto oldView = _lastViewport;
const auto newView = Viewport::FromInclusive(srNewViewport);
const auto oldSize = _lastViewport.Dimensions();
const auto newSize = newView.Dimensions();

_lastViewport = newView;

if ((oldView.Height() != newView.Height()) || (oldView.Width() != newView.Width()))
if (oldSize != newSize)
{
// Don't emit a resize event if we've requested it be suppressed
if (!_suppressResizeRepaint)
{
hr = _ResizeWindow(newView.Width(), newView.Height());
hr = _ResizeWindow(newSize.X, newSize.Y);
}

if (_resizeQuirk)
{
// GH#3490 - When the viewport width changed, don't do anything extra here.
// If the buffer had areas that were invalid due to the resize, then the
// buffer will have triggered it's own invalidations for what it knows is
// invalid. Previously, we'd invalidate everything if the width changed,
// because we couldn't be sure if lines were reflowed.
_invalidMap.resize(til::size{ newSize });
}
else
{
if (SUCCEEDED(hr))
{
_invalidMap.resize(til::size{ newSize }, true); // resize while filling in new space with repaint requests.

// Viewport is smaller now - just update it all.
if (oldSize.Y > newSize.Y || oldSize.X > newSize.X)
{
hr = InvalidateAll();
}
}
}

_resized = true;
}

Expand All @@ -269,29 +292,7 @@ CATCH_RETURN();
// If we only clear the flag when the new viewport is different, this can
// lead to the first _actual_ resize being suppressed.
_suppressResizeRepaint = false;

if (_resizeQuirk)
{
// GH#3490 - When the viewport width changed, don't do anything extra here.
// If the buffer had areas that were invalid due to the resize, then the
// buffer will have triggered it's own invalidations for what it knows is
// invalid. Previously, we'd invalidate everything if the width changed,
// because we couldn't be sure if lines were reflowed.
_invalidMap.resize(til::size{ newView.Dimensions() });
}
else
{
if (SUCCEEDED(hr))
{
_invalidMap.resize(til::size{ newView.Dimensions() }, true); // resize while filling in new space with repaint requests.

// Viewport is smaller now - just update it all.
if (oldView.Height() > newView.Height() || oldView.Width() > newView.Width())
{
hr = InvalidateAll();
}
}
}
_lastViewport = newView;

return hr;
}
Expand Down

0 comments on commit ad2358d

Please sign in to comment.