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

vt: make sure to Flush the entire parsed sequence #4870

Merged
merged 2 commits into from
Mar 11, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/terminal/parser/stateMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ StateMachine::StateMachine(std::unique_ptr<IStateMachineEngine> engine) :
_intermediates{},
_parameters{},
_oscString{},
_cachedSequence{ std::nullopt },
_processingIndividually(false)
{
_ActionClear();
Expand Down Expand Up @@ -533,6 +534,7 @@ void StateMachine::_ActionSs3Dispatch(const wchar_t wch)
void StateMachine::_EnterGround() noexcept
{
_state = VTStates::Ground;
_cachedSequence.reset(); // entering ground means we've completed the pending sequence
_trace.TraceStateChange(L"Ground");
}

Expand Down Expand Up @@ -1230,7 +1232,13 @@ bool StateMachine::FlushToTerminal()
// that pwchCurr was processed.
// However, if we're here, then the processing of pwchChar triggered the
// engine to request the entire sequence get passed through, including pwchCurr.
return _engine->ActionPassThroughString(_run);
bool succeeded = true;
if (succeeded && _cachedSequence.has_value())
{
succeeded = _engine->ActionPassThroughString(*_cachedSequence);
_cachedSequence.reset();
}
return succeeded && _engine->ActionPassThroughString(_run);
Copy link
Member

Choose a reason for hiding this comment

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

You're being tricky here and it's taking more than instantaneous to figure out what you're doing. Can you please add more comments to explain your trickery?

(Also that first succeeded test is weird given it's always true...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the first succeeded test is in case anybody ever adds another thing that might fail in this flow ;P but yeah, fair. I'll comment it

}

// Routine Description:
Expand Down Expand Up @@ -1365,6 +1373,13 @@ void StateMachine::ProcessString(const std::wstring_view string)
// after dispatching the characters
_EnterGround();
}
else
{
// If the engine doesn't require flushing at the end of the string, we
// want to cache the partial sequence in case we have to flush the whole
// thing to the terminal later.
_cachedSequence = _cachedSequence.value_or(std::wstring{}) + std::wstring{ _run };
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/terminal/parser/stateMachine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ namespace Microsoft::Console::VirtualTerminal
std::wstring _oscString;
size_t _oscParameter;

std::optional<std::wstring> _cachedSequence;

// This is tracked per state machine instance so that separate calls to Process*
// can start and finish a sequence.
bool _processingIndividually;
Expand Down
68 changes: 65 additions & 3 deletions src/terminal/parser/ut_parser/StateMachineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,25 @@ using namespace Microsoft::Console::VirtualTerminal;
class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStateMachineEngine
{
public:
void ResetTestState()
{
printed.clear();
passedThrough.clear();
csiParams.reset();
}

bool ActionExecute(const wchar_t /* wch */) override { return true; };
bool ActionExecuteFromEscape(const wchar_t /* wch */) override { return true; };
bool ActionPrint(const wchar_t /* wch */) override { return true; };
bool ActionPrintString(const std::wstring_view string) override
{
printed = string;
printed += string;
return true;
};

bool ActionPassThroughString(const std::wstring_view string) override
{
passedThrough = string;
passedThrough += string;
return true;
};

Expand All @@ -52,7 +59,15 @@ class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStat

bool ActionOscDispatch(const wchar_t /* wch */,
const size_t /* parameter */,
const std::wstring_view /* string */) override { return true; };
const std::wstring_view /* string */) override
{
if (pfnFlushToTerminal)
{
pfnFlushToTerminal();
return true;
}
return true;
};

bool ActionSs3Dispatch(const wchar_t /* wch */,
const std::basic_string_view<size_t> /* parameters */) override { return true; };
Expand Down Expand Up @@ -111,6 +126,7 @@ class Microsoft::Console::VirtualTerminal::StateMachineTest
TEST_METHOD(PassThroughUnhandled);
TEST_METHOD(RunStorageBeforeEscape);
TEST_METHOD(BulkTextPrint);
TEST_METHOD(PassThroughUnhandledSplitAcrossWrites);
};

void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother()
Expand Down Expand Up @@ -182,3 +198,49 @@ void StateMachineTest::BulkTextPrint()
// Then ensure the entire buffered run was printed all at once back to us.
VERIFY_ARE_EQUAL(String(L"12345 Hello World"), String(engine.printed.c_str()));
}

void StateMachineTest::PassThroughUnhandledSplitAcrossWrites()
{
auto enginePtr{ std::make_unique<TestStateMachineEngine>() };
// this dance is required because StateMachine presumes to take ownership of its engine.
auto& engine{ *enginePtr.get() };
StateMachine machine{ std::move(enginePtr) };

// Hook up the passthrough function.
engine.pfnFlushToTerminal = std::bind(&StateMachine::FlushToTerminal, &machine);

// Broken in two pieces (test case from GH#3081)
machine.ProcessString(L"\x1b[?12");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"34h");
VERIFY_ARE_EQUAL(L"\x1b[?1234h", engine.passedThrough); // whole sequence out, no other output
VERIFY_ARE_EQUAL(L"", engine.printed);

engine.ResetTestState();

// Three pieces
machine.ProcessString(L"\x1b[?2");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"34");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"5h");
VERIFY_ARE_EQUAL(L"\x1b[?2345h", engine.passedThrough); // whole sequence out, no other output
VERIFY_ARE_EQUAL(L"", engine.printed);

engine.ResetTestState();

// Split during OSC terminator (test case from GH#3080)
machine.ProcessString(L"\x1b]99;foo\x1b");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"\\");
VERIFY_ARE_EQUAL(L"\x1b]99;foo\x1b\\", engine.passedThrough);
VERIFY_ARE_EQUAL(L"", engine.printed);
}