Skip to content

Commit

Permalink
Fix a crash when closing tabs (#18620)
Browse files Browse the repository at this point in the history
WinUI asynchronously updates its tab view items, so it may happen that
we're given a `TabViewItem` that still contains a `TabBase` which has
actually already been removed. Regressed in #15924.

Closes #18581

## Validation Steps Performed
* Close tabs rapidly with middle click
* No crash ✅
  • Loading branch information
lhecker authored Feb 24, 2025
1 parent 265d841 commit 62e7f4b
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 4 deletions.
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TabBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ namespace winrt::TerminalApp::implementation
{
ASSERT_UI_THREAD();

// NOTE: `TerminalPage::_HandleCloseTabRequested` relies on the content being null after this call.
Content(nullptr);
}

Expand Down
30 changes: 26 additions & 4 deletions src/cascadia/TerminalApp/TabManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ namespace winrt::TerminalApp::implementation
// Set this tab's icon to the icon from the content
_UpdateTabIcon(*newTabImpl);

// This is necessary, because WinUI does not have support for middle clicks.
// Its Tapped event doesn't provide the information what button was used either.
tabViewItem.PointerPressed({ this, &TerminalPage::_OnTabPointerPressed });
tabViewItem.PointerReleased({ this, &TerminalPage::_OnTabPointerReleased });
tabViewItem.PointerExited({ this, &TerminalPage::_OnTabPointerExited });
Expand Down Expand Up @@ -903,19 +905,39 @@ namespace winrt::TerminalApp::implementation
if (_tabPointerMiddleButtonPressed && !eventArgs.GetCurrentPoint(nullptr).Properties().IsMiddleButtonPressed())
{
_tabPointerMiddleButtonPressed = false;
if (const auto tabViewItem{ sender.try_as<MUX::Controls::TabViewItem>() })
if (auto tabViewItem{ sender.try_as<MUX::Controls::TabViewItem>() })
{
tabViewItem.ReleasePointerCapture(eventArgs.Pointer());
auto tab = _GetTabByTabViewItem(tabViewItem);
if (!_tabPointerMiddleButtonExited && tab)
if (!_tabPointerMiddleButtonExited)
{
_HandleCloseTabRequested(tab);
_OnTabPointerReleasedCloseTab(std::move(tabViewItem));
}
}
eventArgs.Handled(true);
}
}

safe_void_coroutine TerminalPage::_OnTabPointerReleasedCloseTab(winrt::Microsoft::UI::Xaml::Controls::TabViewItem sender)
{
const auto tab = _GetTabByTabViewItem(sender);
if (!tab)
{
co_return;
}

// WinUI asynchronously updates its tab view items, so it may happen that we're given a
// `TabViewItem` that still contains a `TabBase` which has actually already been removed.
// First we must yield once, to flush out whatever TabView is currently doing.
const auto strong = get_strong();
co_await wil::resume_foreground(Dispatcher());

// `tab.Shutdown()` in `_RemoveTab()` sets the content to null = This checks if the tab is closed.
if (tab.Content())
{
_HandleCloseTabRequested(tab);
}
}

// Method Description:
// - Tracking pointer state for tab remove
// Arguments:
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ namespace winrt::TerminalApp::implementation
bool _tabPointerMiddleButtonExited{ false };
void _OnTabPointerPressed(const IInspectable& sender, const Windows::UI::Xaml::Input::PointerRoutedEventArgs& eventArgs);
void _OnTabPointerReleased(const IInspectable& sender, const Windows::UI::Xaml::Input::PointerRoutedEventArgs& eventArgs);
safe_void_coroutine _OnTabPointerReleasedCloseTab(winrt::Microsoft::UI::Xaml::Controls::TabViewItem sender);
void _OnTabPointerEntered(const IInspectable& sender, const Windows::UI::Xaml::Input::PointerRoutedEventArgs& eventArgs);
void _OnTabPointerExited(const IInspectable& sender, const Windows::UI::Xaml::Input::PointerRoutedEventArgs& eventArgs);

Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalApp/TerminalTab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,9 @@ namespace winrt::TerminalApp::implementation
{
ASSERT_UI_THREAD();

// Don't forget to call the overridden function. :)
TabBase::Shutdown();

if (_rootPane)
{
_rootPane->Shutdown();
Expand Down

0 comments on commit 62e7f4b

Please sign in to comment.