Fix circular TermControl reference (#14228)

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 ✅
microsoft · Oct 17, 2022 · bfd480b · bfd480b
1 parent 6d94fbc
commit bfd480b
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 25 deletions.
diff --git 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
@@ -11,21 +11,22 @@
 //
 
 #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"
 
 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;
     };
 }