Fix a shutdown race condition in ControlCore

src/cascadia/TerminalApp/DebugTapConnection.cpp

@@ -60,9 +60,12 @@ namespace winrt::Microsoft::TerminalApp::implementation
 
     DebugTapConnection::DebugTapConnection(ITerminalConnection wrappedConnection)
     {
-        _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { this, &DebugTapConnection::_OutputHandler });
-        _stateChangedRevoker = wrappedConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*e*/) {
-            StateChanged.raise(*this, nullptr);
+        _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { get_weak(), &DebugTapConnection::_OutputHandler });
+        _stateChangedRevoker = wrappedConnection.StateChanged(winrt::auto_revoke, [weak = get_weak()](auto&& /*s*/, auto&& /*e*/) {
+            if (const auto self = weak.get())
+            {
+                self->StateChanged.raise(*self, nullptr);
+            }
         });
         _wrappedConnection = wrappedConnection;
     }

src/cascadia/TerminalControl/ControlCore.cpp

@@ -90,7 +90,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }();
 
         _settings = winrt::make_self<implementation::ControlSettings>(settings, unfocusedAppearance);
-        _terminal = std::make_shared<::Microsoft::Terminal::Core::Terminal>();
+        _terminal = std::make_unique<::Microsoft::Terminal::Core::Terminal>();
         const auto lock = _terminal->LockForWriting();
 
         _setupDispatcherAndCallbacks();
@@ -142,23 +142,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // If we wait, a screen reader may try to get the AutomationPeer (aka the UIA Engine), and we won't be able to attach
         // the UIA Engine to the renderer. This prevents us from signaling changes to the cursor or buffer.
         {
-            // First create the render thread.
-            // Then stash a local pointer to the render thread so we can initialize it and enable it
-            // to paint itself *after* we hand off its ownership to the renderer.
-            // We split up construction and initialization of the render thread object this way
-            // because the renderer and render thread have circular references to each other.
-            auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>();
-            auto* const localPointerToThread = renderThread.get();
-
             // Now create the renderer and initialize the render thread.
             const auto& renderSettings = _terminal->GetRenderSettings();
-            _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get(), nullptr, 0, std::move(renderThread));
+            _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get());
 
             _renderer->SetBackgroundColorChangedCallback([this]() { _rendererBackgroundColorChanged(); });
             _renderer->SetFrameColorChangedCallback([this]() { _rendererTabColorChanged(); });
             _renderer->SetRendererEnteredErrorStateCallback([this]() { RendererEnteredErrorState.raise(nullptr, nullptr); });
-
-            THROW_IF_FAILED(localPointerToThread->Initialize(_renderer.get()));
         }
 
         UpdateSettings(settings, unfocusedAppearance);
@@ -186,16 +176,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // thread is a workaround for us to hit GH#12607 less often.
         shared->outputIdle = std::make_unique<til::debounced_func_trailing<>>(
             std::chrono::milliseconds{ 100 },
-            [weakTerminal = std::weak_ptr{ _terminal }, weakThis = get_weak(), dispatcher = _dispatcher]() {
+            [weakThis = get_weak(), dispatcher = _dispatcher]() {
                 dispatcher.TryEnqueue(DispatcherQueuePriority::Normal, [weakThis]() {
                     if (const auto self = weakThis.get(); self && !self->_IsClosing())
                     {
                         self->OutputIdle.raise(*self, nullptr);
                     }
                 });
 
-                if (const auto t = weakTerminal.lock())
+                if (const auto self = weakThis.get())
                 {
+                    const auto t = self->_terminal.get();
                     const auto lock = t->LockForWriting();
                     t->UpdatePatternsUnderLock();
                 }
@@ -276,16 +267,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         auto oldState = ConnectionState(); // rely on ControlCore's automatic null handling
         // revoke ALL old handlers immediately
 
+        // TODO: This manual event revoking doesn't make much sense.
+        // We could just drop the old connection. Why have all that Close() stuff?
+        // It also shouldn't need to be exposed to the outside. I suspect we can only
+        // improve this though, once drag/drop of tabs doesn't use "startup actions" anymore.
         _connectionOutputEventRevoker.revoke();
         _connectionStateChangedRevoker.revoke();
 
         _connection = newConnection;
         if (_connection)
         {
             // Subscribe to the connection's disconnected event and call our connection closed handlers.
-            _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) {
-                ConnectionStateChanged.raise(*this, nullptr);
-            });
+            _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, { get_weak(), &ControlCore::_connectionStateChangedHandler });
 
             // Get our current size in rows/cols, and hook them up to
             // this connection too.
@@ -303,8 +296,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                 conpty.ReparentWindow(_owningHwnd);
             }
 
-            // This event is explicitly revoked in the destructor: does not need weak_ref
-            _connectionOutputEventRevoker = _connection.TerminalOutput(winrt::auto_revoke, { this, &ControlCore::_connectionOutputHandler });
+            _connectionOutputEventRevoker = _connection.TerminalOutput(winrt::auto_revoke, { get_weak(), &ControlCore::_connectionOutputHandler });
         }
 
         // Fire off a connection state changed notification, to let our hosting
@@ -2202,6 +2194,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
     }
 
+    void ControlCore::_connectionStateChangedHandler(const TerminalConnection::ITerminalConnection&, const Windows::Foundation::IInspectable&)
+    {
+        ConnectionStateChanged.raise(*this, nullptr);
+    }
+
     ::Microsoft::Console::Render::Renderer* ControlCore::GetRenderer() const noexcept
     {
         return _renderer.get();

src/cascadia/TerminalControl/ControlCore.h

@@ -320,7 +320,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         winrt::com_ptr<ControlSettings> _settings{ nullptr };
 
-        std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };
+        std::unique_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };
         std::wstring _pendingResponses;
 
         // NOTE: _renderEngine must be ordered before _renderer.
@@ -412,6 +412,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         void _raiseReadOnlyWarning();
         void _updateAntiAliasingMode();
         void _connectionOutputHandler(const hstring& hstr);
+        void _connectionStateChangedHandler(const TerminalConnection::ITerminalConnection&, const Windows::Foundation::IInspectable&);
         void _updateHoveredCell(const std::optional<til::point> terminalPosition);
         void _setOpacity(const float opacity, const bool focused = true);
 

src/cascadia/TerminalControl/HwndTerminal.cpp

@@ -206,14 +206,10 @@ HRESULT HwndTerminal::Initialize()
     _terminal = std::make_unique<::Microsoft::Terminal::Core::Terminal>();
     const auto lock = _terminal->LockForWriting();
 
-    auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>();
-    auto* const localPointerToThread = renderThread.get();
     auto& renderSettings = _terminal->GetRenderSettings();
     renderSettings.SetColorTableEntry(TextColor::DEFAULT_BACKGROUND, RGB(12, 12, 12));
     renderSettings.SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, RGB(204, 204, 204));
-    _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get(), nullptr, 0, std::move(renderThread));
-    RETURN_HR_IF_NULL(E_POINTER, localPointerToThread);
-    RETURN_IF_FAILED(localPointerToThread->Initialize(_renderer.get()));
+    _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get());
 
     auto engine = std::make_unique<::Microsoft::Console::Render::AtlasEngine>();
     RETURN_IF_FAILED(engine->SetHwnd(_hwnd.get()));
@@ -234,7 +230,7 @@ HRESULT HwndTerminal::Initialize()
 
     _terminal->Create({ 80, 25 }, 9001, *_renderer);
     _terminal->SetWriteInputCallback([=](std::wstring_view input) noexcept { _WriteTextToConnection(input); });
-    localPointerToThread->EnablePainting();
+    _renderer->EnablePainting();
 
     _multiClickTime = std::chrono::milliseconds{ GetDoubleClickTime() };
 

src/host/srvinit.cpp

@@ -842,16 +842,7 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand,
     {
         if (!gci.IsInVtIoMode())
         {
-            auto renderThread = std::make_unique<RenderThread>();
-            // stash a local pointer to the thread here -
-            // We're going to give ownership of the thread to the Renderer,
-            //      but the thread also need to be told who its renderer is,
-            //      and we can't do that until the renderer is constructed.
-            auto* const localPointerToThread = renderThread.get();
-
-            g.pRender = new Renderer(gci.GetRenderSettings(), &gci.renderData, nullptr, 0, std::move(renderThread));
-
-            THROW_IF_FAILED(localPointerToThread->Initialize(g.pRender));
+            g.pRender = new Renderer(gci.GetRenderSettings(), &gci.renderData);
 
             // Set up the renderer to be used to calculate the width of a glyph,
             //      should we be unable to figure out its width another way.

src/renderer/base/renderer.cpp

@@ -25,22 +25,13 @@ static constexpr auto renderBackoffBaseTimeMilliseconds{ 150 };
 // - Creates a new renderer controller for a console.
 // Arguments:
 // - pData - The interface to console data structures required for rendering
-// - pEngine - The output engine for targeting each rendering frame
 // Return Value:
 // - An instance of a Renderer.
-Renderer::Renderer(const RenderSettings& renderSettings,
-                   IRenderData* pData,
-                   _In_reads_(cEngines) IRenderEngine** const rgpEngines,
-                   const size_t cEngines,
-                   std::unique_ptr<RenderThread> thread) :
+Renderer::Renderer(const RenderSettings& renderSettings, IRenderData* pData) :
     _renderSettings(renderSettings),
-    _pData(pData),
-    _pThread{ std::move(thread) }
+    _pData(pData)
 {
-    for (size_t i = 0; i < cEngines; i++)
-    {
-        AddRenderEngine(rgpEngines[i]);
-    }
+    THROW_IF_FAILED(_pThread->Initialize(this));
 }
 
 // Routine Description:

src/renderer/base/renderer.hpp

@@ -28,11 +28,7 @@ namespace Microsoft::Console::Render
     class Renderer
     {
     public:
-        Renderer(const RenderSettings& renderSettings,
-                 IRenderData* pData,
-                 _In_reads_(cEngines) IRenderEngine** const pEngine,
-                 const size_t cEngines,
-                 std::unique_ptr<RenderThread> thread);
+        Renderer(const RenderSettings& renderSettings, IRenderData* pData);
 
         ~Renderer();
 
@@ -121,7 +117,7 @@ namespace Microsoft::Console::Render
         const RenderSettings& _renderSettings;
         std::array<IRenderEngine*, 2> _engines{};
         IRenderData* _pData = nullptr; // Non-ownership pointer
-        std::unique_ptr<RenderThread> _pThread;
+        std::unique_ptr<RenderThread> _pThread = std::make_unique<RenderThread>();
         static constexpr size_t _firstSoftFontChar = 0xEF20;
         size_t _lastSoftFontChar = 0;
         uint16_t _hyperlinkHoveredId = 0;