From bfd480b885284807fab932799b64920f16fc486a Mon Sep 17 00:00:00 2001
From: Leonard Hecker <lhecker@microsoft.com>
Date: Mon, 17 Oct 2022 23:14:00 +0200
Subject: [PATCH] Fix circular TermControl reference (#14228)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This regression was introduced in b3c9f01. Since `TermControl` is the XAML
object that owns its scrollbar and the scrollbar's `VisualStateManager`
a strong reference back to the `TermControl` results in a circular reference.

## Validation Steps Performed
* Set a breakpoint on `TermControl::~TermControl()`
* Breakpoint hits on tab close ✅
---
 .../ScrollBarVisualStateManager.cpp           | 43 ++++++++++---------
 .../ScrollBarVisualStateManager.h             | 11 ++---
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/src/cascadia/TerminalControl/ScrollBarVisualStateManager.cpp b/src/cascadia/TerminalControl/ScrollBarVisualStateManager.cpp
index b42a853df80..c0508926d7e 100644
--- a/src/cascadia/TerminalControl/ScrollBarVisualStateManager.cpp
+++ b/src/cascadia/TerminalControl/ScrollBarVisualStateManager.cpp
@@ -5,13 +5,10 @@
 #include "ScrollBarVisualStateManager.h"
 #include "ScrollBarVisualStateManager.g.cpp"
 
+using namespace winrt::Windows::UI::Xaml::Media;
+
 namespace winrt::Microsoft::Terminal::Control::implementation
 {
-    ScrollBarVisualStateManager::ScrollBarVisualStateManager() :
-        _termControl(nullptr)
-    {
-    }
-
     bool ScrollBarVisualStateManager::GoToStateCore(
         winrt::Windows::UI::Xaml::Controls::Control const& control,
         winrt::Windows::UI::Xaml::FrameworkElement const& templateRoot,
@@ -20,33 +17,39 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         winrt::Windows::UI::Xaml::VisualState const& state,
         bool useTransitions)
     {
-        if (!_termControl)
+        if (!_initialized)
         {
-            for (auto parent = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(control);
-                 parent != nullptr;
-                 parent = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(parent))
+            _initialized = true;
+
+            Control::TermControl termControl{ nullptr };
+
+            for (auto parent = VisualTreeHelper::GetParent(control); parent; parent = VisualTreeHelper::GetParent(parent))
             {
-                if (parent.try_as(_termControl))
+                if (parent.try_as(termControl))
                 {
+                    _termControl = winrt::get_self<TermControl>(termControl)->get_weak();
                     break;
                 }
             }
-        }
 
-        WINRT_ASSERT(_termControl);
+            assert(termControl);
+        }
 
-        auto scrollState = _termControl.Settings().ScrollState();
-        if (scrollState == ScrollbarState::Always)
+        if (const auto termControl = _termControl.get())
         {
-            // If we're in Always mode, and the control is trying to collapse,
-            // go back to expanded
-            if (stateName == L"Collapsed" || stateName == L"CollapsedWithoutAnimation")
+            const auto scrollState = termControl->Settings().ScrollState();
+            if (scrollState == ScrollbarState::Always)
             {
-                for (auto foundState : group.States())
+                // If we're in Always mode, and the control is trying to collapse,
+                // go back to expanded
+                if (stateName == L"Collapsed" || stateName == L"CollapsedWithoutAnimation")
                 {
-                    if (foundState.Name() == L"ExpandedWithoutAnimation")
+                    for (auto foundState : group.States())
                     {
-                        return base_type::GoToStateCore(control, templateRoot, foundState.Name(), group, foundState, false);
+                        if (foundState.Name() == L"ExpandedWithoutAnimation")
+                        {
+                            return base_type::GoToStateCore(control, templateRoot, foundState.Name(), group, foundState, false);
+                        }
                     }
                 }
             }
diff --git a/src/cascadia/TerminalControl/ScrollBarVisualStateManager.h b/src/cascadia/TerminalControl/ScrollBarVisualStateManager.h
index 98c3c2bff9c..a89d13a6714 100644
--- a/src/cascadia/TerminalControl/ScrollBarVisualStateManager.h
+++ b/src/cascadia/TerminalControl/ScrollBarVisualStateManager.h
@@ -11,8 +11,10 @@
 //
 
 #pragma once
-#include "winrt/Windows.UI.Xaml.h"
-#include "winrt/Windows.UI.Xaml.Controls.h"
+#include <winrt/Windows.UI.Xaml.h>
+#include <winrt/Windows.UI.Xaml.Controls.h>
+
+#include "TermControl.h"
 
 #include "ScrollBarVisualStateManager.g.h"
 
@@ -20,12 +22,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 {
     struct ScrollBarVisualStateManager : ScrollBarVisualStateManagerT<ScrollBarVisualStateManager>
     {
-        ScrollBarVisualStateManager();
-
         bool GoToStateCore(winrt::Windows::UI::Xaml::Controls::Control const& control, winrt::Windows::UI::Xaml::FrameworkElement const& templateRoot, hstring const& stateName, winrt::Windows::UI::Xaml::VisualStateGroup const& group, winrt::Windows::UI::Xaml::VisualState const& state, bool useTransitions);
 
     private:
-        winrt::Microsoft::Terminal::Control::TermControl _termControl;
+        winrt::weak_ref<TermControl> _termControl;
+        bool _initialized = false;
     };
 }