Fix two panes being closed when just one is (#17358)

#17333 introduced a regression: While it fixes a recursion *into*
`Pane::Close()` that doesn't fix the recursion outside of it.
In this case, `Close()` raises the `Closed` event which results
in another tab being closed because it's bound to `_RemoveTab`.
The recursion is now obvious, because I made the entire process
synchronous. Previously, it would (hopefully) just be scheduled
after the pane and its content are already gone.

The issue can be fixed by moving the recursion check from
`Pane::Close()` to `TerminalTab::Shutdown()` but I felt like
it would better to fix the issue a bit more thoroughly.

`IPaneContent` can raise a `CloseRequested` event to indicate it wants
to be closed. However, that also contained recursion, because the
content would call its own `Close()` to raise the event, which the
tab catches, calls `Close()` on the `Pane` which calls `Close()` on
the content which raises the event again and so on. That's what was
fixed in #17333 among others. We can do this better by not raising
the event from `IPaneContent::Close()`. Instead, that method will now
be exclusively called by `Pane`. The `CloseRequested` event will now
truly be just a request and nothing more. Furthermore, the ownership
of the event handling was moved from the `TerminalTab` to the `Pane`.

To make all of this a bit simpler and more robust, two new methods
were added to `Pane`: `_takePaneContent` and `_setPaneContent`.
These methods ensure that `Close()` is called on the content,
that the event handlers are always added and revoked
and that the ownership transfers cleanly between panes.

## Validation Steps Performed
* Open 3 tabs, close the middle one ✅
* Open 3 vertical panes, close the middle one ✅
* Drag tabs with multiple panes between windows ✅

(cherry picked from commit ce0f8d6)
Service-Card-Id: 92678790
Service-Version: 1.21

Author: lhecker

src/cascadia/TerminalApp/Pane.cpp

@@ -27,10 +27,10 @@ static const int CombinedPaneBorderSize = 2 * PaneBorderSize;
 static const int AnimationDurationInMilliseconds = 200;
 static const Duration AnimationDuration = DurationHelper::FromTimeSpan(winrt::Windows::Foundation::TimeSpan(std::chrono::milliseconds(AnimationDurationInMilliseconds)));
 
-Pane::Pane(const IPaneContent& content, const bool lastFocused) :
-    _content{ content },
+Pane::Pane(IPaneContent content, const bool lastFocused) :
     _lastActive{ lastFocused }
 {
+    _setPaneContent(std::move(content));
     _root.Children().Append(_borderFirst);
 
     const auto& control{ _content.GetRoot() };
@@ -985,17 +985,7 @@ 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;
+    _setPaneContent(nullptr);
     // Fire our Closed event to tell our parent that we should be removed.
     Closed.raise(nullptr, nullptr);
 }
@@ -1007,7 +997,7 @@ void Pane::Shutdown()
 {
     if (_IsLeaf())
     {
-        _content.Close();
+        _setPaneContent(nullptr);
     }
     else
     {
@@ -1411,7 +1401,7 @@ void Pane::_CloseChild(const bool closeFirst)
         _borders = _GetCommonBorders();
 
         // take the control, profile, id and isDefTermSession of the pane that _wasn't_ closed.
-        _content = remainingChild->_content;
+        _setPaneContent(remainingChild->_takePaneContent());
         _id = remainingChild->Id();
 
         // Revoke the old event handlers. Remove both the handlers for the panes
@@ -1716,6 +1706,34 @@ void Pane::_SetupChildCloseHandlers()
     });
 }
 
+// With this method you take ownership of the control from this Pane.
+// Assign it to another Pane with _setPaneContent() or Close() it.
+IPaneContent Pane::_takePaneContent()
+{
+    _closeRequestedRevoker.revoke();
+    return std::move(_content);
+}
+
+// This method safely sets the content of the Pane. It'll ensure to revoke and
+// assign event handlers, and to Close() the existing content if there's any.
+// The new content can be nullptr to remove any content.
+void Pane::_setPaneContent(IPaneContent content)
+{
+    // The IPaneContent::Close() implementation may be buggy and raise the CloseRequested event again.
+    // _takePaneContent() avoids this as it revokes the event handler.
+    if (const auto c = _takePaneContent())
+    {
+        c.Close();
+    }
+
+    _content = std::move(content);
+
+    if (_content)
+    {
+        _closeRequestedRevoker = _content.CloseRequested(winrt::auto_revoke, [this](auto&&, auto&&) { Close(); });
+    }
+}
+
 // Method Description:
 // - Sets up row/column definitions for this pane. There are three total
 //   row/cols. The middle one is for the separator. The first and third are for
@@ -2266,8 +2284,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitDirect
     else
     {
         //   Move our control, guid, isDefTermSession into the first one.
-        _firstChild = std::make_shared<Pane>(_content);
-        _content = nullptr;
+        _firstChild = std::make_shared<Pane>(_takePaneContent());
         _firstChild->_broadcastEnabled = _broadcastEnabled;
     }
 
@@ -2462,6 +2479,11 @@ bool Pane::_HasChild(const std::shared_ptr<Pane> child)
     });
 }
 
+winrt::TerminalApp::TerminalPaneContent Pane::_getTerminalContent() const
+{
+    return _IsLeaf() ? _content.try_as<winrt::TerminalApp::TerminalPaneContent>() : nullptr;
+}
+
 // Method Description:
 // - Recursive function that finds a pane with the given ID
 // Arguments:

src/cascadia/TerminalApp/Pane.h

@@ -62,7 +62,7 @@ struct PaneResources
 class Pane : public std::enable_shared_from_this<Pane>
 {
 public:
-    Pane(const winrt::TerminalApp::IPaneContent& content,
+    Pane(winrt::TerminalApp::IPaneContent content,
          const bool lastFocused = false);
 
     Pane(std::shared_ptr<Pane> first,
@@ -248,13 +248,13 @@ 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 };
 
     winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker;
     winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker;
+    winrt::TerminalApp::IPaneContent::CloseRequested_revoker _closeRequestedRevoker;
 
     Borders _borders{ Borders::None };
 
@@ -264,11 +264,10 @@ class Pane : public std::enable_shared_from_this<Pane>
     bool _IsLeaf() const noexcept;
     bool _HasFocusedChild() const noexcept;
     void _SetupChildCloseHandlers();
+    winrt::TerminalApp::IPaneContent _takePaneContent();
+    void _setPaneContent(winrt::TerminalApp::IPaneContent content);
     bool _HasChild(const std::shared_ptr<Pane> child);
-    winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const
-    {
-        return _IsLeaf() ? _content.try_as<winrt::TerminalApp::TerminalPaneContent>() : nullptr;
-    }
+    winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const;
 
     std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> _Split(winrt::Microsoft::Terminal::Settings::Model::SplitDirection splitType,
                                                                    const float splitSize,

src/cascadia/TerminalApp/ScratchpadContent.cpp

@@ -45,7 +45,6 @@ namespace winrt::TerminalApp::implementation
     }
     void ScratchpadContent::Close()
     {
-        CloseRequested.raise(*this, nullptr);
     }
 
     INewContentArgs ScratchpadContent::GetNewTerminalArgs(const BuildStartupKind /* kind */) const

src/cascadia/TerminalApp/SettingsPaneContent.cpp

@@ -47,7 +47,6 @@ namespace winrt::TerminalApp::implementation
     }
     void SettingsPaneContent::Close()
     {
-        CloseRequested.raise(*this, nullptr);
     }
 
     INewContentArgs SettingsPaneContent::GetNewTerminalArgs(const BuildStartupKind /*kind*/) const

src/cascadia/TerminalApp/TerminalPaneContent.cpp

@@ -78,8 +78,6 @@ namespace winrt::TerminalApp::implementation
             _bellPlayer = nullptr;
             _bellPlayerCreated = false;
         }
-
-        CloseRequested.raise(*this, nullptr);
     }
 
     winrt::hstring TerminalPaneContent::Icon() const
@@ -239,19 +237,20 @@ namespace winrt::TerminalApp::implementation
 
         if (_profile)
         {
-            if (_isDefTermSession && _profile.CloseOnExit() == CloseOnExitMode::Automatic)
-            {
-                // For 'automatic', we only care about the connection state if we were launched by Terminal
-                // Since we were launched via defterm, ignore the connection state (i.e. we treat the
-                // close on exit mode as 'always', see GH #13325 for discussion)
-                Close();
-            }
-
             const auto mode = _profile.CloseOnExit();
-            if ((mode == CloseOnExitMode::Always) ||
-                ((mode == CloseOnExitMode::Graceful || mode == CloseOnExitMode::Automatic) && newConnectionState == ConnectionState::Closed))
+
+            if (
+                // This one is obvious: If the user asked for "always" we do just that.
+                (mode == CloseOnExitMode::Always) ||
+                // Otherwise, and unless the user asked for the opposite of "always",
+                // close the pane when the connection closed gracefully (not failed).
+                (mode != CloseOnExitMode::Never && newConnectionState == ConnectionState::Closed) ||
+                // However, defterm handoff can result in Windows Terminal randomly opening which may be annoying,
+                // so by default we should at least always close the pane, even if the command failed.
+                // See GH #13325 for discussion.
+                (mode == CloseOnExitMode::Automatic && _isDefTermSession))
             {
-                Close();
+                CloseRequested.raise(nullptr, nullptr);
             }
         }
     }
@@ -331,7 +330,7 @@ namespace winrt::TerminalApp::implementation
     void TerminalPaneContent::_closeTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
                                                              const winrt::Windows::Foundation::IInspectable& /*args*/)
     {
-        Close();
+        CloseRequested.raise(nullptr, nullptr);
     }
 
     void TerminalPaneContent::_restartTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,

src/cascadia/TerminalApp/TerminalTab.cpp

@@ -947,26 +947,6 @@ namespace winrt::TerminalApp::implementation
         auto dispatcher = TabViewItem().Dispatcher();
         ContentEventTokens events{};
 
-        events.CloseRequested = content.CloseRequested(
-            winrt::auto_revoke,
-            [this](auto&& sender, auto&&) {
-                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)
-                    {
-                        pane->Close();
-                    }
-                }
-            });
-
         events.TitleChanged = content.TitleChanged(
             winrt::auto_revoke,
             [dispatcher, weakThis](auto&&, auto&&) -> winrt::fire_and_forget {

src/cascadia/TerminalApp/TerminalTab.h

@@ -134,7 +134,6 @@ namespace winrt::TerminalApp::implementation
             winrt::TerminalApp::IPaneContent::ConnectionStateChanged_revoker ConnectionStateChanged;
             winrt::TerminalApp::IPaneContent::ReadOnlyChanged_revoker ReadOnlyChanged;
             winrt::TerminalApp::IPaneContent::FocusRequested_revoker FocusRequested;
-            winrt::TerminalApp::IPaneContent::CloseRequested_revoker CloseRequested;
 
             // These events literally only apply if the content is a TermControl.
             winrt::Microsoft::Terminal::Control::TermControl::KeySent_revoker KeySent;

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -431,12 +431,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         try
         {
             // GH#11556 - make sure to format the error code to this string as an UNSIGNED int
-            winrt::hstring exitText{ fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status)) };
-            TerminalOutput.raise(L"\r\n");
-            TerminalOutput.raise(exitText);
-            TerminalOutput.raise(L"\r\n");
-            TerminalOutput.raise(RS_(L"CtrlDToClose"));
-            TerminalOutput.raise(L"\r\n");
+            const auto msg1 = fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status));
+            const auto msg2 = RS_(L"CtrlDToClose");
+            const auto msg = fmt::format(FMT_COMPILE(L"\r\n{}\r\n{}\r\n"), msg1, msg2);
+            TerminalOutput.raise(msg);
         }
         CATCH_LOG();
     }