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

@@ -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,19 +176,20 @@ 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]() {
+            [this, 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())
-                {
-                    const auto lock = t->LockForWriting();
-                    t->UpdatePatternsUnderLock();
-                }
+                // We can't use a weak-ref to `_terminal` here, because it takes significant
+                // dependency on the lifetime of `this` (primarily our _renderer).
+                // We can't use a weak-ref to `this` either, because if we end up being the last holder of a strong-ref
+                // (after promotion to it), we'd drop ControlCore on a background thread, and it's not thread-safe.
+                const auto lock = _terminal->LockForWriting();
+                _terminal->UpdatePatternsUnderLock();
             });
 
         // If you rapidly show/hide Windows Terminal, something about GotFocus()/LostFocus() gets broken.
@@ -227,9 +218,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     ControlCore::~ControlCore()
     {
         Close();
-
-        _renderer.reset();
-        _renderEngine.reset();
     }
 
     void ControlCore::Detach()
@@ -276,16 +264,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 +293,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 +2191,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

@@ -311,63 +311,61 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> updateScrollBar;
         };
 
-        std::atomic<bool> _initializedTerminal{ false };
-        bool _closing{ false };
-
-        TerminalConnection::ITerminalConnection _connection{ nullptr };
-        TerminalConnection::ITerminalConnection::TerminalOutput_revoker _connectionOutputEventRevoker;
-        TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker;
-
-        winrt::com_ptr<ControlSettings> _settings{ nullptr };
-
-        std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };
-        std::wstring _pendingResponses;
-
         // NOTE: _renderEngine must be ordered before _renderer.
-        //
         // As _renderer has a dependency on _renderEngine (through a raw pointer)
         // we must ensure the _renderer is deallocated first.
-        // (C++ class members are destroyed in reverse order.)
         std::unique_ptr<::Microsoft::Console::Render::Atlas::AtlasEngine> _renderEngine{ nullptr };
         std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr };
 
-        ::Search _searcher;
-        bool _snapSearchResultToSelection;
-
-        winrt::handle _lastSwapChainHandle{ nullptr };
+        // Caches responses generated by our VT parser (= improved batching).
+        std::wstring _pendingResponses;
 
+        // Font stuff.
         FontInfoDesired _desiredFont;
         FontInfo _actualFont;
         bool _builtinGlyphs = true;
         bool _colorGlyphs = true;
         CSSLengthPercentage _cellWidth;
         CSSLengthPercentage _cellHeight;
 
-        // storage location for the leading surrogate of a utf-16 surrogate pair
-        std::optional<wchar_t> _leadingSurrogate{ std::nullopt };
-
-        std::optional<til::point> _lastHoveredCell{ std::nullopt };
-        // Track the last hyperlink ID we hovered over
-        uint16_t _lastHoveredId{ 0 };
-
-        bool _isReadOnly{ false };
-
-        std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _lastHoveredInterval{ std::nullopt };
-
-        // These members represent the size of the surface that we should be
-        // rendering to.
+        // Rendering stuff.
+        winrt::handle _lastSwapChainHandle{ nullptr };
+        uint64_t _owningHwnd{ 0 };
         float _panelWidth{ 0 };
         float _panelHeight{ 0 };
         float _compositionScale{ 0 };
 
-        uint64_t _owningHwnd{ 0 };
+        // Audio stuff.
+        MidiAudio _midiAudio;
+        winrt::Windows::System::DispatcherQueueTimer _midiAudioSkipTimer{ nullptr };
 
+        // Other stuff.
         winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr };
-        til::shared_mutex<SharedState> _shared;
-
         til::point _contextMenuBufferPosition{ 0, 0 };
-
         Windows::Foundation::Collections::IVector<hstring> _cachedQuickFixes{ nullptr };
+        ::Search _searcher;
+        std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _lastHoveredInterval;
+        std::optional<wchar_t> _leadingSurrogate;
+        std::optional<til::point> _lastHoveredCell;
+        uint16_t _lastHoveredId{ 0 };
+        std::atomic<bool> _initializedTerminal{ false };
+        bool _isReadOnly{ false };
+        bool _closing{ false };
+
+        // ----------------------------------------------------------------------------------------
+        // These are ordered last to ensure they're destroyed first.
+        // This ensures that their respective contents stops taking dependency on the above.
+        // This includes all the smart-pointer stuff, since they're "safe" to destroy.
+        // ----------------------------------------------------------------------------------------
+
+        std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };
+        winrt::com_ptr<ControlSettings> _settings{ nullptr };
+        TerminalConnection::ITerminalConnection _connection{ nullptr };
+        TerminalConnection::ITerminalConnection::TerminalOutput_revoker _connectionOutputEventRevoker;
+        TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker;
+        // NOTE: `_shared->outputIdle` is the most important offender here by far.
+        // It takes dependency on the raw `this` pointer (necessarily), runs async, and runs often.
+        til::shared_mutex<SharedState> _shared;
 
         void _setupDispatcherAndCallbacks();
 
@@ -396,12 +394,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         void _terminalWindowSizeChanged(int32_t width, int32_t height);
 
         safe_void_coroutine _terminalCompletionsChanged(std::wstring_view menuJson, unsigned int replaceLength);
-
 #pragma endregion
 
-        MidiAudio _midiAudio;
-        winrt::Windows::System::DispatcherQueueTimer _midiAudioSkipTimer{ nullptr };
-
 #pragma region RendererCallbacks
         void _rendererWarning(const HRESULT hr, wil::zwstring_view parameter);
         safe_void_coroutine _renderEngineSwapChainChanged(const HANDLE handle);
@@ -412,6 +406,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;