Fix a crash when dragging tabs between windows

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(std::move(remainingChild->_content));
         _id = remainingChild->Id();
 
         // Revoke the old event handlers. Remove both the handlers for the panes
@@ -1716,6 +1706,25 @@ void Pane::_SetupChildCloseHandlers()
     });
 }
 
+void Pane::_setPaneContent(IPaneContent content)
+{
+    // The IPaneContent::Close() implementation may be buggy and raise the CloseRequested event again.
+    // To avoid this we unbind the event handlers before calling Close().
+    _closeRequestedRevoker.revoke();
+
+    if (_content)
+    {
+        _content.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 +2275,8 @@ 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>(std::move(_content));
+        _setPaneContent(nullptr);
         _firstChild->_broadcastEnabled = _broadcastEnabled;
     }
 
@@ -2462,6 +2471,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 @@ private:
 
     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,9 @@ private:
     bool _IsLeaf() const noexcept;
     bool _HasFocusedChild() const noexcept;
     void _SetupChildCloseHandlers();
+    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/WindowsTerminal/WindowThread.cpp

@@ -50,15 +50,31 @@ int WindowThread::RunMessagePump()
 
 void WindowThread::_pumpRemainingXamlMessages()
 {
-    MSG msg = {};
-    while (PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE))
-    {
-        ::DispatchMessageW(&msg);
-    }
 }
 
 void WindowThread::RundownForExit()
 {
+    // First of all, closing DesktopWindowXamlSource does not actually deinitialize DXamlCore.
+    // Instead it calls WindowsXamlManager::EnqueueClose which, as the name indicates,
+    // asynchronously deinitializes everything. This is why we have _pumpRemainingXamlMessages.
+    //
+    // Now, the funny thing is that DXamlCore::GetCurrent() can return nullptr. XAML knows it can return nullptr,
+    // because XAML is entirely built around it being nullable. But XAML never ever checks if it is.
+    // So what happens if we have a pending drag on a TabView item while also a pending deinitialization of DXamlCore?
+    // It's a crash! Fun! GH#15689
+    //
+    // This happens because we're told the TabView was rearranged and think the last tab is gone.
+    // As a response we close the window. But in reality, the underlying UIElement.StartDragAsync hasn't
+    // asynchronously completed yet. Once it does, it'll call DXamlCore::GetCurrent() which now is nullptr.
+    // To fix this we simply pump all remaining messages before causing the EnqueueClose (via _host->Close()).
+    while (MsgWaitForMultipleObjects(0, nullptr, FALSE, 1000, QS_ALLINPUT) == WAIT_OBJECT_0)
+    {
+        for (MSG msg; PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE);)
+        {
+            DispatchMessageW(&msg);
+        }
+    }
+
     if (_host)
     {
         _host->UpdateSettingsRequested(_UpdateSettingsRequestedToken);
@@ -72,15 +88,11 @@ void WindowThread::RundownForExit()
         _warmWindow->Close();
     }
 
-    // !! LOAD BEARING !!
-    //
-    // Make sure to finish pumping all the messages for our thread here. We
-    // may think we're all done, but we're not quite. XAML needs more time
-    // to pump the remaining events through, even at the point we're
-    // exiting. So do that now. If you don't, then the last tab to close
-    // will never actually destruct the last tab / TermControl / ControlCore
-    // / renderer.
-    _pumpRemainingXamlMessages();
+    // See the first paragraph in the big comment at the start of this method.
+    for (MSG msg; PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE);)
+    {
+        DispatchMessageW(&msg);
+    }
 }
 
 // Method Description: