From f470f49962be93d20aafdd5faa51e10c28caae5d Mon Sep 17 00:00:00 2001
From: Leonard Hecker <lhecker@microsoft.com>
Date: Thu, 6 May 2021 02:45:45 +0200
Subject: [PATCH] Fix crash on exit introduced in ac265aa

ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer,
which TermControl owns. We must ensure that we first destroy the
ControlCore before the UiaEngine instance (both owned by TermControl).
Otherwise a deallocated IRenderEngine is accessed when
ControlCore calls Renderer::TriggerTeardown.
---
 src/cascadia/TerminalControl/ControlCore.cpp | 22 ++------------------
 src/cascadia/TerminalControl/ControlCore.h   |  7 ++++++-
 src/cascadia/TerminalControl/TermControl.h   | 18 +++++++++-------
 3 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index 966f4f5e32c..f67d74a5852 100644
--- a/src/cascadia/TerminalControl/ControlCore.cpp
+++ b/src/cascadia/TerminalControl/ControlCore.cpp
@@ -101,27 +101,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     {
         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)
+        if (_renderer)
         {
-            renderer->TriggerTeardown();
+            _renderer->TriggerTeardown();
         }
     }
 
diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h
index 8ab58a2243d..1a6f0ca2914 100644
--- a/src/cascadia/TerminalControl/ControlCore.h
+++ b/src/cascadia/TerminalControl/ControlCore.h
@@ -172,8 +172,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         std::unique_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };
 
-        std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr };
+        // 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::DxEngine> _renderEngine{ nullptr };
+        std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr };
 
         IControlSettings _settings{ nullptr };
 
diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h
index 54fd49c37f2..9e7cd29349c 100644
--- a/src/cascadia/TerminalControl/TermControl.h
+++ b/src/cascadia/TerminalControl/TermControl.h
@@ -130,22 +130,26 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     private:
         friend struct TermControlT<TermControl>; // friend our parent so it can bind private event handlers
 
+        // NOTE: _uiaEngine must be ordered before _core.
+        //
+        // ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which we own.
+        // We must ensure that we first destroy the ControlCore before the UiaEngine instance
+        // in order to safely resolve this unsafe pointer dependency. Otherwise a deallocated
+        // IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown.
+        // (C++ class members are destroyed in reverse order.)
+        std::unique_ptr<::Microsoft::Console::Render::UiaEngine> _uiaEngine;
+
         winrt::com_ptr<ControlCore> _core;
         winrt::com_ptr<ControlInteractivity> _interactivity;
-
-        bool _initializedTerminal;
-
         winrt::com_ptr<SearchBoxControl> _searchBox;
 
-        std::unique_ptr<::Microsoft::Console::Render::UiaEngine> _uiaEngine;
-
         IControlSettings _settings;
-        bool _focused;
         std::atomic<bool> _closing;
+        bool _focused;
+        bool _initializedTerminal;
 
         std::shared_ptr<ThrottledFunc<>> _tsfTryRedrawCanvas;
         std::shared_ptr<ThrottledFunc<>> _updatePatternLocations;
-
         std::shared_ptr<ThrottledFunc<>> _playWarningBell;
 
         struct ScrollBarUpdate