From ac265aab99d5f79345074fbff66c54a816120de1 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 4 May 2021 23:17:37 +0200 Subject: [PATCH] Fix TerminalControl crash on exit (#10031) ## 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. --- .github/actions/spelling/dictionary/apis.txt | 23 ++++--- .github/actions/spelling/expect/expect.txt | 47 +++++--------- .github/actions/spelling/expect/web.txt | 1 - .vscode/settings.json | 62 ++++++++++++++++++- src/cascadia/TerminalControl/ControlCore.cpp | 47 +++++++------- .../UnitTests_TerminalCore/ScrollTest.cpp | 2 +- src/host/ScreenBufferRenderTarget.cpp | 2 +- src/host/ScreenBufferRenderTarget.hpp | 2 +- src/renderer/base/renderer.cpp | 2 +- src/renderer/base/renderer.hpp | 2 +- src/renderer/inc/DummyRenderTarget.hpp | 2 +- src/renderer/inc/IRenderTarget.hpp | 2 +- src/renderer/inc/IRenderer.hpp | 2 +- 13 files changed, 121 insertions(+), 75 deletions(-) diff --git a/.github/actions/spelling/dictionary/apis.txt b/.github/actions/spelling/dictionary/apis.txt index dc413e88bb..b455ec211b 100644 --- a/.github/actions/spelling/dictionary/apis.txt +++ b/.github/actions/spelling/dictionary/apis.txt @@ -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 diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 85792449ea..10dfff7017 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -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 diff --git a/.github/actions/spelling/expect/web.txt b/.github/actions/spelling/expect/web.txt index b68cd0a783..ec76321ef4 100644 --- a/.github/actions/spelling/expect/web.txt +++ b/.github/actions/spelling/expect/web.txt @@ -15,5 +15,4 @@ winui appshellintegration cppreference gfycat -what3words Guake diff --git a/.vscode/settings.json b/.vscode/settings.json index 1b6028e5ae..2b746e3d19 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -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, diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 46775a7031..966f4f5e32 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -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. } } diff --git a/src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp b/src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp index ee5e92948f..7d376234ce 100644 --- a/src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp @@ -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) diff --git a/src/host/ScreenBufferRenderTarget.cpp b/src/host/ScreenBufferRenderTarget.cpp index 72631a8ee9..87f98ce29a 100644 --- a/src/host/ScreenBufferRenderTarget.cpp +++ b/src/host/ScreenBufferRenderTarget.cpp @@ -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(); diff --git a/src/host/ScreenBufferRenderTarget.hpp b/src/host/ScreenBufferRenderTarget.hpp index d90f80c0fc..09a189691c 100644 --- a/src/host/ScreenBufferRenderTarget.hpp +++ b/src/host/ScreenBufferRenderTarget.hpp @@ -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; diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index 862821db4d..b53bc6eadf 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -319,7 +319,7 @@ void Renderer::TriggerRedrawAll() // - // Return Value: // - -void Renderer::TriggerTeardown() +void Renderer::TriggerTeardown() noexcept { // We need to shut down the paint thread on teardown. _pThread->WaitForPaintCompletionAndDisable(INFINITE); diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp index ba9963e4eb..6f8f6d7a8c 100644 --- a/src/renderer/base/renderer.hpp +++ b/src/renderer/base/renderer.hpp @@ -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; diff --git a/src/renderer/inc/DummyRenderTarget.hpp b/src/renderer/inc/DummyRenderTarget.hpp index 1fca6153c8..e00a22a164 100644 --- a/src/renderer/inc/DummyRenderTarget.hpp +++ b/src/renderer/inc/DummyRenderTarget.hpp @@ -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 {} diff --git a/src/renderer/inc/IRenderTarget.hpp b/src/renderer/inc/IRenderTarget.hpp index 54931cf2ce..f12f243e1a 100644 --- a/src/renderer/inc/IRenderTarget.hpp +++ b/src/renderer/inc/IRenderTarget.hpp @@ -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; diff --git a/src/renderer/inc/IRenderer.hpp b/src/renderer/inc/IRenderer.hpp index ce9dbec18a..79058c5ae5 100644 --- a/src/renderer/inc/IRenderer.hpp +++ b/src/renderer/inc/IRenderer.hpp @@ -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;