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: 91258717
Service-Version: 1.19
  • Loading branch information
lhecker authored and DHowett committed Dec 4, 2023
1 parent 85eee09 commit 486f7ef
Showing 1 changed file with 16 additions and 8 deletions.
24 changes: 16 additions & 8 deletions src/cascadia/TerminalApp/TerminalTab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -933,9 +933,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 @@ -944,8 +948,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 @@ -955,33 +960,36 @@ 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.stateToken = control.ConnectionStateChanged([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->_UpdateConnectionClosedState();
}
});

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 486f7ef

Please sign in to comment.