Fix a crash when closing tabs #18620
Conversation
| @@ -851,6 +851,9 @@ namespace winrt::TerminalApp::implementation | |||
| { | |||
| ASSERT_UI_THREAD(); | |||
|
|
|||
| // Don't forget to call the overridden function. :) | |||
| TabBase::Shutdown(); | |||
There was a problem hiding this comment.
(FWIW this is a Crucial part of this PR as it sets the content to null. Also, TIL: We have a tab content that is different from the pane content. Was slightly confused by that naming.)
There was a problem hiding this comment.
@zadjii-msft I thought we merged terminaltab back into tabbase 😢
microsoft-github-policy-service
bot
added
Issue-Bug
| @@ -851,6 +851,9 @@ namespace winrt::TerminalApp::implementation | |||
| { | |||
| ASSERT_UI_THREAD(); | |||
|
|
|||
| // Don't forget to call the overridden function. :) | |||
| TabBase::Shutdown(); | |||
There was a problem hiding this comment.
@zadjii-msft I thought we merged terminaltab back into tabbase 😢
| @@ -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>()) | |||
There was a problem hiding this comment.
why the syntax change? was the previous one wrong?
There was a problem hiding this comment.
I believe we're alone in our usage of the auto foo{ ... } syntax. Not even the C++ Core Guidelines use it: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto
As you know, the brace-initializer syntax is historically overladen and rather brittle, so I see people recommend using it only for constructing explicitly named types, and I concur. In fact, since ~C++20 it's actually been going back from there with the consortium's trend towards "we can be like Rust if we have static analyzers", hence members recommending even things like int foo = bar(), despite the risk for unintentional narrowing.
lhecker
added
the
AutoMerge
microsoft-github-policy-service
bot
merged commit 62e7f4b
into
main
microsoft-github-policy-service
bot
deleted the
dev/lhecker/18581-tab-close-crash
branch
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 ✅ (cherry picked from commit 62e7f4b) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgXpjpM PVTI_lADOAF3p4s4AxadtzgXsRQM Service-Version: 1.23
|
Is this fix related to that crash #18458 ? |
WinUI asynchronously updates its tab view items, so it may happen that
we're given a
TabViewItemthat still contains aTabBasewhich hasactually already been removed. Regressed in #15924.
Closes #18581
Validation Steps Performed