Skip to content

Commit

Permalink
Fix conpty not emitting colored spaces on the VERY FIRST frame (micro…
Browse files Browse the repository at this point in the history
…soft#13665)

This bug arose from a "race condition" in the first frame handling of conpty. We'd try to optimize out spaces if we've cleared the entire frame (which always happens on the first frame). However, doing that even for colored spaces meant that things like powerline prompts could be emitted to conhost during the first frame, and we'd optimize the spaces out. That's silly. 

This is hard to repro naturally, but this comment has another repro I used
microsoft#8341 (comment)

Modified to facilitate simpler testing, it looks like:

#### Before
![image](https://user-images.githubusercontent.com/18356694/182680119-bb22179c-a328-43f3-b64a-0d1d5773b813.png)

#### After
![image](https://user-images.githubusercontent.com/18356694/182680159-805964c5-c4cc-411a-8865-3866fca8d6e9.png)


* [x] Closes microsoft#8341
* [x] Tests added

Co-authored by @DHowett
  • Loading branch information
zadjii-msft committed Aug 8, 2022
1 parent ffe9a0f commit 210a98e
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 6 deletions.
73 changes: 73 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
TEST_METHOD(SimpleAltBufferTest);
TEST_METHOD(AltBufferToAltBufferTest);

TEST_METHOD(TestPowerLineFirstFrame);

TEST_METHOD(AltBufferResizeCrash);

TEST_METHOD(TestNoExtendedAttrsOptimization);
Expand Down Expand Up @@ -4111,6 +4113,77 @@ void ConptyRoundtripTests::AltBufferToAltBufferTest()
verifyBuffer(*termAltTb, term->_GetMutableViewport().ToExclusive(), Frame::StillInAltBuffer);
}

void ConptyRoundtripTests::TestPowerLineFirstFrame()
{
Log::Comment(L"This is a test for GH#8341. If we received colored spaces "
L"BEFORE the first frame, we should still emit them!");

auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& sm = si.GetStateMachine();

auto* hostTb = &si.GetTextBuffer();
auto* termTb = term->_mainBuffer.get();

_checkConptyOutput = false;

TextAttribute whiteOnGreen{};
whiteOnGreen.SetIndexedForeground(TextColor::DARK_WHITE);
whiteOnGreen.SetIndexedBackground(TextColor::BRIGHT_GREEN);

TextAttribute greenOnBlack{};
greenOnBlack.SetIndexedForeground(TextColor::BRIGHT_GREEN);
greenOnBlack.SetIndexedBackground(TextColor::BRIGHT_BLACK);

TextAttribute whiteOnBlack{};
whiteOnBlack.SetIndexedForeground(TextColor::DARK_WHITE);
whiteOnBlack.SetIndexedBackground(TextColor::BRIGHT_BLACK);

TextAttribute blackOnDefault{};
blackOnDefault.SetIndexedForeground(TextColor::BRIGHT_BLACK);

TextAttribute defaultOnDefault{};

Log::Comment(L"========== Fill test content ==========");

// As a pwsh one-liner:
//
// "`e[37m`e[102m foo\bar `e[92m`e[100m▶ `e[37mBar `e[90m`e[49m▶ `e[m"
//
// Generally taken from
// https://github.com/microsoft/terminal/issues/8341#issuecomment-731310022,
// but minimized for easier testing.

sm.ProcessString(L"\x1b[37m\x1b[102m" // dark white on bright green
L" foo\\bar ");
sm.ProcessString(L"\x1b[92m\x1b[100m" // bright green on bright black
L"");
sm.ProcessString(L"\x1b[37m" // dark white on bright black
L"Bar ");
sm.ProcessString(L"\x1b[90m\x1b[49m" // bright black on default
L"");
sm.ProcessString(L"\x1b[m\n"); // default on default

auto verifyBuffer = [&](const TextBuffer& tb) {
// If this test fails on character 8, then it's because we didn't emit the space, we just moved ahead.
auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, whiteOnGreen, 9u);
TestUtils::VerifyLineContains(iter0, OutputCellIterator{ greenOnBlack, 2u });
TestUtils::VerifyLineContains(iter0, OutputCellIterator{ whiteOnBlack, 4u });
TestUtils::VerifyLineContains(iter0, OutputCellIterator{ blackOnDefault, 2u });
};

Log::Comment(L"========== Check host buffer ==========");
verifyBuffer(*hostTb);

Log::Comment(L"========== Paint first frame ==========");
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"========== Check terminal buffer ==========");
verifyBuffer(*termTb);
}

void ConptyRoundtripTests::AltBufferResizeCrash()
{
Log::Comment(L"During the review for GH#12719, it was noticed that this "
Expand Down
28 changes: 28 additions & 0 deletions src/host/ut_host/ConptyOutputTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ class ConptyOutputTests
TEST_METHOD(WriteAFewSimpleLines);
TEST_METHOD(InvalidateUntilOneBeforeEnd);
TEST_METHOD(SetConsoleTitleWithControlChars);
TEST_METHOD(IncludeBackgroundColorChangesInFirstFrame);

private:
bool _writeCallback(const char* const pch, const size_t cch);
Expand Down Expand Up @@ -371,6 +372,7 @@ void ConptyOutputTests::SetConsoleTitleWithControlChars()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:control", L"{0x00, 0x0A, 0x1B, 0x80, 0x9B, 0x9C}")
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
END_TEST_METHOD_PROPERTIES()

int control;
Expand Down Expand Up @@ -400,3 +402,29 @@ void ConptyOutputTests::SetConsoleTitleWithControlChars()

VERIFY_SUCCEEDED(renderer.PaintFrame());
}

void ConptyOutputTests::IncludeBackgroundColorChangesInFirstFrame()
{
auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& sm = si.GetStateMachine();

sm.ProcessString(L"\x1b[41mRun 1 \x1b[42mRun 2 \x1b[43mRun 3 \x1b[m");

expectedOutput.push_back("\x1b[2J"); // standard init sequence for the first frame
expectedOutput.push_back("\x1b[m"); // standard init sequence for the first frame
expectedOutput.push_back("\x1b[41m");
expectedOutput.push_back("\x1b[H"); // standard init sequence for the first frame
expectedOutput.push_back("Run 1 ");
expectedOutput.push_back("\x1b[42m");
expectedOutput.push_back("Run 2 ");
expectedOutput.push_back("\x1b[43m");
expectedOutput.push_back("Run 3 ");

// This is also part of the standard init sequence.
expectedOutput.push_back("\x1b[?25h");

VERIFY_SUCCEEDED(renderer.PaintFrame());
}
8 changes: 6 additions & 2 deletions src/inc/TestUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,19 @@ class TerminalCoreUnitTests::TestUtils
auto actualAttrs = actual->TextAttr();
auto expectedAttrs = expected->TextAttr();

auto mismatched = (actualChars != expectedChars || actualAttrs != expectedAttrs);
auto mismatched = ((!expectedChars.empty() && actualChars != expectedChars) || actualAttrs != expectedAttrs);
if (mismatched)
{
WEX::Logging::Log::Comment(WEX::Common::NoThrowString().Format(
L"Character or attribute at index %d was mismatched", charsProcessed));
}

VERIFY_ARE_EQUAL(expectedChars, actualChars);
if (!expectedChars.empty())
{
VERIFY_ARE_EQUAL(expectedChars, actualChars);
}
VERIFY_ARE_EQUAL(expectedAttrs, actualAttrs);

if (mismatched)
{
return false;
Expand Down
11 changes: 7 additions & 4 deletions src/renderer/vt/paint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,13 @@ using namespace Microsoft::Console::Types;
// the lines _wrapped_. It doesn't care to manually break the lines, but if
// we trimmed the spaces off here, we'd print all the "~"s one after another
// on the same line.
const auto removeSpaces = !lineWrapped && (useEraseChar ||
_clearedAllThisFrame ||
(_newBottomLine && printingBottomLine && bgMatched));
const auto cchActual = removeSpaces ? nonSpaceLength : cchLine;
static const TextAttribute defaultAttrs{};
const bool removeSpaces = !lineWrapped && (useEraseChar // we determined earlier that ECH is optimal
|| (_clearedAllThisFrame && _lastTextAttributes == defaultAttrs) // OR we cleared the last frame to the default attributes (specifically)
|| (_newBottomLine && printingBottomLine && bgMatched)); // OR we just scrolled a new line onto the bottom of the screen with the correct attributes
const size_t cchActual = removeSpaces ?
(cchLine - numSpaces) :
cchLine;

const auto columnsActual = removeSpaces ?
(totalWidth - numSpaces) :
Expand Down

0 comments on commit 210a98e

Please sign in to comment.