Repository: microsoft/terminal (language: cpp)

Issue:
Terminal hangs when exiting a debug tap session
### Windows Terminal version

1.17.1023

### Windows build number

10.0.19044.2364

### Other Software

_No response_

### Steps to reproduce

1. Make sure the `debugFeatures` option is enabled in the settings.
2. Open a bash shell with a debug tab.
3. Exit the shell with `exit`.


### Expected Behavior

You should be left with the debug tab pane which you can then close.

### Actual Behavior

The terminal hangs.

Running from within the debugger, the main thread appears to be blocked on the `_createCloseLock` in `_ControlConnectionStateChangedHandler`, but there didn't appear to be any other threads holding that lock. I didn't spend that much time investigating, though, so it's quite likely I missed something.


Discussion / hints:
Promoted to P1, marked Sev-Crash (though it's a hang). :smile:
idly: I wonder if #14548 helps / hurts

Fix patch:
diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp
index c70b3fa27da..32cf032606f 100644
--- a/src/cascadia/TerminalApp/Pane.cpp
+++ b/src/cascadia/TerminalApp/Pane.cpp
@@ -684,8 +684,6 @@ bool Pane::SwapPanes(std::shared_ptr<Pane> first, std::shared_ptr<Pane> second)
         return false;
     }
 
-    std::unique_lock lock{ _createCloseLock };
-
     // Recurse through the tree to find the parent panes of each pane that is
     // being swapped.
     auto firstParent = _FindParentOfPane(first);
@@ -1033,12 +1031,31 @@ Pane::PaneNeighborSearch Pane::_FindPaneAndNeighbor(const std::shared_ptr<Pane>
 // - <none>
 // Return Value:
 // - <none>
-void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
-                                                 const winrt::Windows::Foundation::IInspectable& /*args*/)
+winrt::fire_and_forget Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender,
+                                                                   const winrt::Windows::Foundation::IInspectable& /*args*/)
 {
-    std::unique_lock lock{ _createCloseLock };
-    // It's possible that this event handler started being executed, then before
-    // we got the lock, another thread created another child. So our control is
+    auto newConnectionState = ConnectionState::Closed;
+    if (const auto coreState = sender.try_as<ICoreState>())
+    {
+        newConnectionState = coreState.ConnectionState();
+    }
+
+    if (newConnectionState < ConnectionState::Closed)
+    {
+        // Pane doesn't care if the connection isn't entering a terminal state.
+        co_return;
+    }
+
+    const auto weakThis = weak_from_this();
+    co_await wil::resume_foreground(_root.Dispatcher());
+    const auto strongThis = weakThis.lock();
+    if (!strongThis)
+    {
+        co_return;
+    }
+
+    // It's possible that this event handler started being executed, scheduled
+    // on the UI thread, another child got created. So our control is
     // actually no longer _our_ control, and instead could be a descendant.
     //
     // When the control's new Pane takes ownership of the control, the new
@@ -1046,24 +1063,16 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
     // fired after this handler returns, and will properly cleanup state.
     if (!_IsLeaf())
     {
-        return;
+        co_return;
     }
 
-    const auto newConnectionState = _control.ConnectionState();
     const auto previousConnectionState = std::exchange(_connectionState, newConnectionState);
-
-    if (newConnectionState < ConnectionState::Closed)
-    {
-        // Pane doesn't care if the connection isn't entering a terminal state.
-        return;
-    }
-
     if (previousConnectionState < ConnectionState::Connected && newConnectionState >= ConnectionState::Failed)
     {
         // A failure to complete the connection (before it has _connected_) is not covered by "closeOnExit".
         // This is to prevent a misconfiguration (closeOnExit: always, startingDirectory: garbage) resulting
         // in Terminal flashing open and immediately closed.
-        return;
+        co_return;
     }
 
     if (_profile)
@@ -1088,8 +1097,6 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
 void Pane::_CloseTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
                                           const winrt::Windows::Foundation::IInspectable& /*args*/)
 {
-    std::unique_lock lock{ _createCloseLock };
-
     // It's possible that this event handler started being executed, then before
     // we got the lock, another thread created another child. So our control is
     // actually no longer _our_ control, and instead could be a descendant.
@@ -1238,10 +1245,6 @@ void Pane::Close()
 //   and connections beneath it.
 void Pane::Shutdown()
 {
-    // Lock the create/close lock so that another operation won't concurrently
-    // modify our tree
-    std::unique_lock lock{ _createCloseLock };
-
     // Clear out our media player callbacks, and stop any playing media. This
     // will prevent the callback from being triggered after we've closed, and
     // also make sure that our sound stops when we're closed.
@@ -1594,10 +1597,6 @@ std::shared_ptr<Pane> Pane::DetachPane(std::shared_ptr<Pane> pane)
 // - <none>
 void Pane::_CloseChild(const bool closeFirst, const bool isDetaching)
 {
-    // Lock the create/close lock so that another operation won't concurrently
-    // modify our tree
-    std::unique_lock lock{ _createCloseLock };
-
     // If we're a leaf, then chances are both our children closed in close
     // succession. We waited on the lock while the other child was closed, so
     // now we don't have a child to close anymore. Return here. When we moved
@@ -1820,135 +1819,128 @@ void Pane::_CloseChild(const bool closeFirst, const bool isDetaching)
     closedChild->_ClosedByParentHandlers();
 }
 
-winrt::fire_and_forget Pane::_CloseChildRoutine(const bool closeFirst)
+void Pane::_CloseChildRoutine(const bool closeFirst)
 {
-    auto weakThis{ shared_from_this() };
-
-    co_await wil::resume_foreground(_root.Dispatcher());
+    // This will query if animations are enabled via the "Show animations in
+    // Windows" setting in the OS
+    winrt::Windows::UI::ViewManagement::UISettings uiSettings;
+    const auto animationsEnabledInOS = uiSettings.AnimationsEnabled();
+    const auto animationsEnabledInApp = Media::Animation::Timeline::AllowDependentAnimations();
 
-    if (auto pane{ weakThis.get() })
+    // GH#7252: If either child is zoomed, just skip the animation. It won't work.
+    const auto eitherChildZoomed = _firstChild->_zoomed || _secondChild->_zoomed;
+    // If animations are disabled, just skip this and go straight to
+    // _CloseChild. Curiously, the pane opening animation doesn't need this,
+    // and will skip straight to Completed when animations are disabled, but
+    // this one doesn't seem to.
+    if (!animationsEnabledInOS || !animationsEnabledInApp || eitherChildZoomed)
     {
-        // This will query if animations are enabled via the "Show animations in
-        // Windows" setting in the OS
-        winrt::Windows::UI::ViewManagement::UISettings uiSettings;
-        const auto animationsEnabledInOS = uiSettings.AnimationsEnabled();
-        const auto animationsEnabledInApp = Media::Animation::Timeline::AllowDependentAnimations();
-
-        // GH#7252: If either child is zoomed, just skip the animation. It won't work.
-        const auto eitherChildZoomed = pane->_firstChild->_zoomed || pane->_secondChild->_zoomed;
-        // If animations are disabled, just skip this and go straight to
-        // _CloseChild. Curiously, the pane opening animation doesn't need this,
-        // and will skip straight to Completed when animations are disabled, but
-        // this one doesn't seem to.
-        if (!animationsEnabledInOS || !animationsEnabledInApp || eitherChildZoomed)
-        {
-            pane->_CloseChild(closeFirst, false);
-            co_return;
-        }
+        _CloseChild(closeFirst, false);
+        return;
+    }
 
-        // Setup the animation
+    // Setup the animation
 
-        auto removedChild = closeFirst ? _firstChild : _secondChild;
-        auto remainingChild = closeFirst ? _secondChild : _firstChild;
-        const auto splitWidth = _splitState == SplitState::Vertical;
+    auto removedChild = closeFirst ? _firstChild : _secondChild;
+    auto remainingChild = closeFirst ? _secondChild : _firstChild;
+    const auto splitWidth = _splitState == SplitState::Vertical;
 
-        Size removedOriginalSize{
-            ::base::saturated_cast<float>(removedChild->_root.ActualWidth()),
-            ::base::saturated_cast<float>(removedChild->_root.ActualHeight())
-        };
-        Size remainingOriginalSize{
-            ::base::saturated_cast<float>(remainingChild->_root.ActualWidth()),
-            ::base::saturated_cast<float>(remainingChild->_root.ActualHeight())
-        };
+    Size removedOriginalSize{
+        ::base::saturated_cast<float>(removedChild->_root.ActualWidth()),
+        ::base::saturated_cast<float>(removedChild->_root.ActualHeight())
+    };
+    Size remainingOriginalSize{
+        ::base::saturated_cast<float>(remainingChild->_root.ActualWidth()),
+        ::base::saturated_cast<float>(remainingChild->_root.ActualHeight())
+    };
 
-        // Remove both children from the grid
-        _borderFirst.Child(nullptr);
-        _borderSecond.Child(nullptr);
+    // Remove both children from the grid
+    _borderFirst.Child(nullptr);
+    _borderSecond.Child(nullptr);
 
-        if (_splitState == SplitState::Vertical)
-        {
-            Controls::Grid::SetColumn(_borderFirst, 0);
-            Controls::Grid::SetColumn(_borderSecond, 1);
-        }
-        else if (_splitState == SplitState::Horizontal)
-        {
-            Controls::Grid::SetRow(_borderFirst, 0);
-            Controls::Grid::SetRow(_borderSecond, 1);
-        }
+    if (_splitState == SplitState::Vertical)
+    {
+        Controls::Grid::SetColumn(_borderFirst, 0);
+        Controls::Grid::SetColumn(_borderSecond, 1);
+    }
+    else if (_splitState == SplitState::Horizontal)
+    {
+        Controls::Grid::SetRow(_borderFirst, 0);
+        Controls::Grid::SetRow(_borderSecond, 1);
+    }
 
-        // Create the dummy grid. This grid will be the one we actually animate,
-        // in the place of the closed pane.
-        Controls::Grid dummyGrid;
-        // GH#603 - we can safely add a BG here, as the control is gone right
-        // away, to fill the space as the rest of the pane expands.
-        dummyGrid.Background(_themeResources.unfocusedBorderBrush);
-        // It should be the size of the closed pane.
-        dummyGrid.Width(removedOriginalSize.Width);
-        dummyGrid.Height(removedOriginalSize.Height);
+    // Create the dummy grid. This grid will be the one we actually animate,
+    // in the place of the closed pane.
+    Controls::Grid dummyGrid;
+    // GH#603 - we can safely add a BG here, as the control is gone right
+    // away, to fill the space as the rest of the pane expands.
+    dummyGrid.Background(_themeResources.unfocusedBorderBrush);
+    // It should be the size of the closed pane.
+    dummyGrid.Width(removedOriginalSize.Width);
+    dummyGrid.Height(removedOriginalSize.Height);
 
-        _borderFirst.Child(closeFirst ? dummyGrid : remainingChild->GetRootElement());
-        _borderSecond.Child(closeFirst ? remainingChild->GetRootElement() : dummyGrid);
+    _borderFirst.Child(closeFirst ? dummyGrid : remainingChild->GetRootElement());
+    _borderSecond.Child(closeFirst ? remainingChild->GetRootElement() : dummyGrid);
 
-        // Set up the rows/cols as auto/auto, so they'll only use the size of
-        // the elements in the grid.
-        //
-        // * For the closed pane, we want to make that row/col "auto" sized, so
-        //   it takes up as much space as is available.
-        // * For the remaining pane, we'll make that row/col "*" sized, so it
-        //   takes all the remaining space. As the dummy grid is resized down,
-        //   the remaining pane will expand to take the rest of the space.
-        _root.ColumnDefinitions().Clear();
-        _root.RowDefinitions().Clear();
-        if (_splitState == SplitState::Vertical)
-        {
-            auto firstColDef = Controls::ColumnDefinition();
-            auto secondColDef = Controls::ColumnDefinition();
-            firstColDef.Width(!closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
-            secondColDef.Width(closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
-            _root.ColumnDefinitions().Append(firstColDef);
-            _root.ColumnDefinitions().Append(secondColDef);
-        }
-        else if (_splitState == SplitState::Horizontal)
-        {
-            auto firstRowDef = Controls::RowDefinition();
-            auto secondRowDef = Controls::RowDefinition();
-            firstRowDef.Height(!closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
-            secondRowDef.Height(closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
-            _root.RowDefinitions().Append(firstRowDef);
-            _root.RowDefinitions().Append(secondRowDef);
-        }
+    // Set up the rows/cols as auto/auto, so they'll only use the size of
+    // the elements in the grid.
+    //
+    // * For the closed pane, we want to make that row/col "auto" sized, so
+    //   it takes up as much space as is available.
+    // * For the remaining pane, we'll make that row/col "*" sized, so it
+    //   takes all the remaining space. As the dummy grid is resized down,
+    //   the remaining pane will expand to take the rest of the space.
+    _root.ColumnDefinitions().Clear();
+    _root.RowDefinitions().Clear();
+    if (_splitState == SplitState::Vertical)
+    {
+        auto firstColDef = Controls::ColumnDefinition();
+        auto secondColDef = Controls::ColumnDefinition();
+        firstColDef.Width(!closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
+        secondColDef.Width(closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
+        _root.ColumnDefinitions().Append(firstColDef);
+        _root.ColumnDefinitions().Append(secondColDef);
+    }
+    else if (_splitState == SplitState::Horizontal)
+    {
+        auto firstRowDef = Controls::RowDefinition();
+        auto secondRowDef = Controls::RowDefinition();
+        firstRowDef.Height(!closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
+        secondRowDef.Height(closeFirst ? GridLengthHelper::FromValueAndType(1, GridUnitType::Star) : GridLengthHelper::Auto());
+        _root.RowDefinitions().Append(firstRowDef);
+        _root.RowDefinitions().Append(secondRowDef);
+    }
 
-        // Animate the dummy grid from its current size down to 0
-        Media::Animation::DoubleAnimation animation{};
-        animation.Duration(AnimationDuration);
-        animation.From(splitWidth ? removedOriginalSize.Width : removedOriginalSize.Height);
-        animation.To(0.0);
-        // This easing is the same as the entrance animation.
-        animation.EasingFunction(Media::Animation::QuadraticEase{});
-        animation.EnableDependentAnimation(true);
+    // Animate the dummy grid from its current size down to 0
+    Media::Animation::DoubleAnimation animation{};
+    animation.Duration(AnimationDuration);
+    animation.From(splitWidth ? removedOriginalSize.Width : removedOriginalSize.Height);
+    animation.To(0.0);
+    // This easing is the same as the entrance animation.
+    animation.EasingFunction(Media::Animation::QuadraticEase{});
+    animation.EnableDependentAnimation(true);
 
-        Media::Animation::Storyboard s;
-        s.Duration(AnimationDuration);
-        s.Children().Append(animation);
-        s.SetTarget(animation, dummyGrid);
-        s.SetTargetProperty(animation, splitWidth ? L"Width" : L"Height");
+    Media::Animation::Storyboard s;
+    s.Duration(AnimationDuration);
+    s.Children().Append(animation);
+    s.SetTarget(animation, dummyGrid);
+    s.SetTargetProperty(animation, splitWidth ? L"Width" : L"Height");
 
-        // Start the animation.
-        s.Begin();
+    // Start the animation.
+    s.Begin();
 
-        std::weak_ptr<Pane> weakThis{ shared_from_this() };
+    std::weak_ptr<Pane> weakThis{ shared_from_this() };
 
-        // When the animation is completed, reparent the child's content up to
-        // us, and remove the child nodes from the tree.
-        animation.Completed([weakThis, closeFirst](auto&&, auto&&) {
-            if (auto pane{ weakThis.lock() })
-            {
-                // We don't need to manually undo any of the above trickiness.
-                // We're going to re-parent the child's content into us anyways
-                pane->_CloseChild(closeFirst, false);
-            }
-        });
-    }
+    // When the animation is completed, reparent the child's content up to
+    // us, and remove the child nodes from the tree.
+    animation.Completed([weakThis, closeFirst](auto&&, auto&&) {
+        if (auto pane{ weakThis.lock() })
+        {
+            // We don't need to manually undo any of the above trickiness.
+            // We're going to re-parent the child's content into us anyways
+            pane->_CloseChild(closeFirst, false);
+        }
+    });
 }
 
 // Method Description:
@@ -2488,10 +2480,6 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitDirect
 {
     auto actualSplitType = _convertAutomaticOrDirectionalSplitState(splitType);
 
-    // Lock the create/close lock so that another operation won't concurrently
-    // modify our tree
-    std::unique_lock lock{ _createCloseLock };
-
     if (_IsLeaf())
     {
         // revoke our handler - the child will take care of the control now.
diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h
index a6137534dac..6920a90b341 100644
--- a/src/cascadia/TerminalApp/Pane.h
+++ b/src/cascadia/TerminalApp/Pane.h
@@ -265,8 +265,6 @@ class Pane : public std::enable_shared_from_this<Pane>
     winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker;
     winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker;
 
-    std::shared_mutex _createCloseLock{};
-
     Borders _borders{ Borders::None };
 
     bool _zoomed{ false };
@@ -305,11 +303,11 @@ class Pane : public std::enable_shared_from_this<Pane>
                                             const PanePoint offset);
 
     void _CloseChild(const bool closeFirst, const bool isDetaching);
-    winrt::fire_and_forget _CloseChildRoutine(const bool closeFirst);
+    void _CloseChildRoutine(const bool closeFirst);
 
     void _Focus();
     void _FocusFirstChild();
-    void _ControlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/);
+    winrt::fire_and_forget _ControlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/);
     void _ControlWarningBellHandler(const winrt::Windows::Foundation::IInspectable& sender,
                                     const winrt::Windows::Foundation::IInspectable& e);
     void _ControlGotFocusHandler(const winrt::Windows::Foundation::IInspectable& sender,
diff --git a/src/cascadia/TerminalApp/TabBase.cpp b/src/cascadia/TerminalApp/TabBase.cpp
index 00aa0882e13..2d7293ccd6d 100644
--- a/src/cascadia/TerminalApp/TabBase.cpp
+++ b/src/cascadia/TerminalApp/TabBase.cpp
@@ -37,7 +37,6 @@ namespace winrt::TerminalApp::implementation
         ASSERT_UI_THREAD();
 
         Content(nullptr);
-        _ClosedHandlers(nullptr, nullptr);
     }
 
     // Method Description:
diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp
index 8d8cca21ef6..ef0093dc30d 100644
--- a/src/cascadia/TerminalApp/TabManagement.cpp
+++ b/src/cascadia/TerminalApp/TabManagement.cpp
@@ -249,10 +249,13 @@ namespace winrt::TerminalApp::implementation
         });
 
         // When the tab is closed, remove it from our list of tabs.
-        newTabImpl->Closed([tabViewItem, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) {
-            if (auto page{ weakThis.get() })
+        newTabImpl->Closed([weakTab, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) {
+            const auto page = weakThis.get();
+            const auto tab = weakTab.get();
+
+            if (page && tab)
             {
-                page->_RemoveOnCloseRoutine(tabViewItem, page);
+                page->_RemoveTab(*tab);
             }
         });
 
@@ -521,6 +524,11 @@ namespace winrt::TerminalApp::implementation
             _mruTabs.RemoveAt(mruIndex);
         }
 
+        if (tab == _settingsTab)
+        {
+            _settingsTab = nullptr;
+        }
+
         if (_stashed.draggedTab && *_stashed.draggedTab == tab)
         {
             _stashed.draggedTab = nullptr;
diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
index c9c31b85247..d4ec0666b68 100644
--- a/src/cascadia/TerminalApp/TerminalPage.cpp
+++ b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -1169,16 +1169,6 @@ namespace winrt::TerminalApp::implementation
         }
     }
 
-    winrt::fire_and_forget TerminalPage::_RemoveOnCloseRoutine(Microsoft::UI::Xaml::Controls::TabViewItem tabViewItem, winrt::com_ptr<TerminalPage> page)
-    {
-        co_await wil::resume_foreground(page->_tabView.Dispatcher());
-
-        if (auto tab{ _GetTabByTabViewItem(tabViewItem) })
-        {
-            _RemoveTab(tab);
-        }
-    }
-
     std::wstring TerminalPage::_evaluatePathForCwd(const std::wstring_view path)
     {
         return Utils::EvaluateStartingDirectory(_WindowProperties.VirtualWorkingDirectory(), path);
@@ -3851,11 +3841,13 @@ namespace winrt::TerminalApp::implementation
             });
 
             // When the tab is closed, remove it from our list of tabs.
-            newTabImpl->Closed([tabViewItem, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) {
-                if (auto page{ weakThis.get() })
+            newTabImpl->Closed([weakTab, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) {
+                const auto page = weakThis.get();
+                const auto tab = weakTab.get();
+
+                if (page && tab)
                 {
-                    page->_settingsTab = nullptr;
-                    page->_RemoveOnCloseRoutine(tabViewItem, page);
+                    page->_RemoveTab(*tab);
                 }
             });
 
diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h
index 2967954c343..2c0439f9d98 100644
--- a/src/cascadia/TerminalApp/TerminalPage.h
+++ b/src/cascadia/TerminalApp/TerminalPage.h
@@ -390,8 +390,6 @@ namespace winrt::TerminalApp::implementation
         winrt::Windows::Foundation::IAsyncOperation<bool> _PaneConfirmCloseReadOnly(std::shared_ptr<Pane> pane);
         void _AddPreviouslyClosedPaneOrTab(std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs>&& args);
 
-        winrt::fire_and_forget _RemoveOnCloseRoutine(Microsoft::UI::Xaml::Controls::TabViewItem tabViewItem, winrt::com_ptr<TerminalPage> page);
-
         void _Scroll(ScrollDirection scrollDirection, const Windows::Foundation::IReference<uint32_t>& rowsToScroll);
 
         void _SplitPane(const Microsoft::Terminal::Settings::Model::SplitDirection splitType,


Test patch: