Fixed self reference capture in Tab and TerminalPage

src/cascadia/TerminalApp/Tab.cpp

@@ -25,10 +25,6 @@ Tab::Tab(const GUID& profile, const TermControl& control)
 
     _activePane = _rootPane;
 
-    _AttachEventHandlersToPane(_rootPane);
-
-    _AttachEventHandlersToControl(control);
-
     _MakeTabViewItem();
 }
 
@@ -108,6 +104,19 @@ std::optional<GUID> Tab::GetFocusedProfile() const noexcept
     return _activePane->GetFocusedProfile();
 }
 
+// Method Description:
+// - Called after construction of a Tab object to bind event handlers to its
+//   associated Pane and TermControl object
+// Arguments:
+// - control: reference to the TermControl object to bind event to
+// Return Value:
+// - <none>
+void Tab::BindEventHandlers(const TermControl& control) noexcept
+{
+    _AttachEventHandlersToPane(_rootPane);
+    _AttachEventHandlersToControl(control);
+}
+
 // Method Description:
 // - Attempts to update the settings of this tab's tree of panes.
 // Arguments:
@@ -147,8 +156,13 @@ void Tab::UpdateIcon(const winrt::hstring iconPath)
 
     _lastIconPath = iconPath;
 
-    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
-        _tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(_lastIconPath));
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
+
+    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
+        if (auto tab{ weakThis.lock() })
+        {
+            tab->_tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(tab->_lastIconPath));
+        }
     });
 }
 
@@ -175,8 +189,13 @@ void Tab::SetTabText(const winrt::hstring& text)
 {
     // Copy the hstring, so we don't capture a dead reference
     winrt::hstring textCopy{ text };
-    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), this]() {
-        _tabViewItem.Header(winrt::box_value(text));
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
+
+    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), weakThis]() {
+        if (auto tab{ weakThis.lock() })
+        {
+            tab->_tabViewItem.Header(winrt::box_value(text));
+        }
     });
 }
 
@@ -296,12 +315,16 @@ void Tab::ClosePane()
 // - <none>
 void Tab::_AttachEventHandlersToControl(const TermControl& control)
 {
-    control.TitleChanged([this](auto newTitle) {
-        // The title of the control changed, but not necessarily the title
-        // of the tab. Get the title of the active pane of the tab, and set
-        // the tab's text to the active panes' text.
-        auto newTabTitle = GetActiveTitle();
-        SetTabText(newTabTitle);
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
+
+    control.TitleChanged([weakThis](auto newTitle) {
+        // Check if Tab's lifetime has expired
+        if (auto tab{ weakThis.lock() })
+        {
+            // The title of the control changed, but not necessarily the title of the tab.
+            // Set the tab's text to the active panes' text.
+            tab->SetTabText(tab->GetActiveTitle());
+        }
     });
 }
 
@@ -316,22 +339,24 @@ void Tab::_AttachEventHandlersToControl(const TermControl& control)
 // - <none>
 void Tab::_AttachEventHandlersToPane(std::shared_ptr<Pane> pane)
 {
-    pane->GotFocus([this](std::shared_ptr<Pane> sender) {
-        // Do nothing if it's the same pane as before.
-        if (sender == _activePane)
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
+
+    pane->GotFocus([weakThis](std::shared_ptr<Pane> sender) {
+        // Do nothing if the Tab's lifetime is expired or pane isn't new.
+        auto tab{ weakThis.lock() };
+        if (tab && sender != tab->_activePane)
         {
-            return;
-        }
-        // Clear the active state of the entire tree, and mark only the sender as active.
-        _rootPane->ClearActive();
-        _activePane = sender;
-        _activePane->SetActive();
+            // Clear the active state of the entire tree, and mark only the sender as active.
+            tab->_rootPane->ClearActive();
+            tab->_activePane = sender;
+            tab->_activePane->SetActive();
 
-        // Update our own title text to match the newly-active pane.
-        SetTabText(GetActiveTitle());
+            // Update our own title text to match the newly-active pane.
+            tab->SetTabText(tab->GetActiveTitle());
 
-        // Raise our own ActivePaneChanged event.
-        _ActivePaneChangedHandlers();
+            // Raise our own ActivePaneChanged event.
+            tab->_ActivePaneChangedHandlers();
+        }
     });
 }
 

src/cascadia/TerminalApp/Tab.h

@@ -5,11 +5,14 @@
 #include <winrt/Microsoft.UI.Xaml.Controls.h>
 #include "Pane.h"
 
-class Tab
+class Tab : public std::enable_shared_from_this<Tab>
 {
 public:
     Tab(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
 
+    // Called after construction to setup events with weak_ptr
+    void BindEventHandlers(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control) noexcept;
+
     winrt::Microsoft::UI::Xaml::Controls::TabViewItem GetTabViewItem();
     winrt::Windows::UI::Xaml::UIElement GetRootElement();
     winrt::Microsoft::Terminal::TerminalControl::TermControl GetActiveTerminalControl() const;

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -60,23 +60,36 @@ namespace winrt::TerminalApp::implementation
         _tabView = _tabRow.TabView();
         _rearranging = false;
 
-        _tabView.TabDragStarting([this](auto&& /*o*/, auto&& /*a*/) {
-            _rearranging = true;
-            _rearrangeFrom = std::nullopt;
-            _rearrangeTo = std::nullopt;
-        });
+        // weak_ptr to this TerminalPage object lambda capturing
+        auto weakThis{ get_weak() };
 
-        _tabView.TabDragCompleted([this](auto&& /*o*/, auto&& /*a*/) {
-            if (_rearrangeFrom.has_value() && _rearrangeTo.has_value() && _rearrangeTo != _rearrangeFrom)
+        _tabView.TabDragStarting([weakThis](auto&& /*o*/, auto&& /*a*/) {
+            if (auto page{ weakThis.get() })
             {
-                auto tab = _tabs.at(_rearrangeFrom.value());
-                _tabs.erase(_tabs.begin() + _rearrangeFrom.value());
-                _tabs.insert(_tabs.begin() + _rearrangeTo.value(), tab);
+                page->_rearranging = true;
+                page->_rearrangeFrom = std::nullopt;
+                page->_rearrangeTo = std::nullopt;
             }
+        });
+
+        _tabView.TabDragCompleted([weakThis](auto&& /*o*/, auto&& /*a*/) {
+            if (auto page{ weakThis.get() })
+            {
+                auto& from{ page->_rearrangeFrom };
+                auto& to{ page->_rearrangeTo };
 
-            _rearranging = false;
-            _rearrangeFrom = std::nullopt;
-            _rearrangeTo = std::nullopt;
+                if (from.has_value() && to.has_value() && to != from)
+                {
+                    auto& tabs{ page->_tabs };
+                    auto tab = tabs.at(from.value());
+                    tabs.erase(tabs.begin() + from.value());
+                    tabs.insert(tabs.begin() + to.value(), tab);
+                }
+
+                page->_rearranging = false;
+                from = std::nullopt;
+                to = std::nullopt;
+            }
         });
 
         auto tabRowImpl = winrt::get_self<implementation::TabRowControl>(_tabRow);
@@ -100,8 +113,11 @@ namespace winrt::TerminalApp::implementation
         _RegisterActionCallbacks();
 
         //Event Bindings (Early)
-        _newTabButton.Click([this](auto&&, auto&&) {
-            this->_OpenNewTab(std::nullopt);
+        _newTabButton.Click([weakThis](auto&&, auto&&) {
+            if (auto page{ weakThis.get() })
+            {
+                page->_OpenNewTab(std::nullopt);
+            }
         });
         _tabView.SelectionChanged({ this, &TerminalPage::_OnTabSelectionChanged });
         _tabView.TabCloseRequested({ this, &TerminalPage::_OnTabCloseRequested });
@@ -301,8 +317,13 @@ namespace winrt::TerminalApp::implementation
                 profileMenuItem.FontWeight(FontWeights::Bold());
             }
 
-            profileMenuItem.Click([this, profileIndex](auto&&, auto&&) {
-                this->_OpenNewTab({ profileIndex });
+            auto weakThis{ get_weak() };
+
+            profileMenuItem.Click([profileIndex, weakThis](auto&&, auto&&) {
+                if (auto page{ weakThis.get() })
+                {
+                    page->_OpenNewTab({ profileIndex });
+                }
             });
             newTabFlyout.Items().Append(profileMenuItem);
         }
@@ -440,17 +461,21 @@ namespace winrt::TerminalApp::implementation
         // Don't capture a strong ref to the tab. If the tab is removed as this
         // is called, we don't really care anymore about handling the event.
         std::weak_ptr<Tab> weakTabPtr = newTab;
+        auto weakThis{ get_weak() };
+
         // When the tab's active pane changes, we'll want to lookup a new icon
         // for it, and possibly propogate the title up to the window.
-        newTab->ActivePaneChanged([this, weakTabPtr]() {
-            if (auto tab = weakTabPtr.lock())
+        newTab->ActivePaneChanged([weakTabPtr, weakThis]() {
+            auto page{ weakThis.get() };
+            auto tab{ weakTabPtr.lock() };
+
+            if (page && tab)
             {
                 // Possibly update the icon of the tab.
-                _UpdateTabIcon(tab);
-
+                page->_UpdateTabIcon(tab);
                 // Possibly update the title of the tab, window to match the newly
                 // focused pane.
-                _UpdateTitle(tab);
+                page->_UpdateTitle(tab);
             }
         });
 
@@ -467,10 +492,16 @@ namespace winrt::TerminalApp::implementation
         tabViewItem.PointerPressed({ this, &TerminalPage::_OnTabClick });
 
         // When the tab is closed, remove it from our list of tabs.
-        newTab->Closed([tabViewItem, this](auto&& /*s*/, auto&& /*e*/) {
-            _tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, this]() {
-                _RemoveTabViewItem(tabViewItem);
-            });
+        newTab->Closed([tabViewItem, weakThis](auto&& /*s*/, auto&& /*e*/) {
+            if (auto page{ weakThis.get() })
+            {
+                page->_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, weakThis]() {
+                    if (auto page{ weakThis.get() })
+                    {
+                        page->_RemoveTabViewItem(tabViewItem);
+                    }
+                });
+            }
         });
 
         // This is one way to set the tab's selected background color.
@@ -762,19 +793,25 @@ namespace winrt::TerminalApp::implementation
         // Add an event handler when the terminal wants to paste data from the Clipboard.
         term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler });
 
+        // Bind Tab events to the TermControl and the Tab's Pane
+        hostingTab->BindEventHandlers(term);
+
         // Don't capture a strong ref to the tab. If the tab is removed as this
         // is called, we don't really care anymore about handling the event.
         std::weak_ptr<Tab> weakTabPtr = hostingTab;
-        term.TitleChanged([this, weakTabPtr](auto newTitle) {
-            auto tab = weakTabPtr.lock();
-            if (!tab)
+        auto weakThis{ get_weak() };
+
+        term.TitleChanged([weakTabPtr, weakThis](auto newTitle) {
+            auto page{ weakThis.get() };
+            auto tab{ weakTabPtr.lock() };
+
+            if (page && tab)
             {
-                return;
+                // The title of the control changed, but not necessarily the title
+                // of the tab. Get the title of the focused pane of the tab, and set
+                // the tab's text to the focused panes' text.
+                page->_UpdateTitle(tab);
             }
-            // The title of the control changed, but not necessarily the title
-            // of the tab. Get the title of the focused pane of the tab, and set
-            // the tab's text to the focused panes' text.
-            _UpdateTitle(tab);
         });
     }
 
@@ -849,9 +886,12 @@ namespace winrt::TerminalApp::implementation
         // GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex)
         //          sometimes set focus to an incorrect tab after removing some tabs
         auto tab = _tabs.at(tabIndex);
-        _tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, this]() {
-            auto tabViewItem = tab->GetTabViewItem();
-            _tabView.SelectedItem(tabViewItem);
+        auto weakThis{ get_weak() };
+        _tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, weakThis]() {
+            if (auto page{ weakThis.get() })
+            {
+                page->_tabView.SelectedItem(tab->GetTabViewItem());
+            }
         });
     }
 
@@ -1350,10 +1390,14 @@ namespace winrt::TerminalApp::implementation
             _UpdateTitle(tab);
         }
 
-        this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
+        auto weakThis{ get_weak() };
+        this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
             // repopulate the new tab button's flyout with entries for each
             // profile, which might have changed
-            _CreateNewTabFlyout();
+            if (auto page{ weakThis.get() })
+            {
+                page->_CreateNewTabFlyout();
+            }
         });
     }