Skip to content

Commit

Permalink
Fix a coroutine AV crash (#16412)
Browse files Browse the repository at this point in the history
tl;dr: A coroutine lambda does not hold onto captured variables.
This causes an AV crash when closing tabs. I randomly noticed this
in a Debug build as the memory contents got replaced with 0xCD.
In a Release build this bug is probably fairly subtle and not common.

(cherry picked from commit 91fd7d0)
Service-Card-Id: 91258716
Service-Version: 1.18
  • Loading branch information
lhecker authored and DHowett committed Jan 26, 2024
1 parent 80de11b commit 7dc278c
Showing 1 changed file with 14 additions and 7 deletions.
21 changes: 14 additions & 7 deletions src/cascadia/TerminalApp/TerminalTab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -909,9 +909,13 @@ namespace winrt::TerminalApp::implementation
ControlEventTokens events{};

events.titleToken = control.TitleChanged([dispatcher, weakThis](auto&&, auto&&) -> winrt::fire_and_forget {
// The lambda lives in the `std::function`-style container owned by `control`. That is, when the
// `control` gets destroyed the lambda struct also gets destroyed. In other words, we need to
// copy `weakThis` onto the stack, because that's the only thing that gets captured in coroutines.
// See: https://devblogs.microsoft.com/oldnewthing/20211103-00/?p=105870
const auto weakThisCopy = weakThis;
co_await wil::resume_foreground(dispatcher);
// Check if Tab's lifetime has expired
if (auto tab{ weakThis.get() })
if (auto tab{ weakThisCopy.get() })
{
// The title of the control changed, but not necessarily the title of the tab.
// Set the tab's text to the active panes' text.
Expand All @@ -920,8 +924,9 @@ namespace winrt::TerminalApp::implementation
});

events.colorToken = control.TabColorChanged([dispatcher, weakThis](auto&&, auto&&) -> winrt::fire_and_forget {
const auto weakThisCopy = weakThis;
co_await wil::resume_foreground(dispatcher);
if (auto tab{ weakThis.get() })
if (auto tab{ weakThisCopy.get() })
{
// The control's tabColor changed, but it is not necessarily the
// active control in this tab. We'll just recalculate the
Expand All @@ -931,25 +936,27 @@ namespace winrt::TerminalApp::implementation
});

events.taskbarToken = control.SetTaskbarProgress([dispatcher, weakThis](auto&&, auto&&) -> winrt::fire_and_forget {
const auto weakThisCopy = weakThis;
co_await wil::resume_foreground(dispatcher);
// Check if Tab's lifetime has expired
if (auto tab{ weakThis.get() })
if (auto tab{ weakThisCopy.get() })
{
tab->_UpdateProgressState();
}
});

events.readOnlyToken = control.ReadOnlyChanged([dispatcher, weakThis](auto&&, auto&&) -> winrt::fire_and_forget {
const auto weakThisCopy = weakThis;
co_await wil::resume_foreground(dispatcher);
if (auto tab{ weakThis.get() })
if (auto tab{ weakThisCopy.get() })
{
tab->_RecalculateAndApplyReadOnly();
}
});

events.focusToken = control.FocusFollowMouseRequested([dispatcher, weakThis](auto sender, auto) -> winrt::fire_and_forget {
const auto weakThisCopy = weakThis;
co_await wil::resume_foreground(dispatcher);
if (const auto tab{ weakThis.get() })
if (const auto tab{ weakThisCopy.get() })
{
if (tab->_focused())
{
Expand Down

0 comments on commit 7dc278c

Please sign in to comment.