Fix a crash when closing panes (#17333)

Calling Close() from within WalkPanes is not safe. Simply using
_FindPane is enough to fix this.

This PR also fixes another potential source of infinite recursion, and
fixes panes being passed by-value into the callbacks.

Closes #17305

## Validation Steps Performed
* Split panes vertically 3 times
* `exit` the middle, the bottom and final one, in that order
* Doesn't crash ✅

Author: lhecker

src/cascadia/TerminalApp/Pane.cpp

@@ -467,7 +467,7 @@ std::shared_ptr<Pane> Pane::NextPane(const std::shared_ptr<Pane> targetPane)
     std::shared_ptr<Pane> nextPane = nullptr;
     auto foundTarget = false;
 
-    auto foundNext = WalkTree([&](auto pane) {
+    auto foundNext = WalkTree([&](const auto& pane) {
         // If we are a parent pane we don't want to move to one of our children
         if (foundTarget && targetPane->_HasChild(pane))
         {
@@ -985,6 +985,17 @@ void Pane::_ContentLostFocusHandler(const winrt::Windows::Foundation::IInspectab
 // - <none>
 void Pane::Close()
 {
+    // Pane has two events, CloseRequested and Closed. CloseRequested is raised by the content asking to be closed,
+    // but also by the window who owns the tab when it's closing. The event is then caught by the TerminalTab which
+    // calls Close() which then raises the Closed event. Now, if this is the last pane in the window, this will result
+    // in the window raising CloseRequested again which leads to infinite recursion, so we need to guard against that.
+    // Ideally we would have just a single event in the future.
+    if (_closed)
+    {
+        return;
+    }
+
+    _closed = true;
     // Fire our Closed event to tell our parent that we should be removed.
     Closed.raise(nullptr, nullptr);
 }
@@ -1341,7 +1352,7 @@ std::shared_ptr<Pane> Pane::DetachPane(std::shared_ptr<Pane> pane)
         detached->_ApplySplitDefinitions();
 
         // Trigger the detached event on each child
-        detached->WalkTree([](auto pane) {
+        detached->WalkTree([](const auto& pane) {
             pane->Detached.raise(pane);
         });
 
@@ -1542,7 +1553,7 @@ void Pane::_CloseChild(const bool closeFirst)
         {
             // update our path to our first remaining leaf
             _parentChildPath = _firstChild;
-            _firstChild->WalkTree([](auto p) {
+            _firstChild->WalkTree([](const auto& p) {
                 if (p->_IsLeaf())
                 {
                     return true;
@@ -2398,7 +2409,7 @@ void Pane::Id(uint32_t id) noexcept
 bool Pane::FocusPane(const uint32_t id)
 {
     // Always clear the parent child path if we are focusing a leaf
-    return WalkTree([=](auto p) {
+    return WalkTree([=](const auto& p) {
         p->_parentChildPath.reset();
         if (p->_id == id)
         {
@@ -2421,7 +2432,7 @@ bool Pane::FocusPane(const uint32_t id)
 // - true if focus was set
 bool Pane::FocusPane(const std::shared_ptr<Pane> pane)
 {
-    return WalkTree([&](auto p) {
+    return WalkTree([&](const auto& p) {
         if (p == pane)
         {
             p->_Focus();

src/cascadia/TerminalApp/Pane.h

@@ -248,7 +248,7 @@ class Pane : public std::enable_shared_from_this<Pane>
 
     std::optional<uint32_t> _id;
     std::weak_ptr<Pane> _parentChildPath{};
-
+    bool _closed{ false };
     bool _lastActive{ false };
     winrt::event_token _firstClosedToken{ 0 };
     winrt::event_token _secondClosedToken{ 0 };

src/cascadia/TerminalApp/TabManagement.cpp

@@ -739,7 +739,7 @@ namespace winrt::TerminalApp::implementation
             }
 
             // Clean read-only mode to prevent additional prompt if closing the pane triggers closing of a hosting tab
-            pane->WalkTree([](auto p) {
+            pane->WalkTree([](const auto& p) {
                 if (const auto control{ p->GetTerminalControl() })
                 {
                     if (control.ReadOnly())

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -3812,7 +3812,7 @@ namespace winrt::TerminalApp::implementation
             // recipe for disaster. We won't ever open up a tab in this window.
             newTerminalArgs.Elevate(false);
             const auto newPane = _MakePane(newTerminalArgs, nullptr, connection);
-            newPane->WalkTree([](auto pane) {
+            newPane->WalkTree([](const auto& pane) {
                 pane->FinalizeConfigurationGivenDefault();
             });
             _CreateNewTabFromPane(newPane);

src/cascadia/TerminalApp/TerminalTab.cpp

@@ -41,7 +41,7 @@ namespace winrt::TerminalApp::implementation
 
         auto firstId = _nextPaneId;
 
-        _rootPane->WalkTree([&](std::shared_ptr<Pane> pane) {
+        _rootPane->WalkTree([&](const auto& pane) {
             // update the IDs on each pane
             if (pane->_IsLeaf())
             {
@@ -203,7 +203,7 @@ namespace winrt::TerminalApp::implementation
     {
         ASSERT_UI_THREAD();
 
-        _rootPane->WalkTree([&](std::shared_ptr<Pane> pane) {
+        _rootPane->WalkTree([&](const auto& pane) {
             // Attach event handlers to each new pane
             _AttachEventHandlersToPane(pane);
             if (auto content = pane->GetContent())
@@ -275,7 +275,7 @@ namespace winrt::TerminalApp::implementation
         _UpdateHeaderControlMaxWidth();
 
         // Update the settings on all our panes.
-        _rootPane->WalkTree([&](auto pane) {
+        _rootPane->WalkTree([&](const auto& pane) {
             pane->UpdateSettings(settings);
             return false;
         });
@@ -534,7 +534,7 @@ namespace winrt::TerminalApp::implementation
 
         // Add the new event handlers to the new pane(s)
         // and update their ids.
-        pane->WalkTree([&](auto p) {
+        pane->WalkTree([&](const auto& p) {
             _AttachEventHandlersToPane(p);
             if (p->_IsLeaf())
             {
@@ -624,7 +624,7 @@ namespace winrt::TerminalApp::implementation
         // manually.
         _rootPane->Closed(_rootClosedToken);
         auto p = _rootPane;
-        p->WalkTree([](auto pane) {
+        p->WalkTree([](const auto& pane) {
             pane->Detached.raise(pane);
         });
 
@@ -650,7 +650,7 @@ namespace winrt::TerminalApp::implementation
 
         // Add the new event handlers to the new pane(s)
         // and update their ids.
-        pane->WalkTree([&](auto p) {
+        pane->WalkTree([&](const auto& p) {
             _AttachEventHandlersToPane(p);
             if (p->_IsLeaf())
             {
@@ -949,26 +949,20 @@ namespace winrt::TerminalApp::implementation
 
         events.CloseRequested = content.CloseRequested(
             winrt::auto_revoke,
-            [dispatcher, weakThis](auto sender, auto&&) -> winrt::fire_and_forget {
-                // Don't forget! this ^^^^^^^^ sender can't be a reference, this is a async callback.
-
-                // The lambda lives in the `std::function`-style container owned by `control`. That is, when the
-                // `control` gets destroyed the lambda struct also gets destroyed. In other words, we need to
-                // copy `weakThis` onto the stack, because that's the only thing that gets captured in coroutines.
-                // See: https://devblogs.microsoft.com/oldnewthing/20211103-00/?p=105870
-                const auto weakThisCopy = weakThis;
-                co_await wil::resume_foreground(dispatcher);
-                // Check if Tab's lifetime has expired
-                if (auto tab{ weakThisCopy.get() })
+            [this](auto&& sender, auto&&) {
+                if (const auto content{ sender.try_as<TerminalApp::IPaneContent>() })
                 {
-                    if (const auto content{ sender.try_as<TerminalApp::IPaneContent>() })
+                    // Calling Close() while walking the tree is not safe, because Close() mutates the tree.
+                    const auto pane = _rootPane->_FindPane([&](const auto& p) -> std::shared_ptr<Pane> {
+                        if (p->GetContent() == content)
+                        {
+                            return p;
+                        }
+                        return {};
+                    });
+                    if (pane)
                     {
-                        tab->_rootPane->WalkTree([content](std::shared_ptr<Pane> pane) {
-                            if (pane->GetContent() == content)
-                            {
-                                pane->Close();
-                            }
-                        });
+                        pane->Close();
                     }
                 }
             });