maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix TerminalControl crash on exit (#10031)

Browse source 
## Summary of the Pull Request

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.

## PR Checklist
* [x] Closes #9910
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

The crash is a race condition and inherently hard to reproduce.
During validation this PR didn't appear to introduce new crashes.
This commit is contained in:
Leonard Hecker 2021-05-04 23:17:37 +02:00 committed by GitHub
parent 2559ed6efa
commit ac265aab99
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 121 additions and 75 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
.github/actions/spelling/dictionary/apis.txt vendored
View file

@ -4,11 +4,13 @@ alignof
bitfield
bitfields
BUILDNUMBER
charconv
CLASSNOTAVAILABLE
cmdletbinding
colspan
COLORPROPERTY
colspan
COMDLG
cstdint
CXICON
CYICON
D2DERR_SHADER_COMPILE_FAILED
@ -52,6 +54,7 @@ IFile
IInheritable
IMap
IObject
iosfwd
IPackage
IPeasant
IStorage
@ -67,7 +70,6 @@ llu
localtime
lround
LSHIFT
MULTIPLEUSE
msappx
MULTIPLEUSE
NCHITTEST
@ -91,7 +93,6 @@ PAGESCROLL
PICKFOLDERS
pmr
REGCLS
REGCLS
RETURNCMD
rfind
roundf
@ -100,23 +101,22 @@ rx
schandle
semver
serializer
SHELLEXECUTEINFOW
shobjidl
SINGLEUSE
SHOWMINIMIZED
SINGLEUSE
SIZENS
smoothstep
GETDESKWALLPAPER
SHELLEXECUTEINFOW
snprintf
spsc
sregex
STDCPP
strchr
STDMETHOD
strchr
streambuf
Stubless
Subheader
Subpage
UPDATEINIFILE
syscall
TBPF
THEMECHANGED
@ -135,12 +135,19 @@ wsregex
wwinmain
XDocument
XElement
xfacet
xhash
xiosbase
xlocale
xlocbuf
xlocinfo
xlocmes
xlocmon
xlocnum
xloctime
xmemory
XParse
xstddef
xstring
xtree
xutility

47
.github/actions/spelling/expect/expect.txt vendored
View file

@ -27,7 +27,6 @@ ADDREF
addressof
ADDSTRING
ADDTOOL
aef
AEnd
AFew
AFill
@ -98,7 +97,6 @@ ASingle
asm
asmv
asmx
aspnet
aspx
astextplain
AStomps
@ -123,7 +121,6 @@ AVerify
AVI
awch
azuredevopspodcast
azurewebsites
azzle
backend
backgrounded
@ -174,7 +171,6 @@ BODGY
BOLDFONT
BOOLIFY
bools
boostorg
Bopomofo
Borland
BOTTOMLEFT
@ -187,7 +183,6 @@ branchconfig
BRK
Browsable
bsearch
BSODs
bstr
BTNFACE
buf
@ -202,7 +197,7 @@ BValue
byref
bytearray
bytebuffer
Cac
cac
callee
cang
capslock
@ -302,7 +297,7 @@ codepage
codepath
codepoint
codeproject
COINIT
coinit
COLLECTIONURI
colorizing
colororacle
@ -487,7 +482,6 @@ CYSIZEFRAME
CYSMICON
CYVIRTUALSCREEN
CYVSCROLL
dahall
dai
DATABLOCK
DATAVIEW
@ -575,7 +569,6 @@ defaultsettings
DEFAULTTONEAREST
DEFAULTTONULL
DEFAULTTOPRIMARY
DEFCON
defectdefs
DEFERERASE
deff
@ -603,7 +596,6 @@ desktopwindowxamlsource
dest
DESTINATIONNAME
devblogs
developercommunity
devicecode
devicefamily
devops
@ -671,13 +663,13 @@ DUNIT
dup'ed
dvi
dw
dwl
DWLP
dwm
dwmapi
dword
dwrite
dwriteglyphrundescriptionclustermap
dwl
dxgi
dxgidwm
dxinterop
@ -770,10 +762,8 @@ fclose
fcntl
fdc
FDD
fde
fdopen
fdw
fea
fesb
FFDE
FFrom
@ -808,7 +798,6 @@ flyout
fmodern
fmtarg
fmtid
fmtlib
FOLDERID
FONTCHANGE
fontdlg
@ -1013,10 +1002,9 @@ horiz
HORZ
hostable
hostlib
hotkeys
HPA
HPAINTBUFFER
HPCON
hpcon
hpj
hpp
HPR
@ -1172,7 +1160,6 @@ IRenderer
IScheme
ISelection
IShell
isocpp
issuecomment
IState
IStoryboard
@ -1290,7 +1277,7 @@ listptr
listptrsize
lk
lld
llvm
LLVM
llx
LMENU
LMNOP
@ -1435,7 +1422,6 @@ mingw
minimizeall
minkernel
MINMAXINFO
mintty
minwin
minwindef
Mip
@ -1482,7 +1468,7 @@ MSIL
msix
msrc
msvcrt
msvcrtd
MSVCRTD
MSVS
msys
msysgit
@ -1508,7 +1494,7 @@ namestream
nano
natvis
nbsp
Nc
nc
NCCALCSIZE
NCCREATE
NCLBUTTONDOWN
@ -1631,7 +1617,6 @@ numlock
numpad
NUMSCROLL
nupkg
NVDA
NVIDIA
NVR
Nx
@ -1662,11 +1647,11 @@ ONECOREWINDOWS
onehalf
ONLCR
Oo
openconsoleproxy
openbash
opencode
opencon
openconsole
openconsoleproxy
OPENIF
OPENLINK
openps
@ -1784,7 +1769,6 @@ pid
pidl
PIDLIST
pii
pinam
pinvoke
pipename
pipestr
@ -1928,7 +1912,6 @@ pythonw
qi
QJ
qo
QOL
QRSTU
qsort
queryable
@ -1999,7 +1982,7 @@ REGSTR
reingest
Relayout
RELBINPATH
remoting
Remoting
renamer
renderengine
rendersize
@ -2077,8 +2060,8 @@ runut
runxamlformat
rvalue
RVERTICAL
rxvt
RWIN
rxvt
safearray
SAFECAST
safemath
@ -2522,14 +2505,14 @@ unicode
UNICODESTRING
UNICODETEXT
UNICRT
UNINIT
uninit
uninitialize
uninstall
Uniscribe
unittest
unittesting
universaltest
Unk
unk
unknwn
unmark
UNORM
@ -2537,7 +2520,6 @@ unparseable
unpause
Unregister
Unregistering
unte
untests
untextured
untimes
@ -2594,7 +2576,6 @@ Vcount
vcpkg
vcprintf
vcproj
vcrt
vcvarsall
vcxitems
vcxproj
@ -2830,7 +2811,6 @@ wx
wxh
xa
xact
xamarin
xaml
Xamlmeta
xargs
@ -2872,8 +2852,9 @@ XSubstantial
xtended
xterm
XTest
XTPUSHSGR
XTPOPSGR
XTPUSHSGR
xtr
xunit
xutr
xvalue

1
.github/actions/spelling/expect/web.txt vendored
View file

@ -15,5 +15,4 @@ winui
appshellintegration
cppreference
gfycat
what3words
Guake

62
.vscode/settings.json vendored
View file

@ -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,

47
src/cascadia/TerminalControl/ControlCore.cpp
View file

@ -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,
@ -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.
}
}

2
src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp
View file

@ -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)

2
src/host/ScreenBufferRenderTarget.cpp
View file

@ -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();

2
src/host/ScreenBufferRenderTarget.hpp
View file

@ -33,7 +33,7 @@ public:
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;

2
src/renderer/base/renderer.cpp
View file

@ -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);

2
src/renderer/base/renderer.hpp
View file

@ -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;

2
src/renderer/inc/DummyRenderTarget.hpp
View file

@ -25,7 +25,7 @@ public:
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 {}

2
src/renderer/inc/IRenderTarget.hpp
View file

@ -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;

2
src/renderer/inc/IRenderer.hpp
View file

@ -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;