microsoft/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2025-12-11 04:38:24 -06:00
Code Issues Packages Projects Releases 138 Wiki Activity

Avoid async infinite loop on tab close (#15335)

Browse Source 
When a tab gets closed, `_RemoveTab` will call `TabBase::Shutdown()`,
which then re-raises the `Closed` event, which will end up calling
`_RemoveTab` again, etc. The only reason this didn't crash WT so far
is because `_RemoveOnCloseRoutine` contains a `resume_foreground`,
which would resolve the recursion and turn it into CPU usage.
It would spin as long as WinUI hasn't discard the tab object,
which takes an unpredictable amount of time.

Raising the `Closed` event from `Shutdown()` is unnecessary, because
the handlers of the event end up calling `_RemoveTab` anyways.
Technically the entire `Closed` event can be removed now, but I left it
in anyways because resolving the architectural "knot" around the way
tab closing after the last pane closes is implemented requires much
more significant changes.

This commit additionally removes the `_createCloseLock` mutex in `Pane`
as it was very likely not working as intended anyways. Only some methods
were protected by it and it doesn't avoid any STA/MTA/NA issues either.

## Validation Steps Performed
* Closing tabs and panes always ends up calling `Shutdown()` ✅
This commit is contained in:
Leonard Hecker 2023-08-15 21:12:19 +02:00 committed by GitHub
parent 127df073fa
commit 3701d0ee95
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 163 additions and 180 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

300
src/cascadia/TerminalApp/Pane.cpp
View File

@ -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() };
// 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();
co_await wil::resume_foreground(_root.Dispatcher());
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;
}
// Setup the animation
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())
};
// 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);
}
// 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);
// 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);
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();
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);
}
});
_CloseChild(closeFirst, false);
return;
}
// Setup the animation
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())
};
// 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);
}
// 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);
// 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);
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();
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);
}
});
}
// 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.

6
src/cascadia/TerminalApp/Pane.h
View File

@ -265,8 +265,6 @@ private:
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 @@ private:
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,

1
src/cascadia/TerminalApp/TabBase.cpp
View File

@ -37,7 +37,6 @@ namespace winrt::TerminalApp::implementation
ASSERT_UI_THREAD();
Content(nullptr);
_ClosedHandlers(nullptr, nullptr);
}
// Method Description:

14
src/cascadia/TerminalApp/TabManagement.cpp
View File

@ -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;

20
src/cascadia/TerminalApp/TerminalPage.cpp
View File

@ -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);
}
});

2
src/cascadia/TerminalApp/TerminalPage.h
View File

@ -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,