Skip to content

Commit

Permalink
Fix TerminalControl crash on exit
Browse files Browse the repository at this point in the history 
ControlCore's _renderer (IRenderTarget) is allocated as std::unique_ptr,
but is given to Terminal::CreateFromSettings as a reference.
ControlCore::Close deallocates the `_renderer`, but if ThrottledFuncs
are still scheduled to call ControlCore::UpdatePatternLocations
it'll cause Terminal::UpdatePatterns to be called, which in turn ends up
accessing the deallocated IRenderTarget reference and lead to a crash.

A proper solution with shared pointers is nontrivial and should be
attempted at a later point in time. This solution moves the teardown of
the _renderer into ControlCore::~ControlCore, where we can be certain
that no further strong references are held by ThrottledFuncs.
  • Loading branch information
@lhecker
lhecker committed May 4, 2021
1 parent 2559ed6 commit 564743a
Show file tree
Hide file tree
Showing 10 changed files with 92 additions and 33 deletions.
62 changes: 61 additions & 1 deletion .vscode/settings.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,67 @@
"xtree": "cpp",
"xutility": "cpp",
"span": "cpp",
"string_span": "cpp"
"string_span": "cpp",
"algorithm": "cpp",
"atomic": "cpp",
"bit": "cpp",
"cctype": "cpp",
"charconv": "cpp",
"chrono": "cpp",
"clocale": "cpp",
"cmath": "cpp",
"compare": "cpp",
"complex": "cpp",
"concepts": "cpp",
"condition_variable": "cpp",
"cstdarg": "cpp",
"cstddef": "cpp",
"cstdint": "cpp",
"cstdio": "cpp",
"cstdlib": "cpp",
"cstring": "cpp",
"ctime": "cpp",
"cwchar": "cpp",
"cwctype": "cpp",
"exception": "cpp",
"filesystem": "cpp",
"fstream": "cpp",
"functional": "cpp",
"iomanip": "cpp",
"ios": "cpp",
"iosfwd": "cpp",
"iostream": "cpp",
"iterator": "cpp",
"limits": "cpp",
"locale": "cpp",
"map": "cpp",
"memory_resource": "cpp",
"mutex": "cpp",
"new": "cpp",
"numeric": "cpp",
"optional": "cpp",
"ostream": "cpp",
"ratio": "cpp",
"set": "cpp",
"shared_mutex": "cpp",
"sstream": "cpp",
"stdexcept": "cpp",
"stop_token": "cpp",
"streambuf": "cpp",
"string": "cpp",
"system_error": "cpp",
"thread": "cpp",
"typeinfo": "cpp",
"unordered_map": "cpp",
"unordered_set": "cpp",
"xfacet": "cpp",
"xiosbase": "cpp",
"xlocale": "cpp",
"xlocbuf": "cpp",
"xlocinfo": "cpp",
"xmemory": "cpp",
"xstddef": "cpp",
"xtr1common": "cpp"
},
"files.exclude": {
"**/bin/**": true,
Expand Down
47 changes: 23 additions & 24 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,29 @@ namespace winrt::Microsoft::Terminal::Control::implementation
ControlCore::~ControlCore()
{
Close();

// Before destroying this instance we must ensure that we destroy the _renderer
// before the _renderEngine, as well as calling _renderer->TriggerTeardown().
// _renderEngine will be destroyed naturally after this ~destructor() returns.

decltype(_renderer) renderer;
{
// 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();

_renderer.swap(renderer);
}

if (renderer)
{
renderer->TriggerTeardown();
}
}

bool ControlCore::Initialize(const double actualWidth,
Expand Down Expand Up @@ -1164,30 +1187,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// don't really care to wait for the connection to be completely
// 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) })
{
localRenderer->TriggerTeardown();
// renderer is destroyed
}
// renderEngine is destroyed
}

// we don't destroy _terminal here; it now has the same lifetime as the
// control.
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ namespace
virtual void TriggerRedraw(const COORD* const){};
virtual void TriggerRedrawCursor(const COORD* const){};
virtual void TriggerRedrawAll(){};
virtual void TriggerTeardown(){};
virtual void TriggerTeardown() noexcept {};
virtual void TriggerSelection(){};
virtual void TriggerScroll(){};
virtual void TriggerScroll(const COORD* const delta)
Expand Down
2 changes: 1 addition & 1 deletion src/host/ScreenBufferRenderTarget.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ void ScreenBufferRenderTarget::TriggerRedrawAll()
}
}

void ScreenBufferRenderTarget::TriggerTeardown()
void ScreenBufferRenderTarget::TriggerTeardown() noexcept
{
auto* pRenderer = ServiceLocator::LocateGlobals().pRender;
const auto* pActive = &ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer().GetActiveBuffer();
Expand Down
2 changes: 1 addition & 1 deletion src/host/ScreenBufferRenderTarget.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class ScreenBufferRenderTarget final : public Microsoft::Console::Render::IRende
void TriggerRedraw(const COORD* const pcoord) override;
void TriggerRedrawCursor(const COORD* const pcoord) override;
void TriggerRedrawAll() override;
void TriggerTeardown() override;
void TriggerTeardown() noexcept override;
void TriggerSelection() override;
void TriggerScroll() override;
void TriggerScroll(const COORD* const pcoordDelta) override;
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/base/renderer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ void Renderer::TriggerRedrawAll()
// - <none>
// Return Value:
// - <none>
void Renderer::TriggerTeardown()
void Renderer::TriggerTeardown() noexcept
{
// We need to shut down the paint thread on teardown.
_pThread->WaitForPaintCompletionAndDisable(INFINITE);
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/base/renderer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ namespace Microsoft::Console::Render
void TriggerRedraw(const COORD* const pcoord) override;
void TriggerRedrawCursor(const COORD* const pcoord) override;
void TriggerRedrawAll() override;
void TriggerTeardown() override;
void TriggerTeardown() noexcept override;

void TriggerSelection() override;
void TriggerScroll() override;
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/inc/DummyRenderTarget.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class DummyRenderTarget final : public Microsoft::Console::Render::IRenderTarget
void TriggerRedraw(const COORD* const /*pcoord*/) override {}
void TriggerRedrawCursor(const COORD* const /*pcoord*/) override {}
void TriggerRedrawAll() override {}
void TriggerTeardown() override {}
void TriggerTeardown() noexcept override {}
void TriggerSelection() override {}
void TriggerScroll() override {}
void TriggerScroll(const COORD* const /*pcoordDelta*/) override {}
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/inc/IRenderTarget.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ namespace Microsoft::Console::Render
virtual void TriggerRedrawCursor(const COORD* const pcoord) = 0;

virtual void TriggerRedrawAll() = 0;
virtual void TriggerTeardown() = 0;
virtual void TriggerTeardown() noexcept = 0;

virtual void TriggerSelection() = 0;
virtual void TriggerScroll() = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/inc/IRenderer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace Microsoft::Console::Render
virtual void TriggerRedrawCursor(const COORD* const pcoord) = 0;

virtual void TriggerRedrawAll() = 0;
virtual void TriggerTeardown() = 0;
virtual void TriggerTeardown() noexcept = 0;

virtual void TriggerSelection() = 0;
virtual void TriggerScroll() = 0;
Expand Down

1 comment on commit 564743a

@github-actions
Copy link

Choose a reason for hiding this comment

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

Misspellings found, please review:

  • charconv
  • cstdint
  • iosfwd
  • streambuf
  • xfacet
  • xiosbase
  • xlocale
  • xlocbuf
  • xlocinfo
  • xmemory
  • xstddef
  • xtr
To accept these changes, run the following commands from this repository on this branch 
pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aef aspnet azurewebsites boostorg BSODs Cac COINIT dahall DEFCON developercommunity fde fea fmtlib hotkeys HPCON isocpp llvm mintty msvcrtd Nc NVDA pinam QOL remoting UNINIT Unk unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/564743ac86dd7417df7c25592889a6ce5b654385.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"cac charconv coinit cstdint hpcon iosfwd LLVM MSVCRTD nc Remoting streambuf uninit unk xfacet xiosbase xlocale xlocbuf xlocinfo xmemory xstddef xtr "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

Please sign in to comment.