Fix crash in terminal when tab closed while data is being output (#8982)

If there is data being output when a tab is closed, that can sometimes
result in the application crashing, because the renderer may still be in
use at the time is it destroyed. This PR attempts to prevent that from
happening by adding a lock in the `TermControl::Close` method.

What we're trying to prevent is the connection thread still being
active, and potentially accessing the renderer, after it has been
destroyed. So by acquiring the terminal lock in `TermControl::Close`,
after we've stopped accepting new output, we can be sure that the
connection thread is no longer active (it holds the lock while it is
processing output). So once we've acquired and released the lock, it
should be safe to tear down and destroy the renderer.

## Validation Steps Performed

While this crash is difficult to reproduce in general usage, it occurred
quite frequently when testing my `DECPS` implementation (there is some
tricky thread synchronisation, which seems more likely to trigger the
issue). With this patch applied, though, those crashes have stopped
occurring.

I've also stepped through the shutdown code in the debugger, manually
freezing threads to get them aligned in the right way to trigger the
crash (as explained in issue #8734). Again with the patch applied, I can
no longer get the crash to occur.

Closes #8734
This commit is contained in:
James Holderness 2021-02-01 21:28:47 +00:00 committed by GitHub
parent 20152d9756
commit 40ebe5ab54
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -2598,6 +2598,17 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// closed. We can just do it whenever.
_AsyncCloseConnection();
{
// GH#8734:
// We lock the terminal here to make sure it isn't still being
// used in the connection thread before we destroy the renderer.
// However, we must unlock it again prior to triggering the
// teardown, to avoid the render thread being deadlocked. The
// renderer may be waiting to acquire the terminal lock, while
// we're waiting for the renderer to finish.
auto lock = _terminal->LockForWriting();
}
if (auto localRenderEngine{ std::exchange(_renderEngine, nullptr) })
{
if (auto localRenderer{ std::exchange(_renderer, nullptr) })