mirror of
https://github.com/microsoft/terminal.git
synced 2025-12-10 00:48:23 -06:00
Fix a crash when closing panes (#17333)
Calling Close() from within WalkPanes is not safe. Simply using _FindPane is enough to fix this. This PR also fixes another potential source of infinite recursion, and fixes panes being passed by-value into the callbacks. Closes #17305 ## Validation Steps Performed * Split panes vertically 3 times * `exit` the middle, the bottom and final one, in that order * Doesn't crash ✅
This commit is contained in:
Leonard Hecker
GitHub
parent
bdc7c4fdbc
commit
baba406a07
@ -467,7 +467,7 @@ std::shared_ptr<Pane> Pane::NextPane(const std::shared_ptr<Pane> targetPane)
|
||||
std::shared_ptr<Pane> nextPane = nullptr;
|
||||
auto foundTarget = false;
|
||||
|
||||
auto foundNext = WalkTree([&](auto pane) {
|
||||
auto foundNext = WalkTree([&](const auto& pane) {
|
||||
// If we are a parent pane we don't want to move to one of our children
|
||||
if (foundTarget && targetPane->_HasChild(pane))
|
||||
{
|
||||
@ -985,6 +985,17 @@ void Pane::_ContentLostFocusHandler(const winrt::Windows::Foundation::IInspectab
|
||||
// - <none>
|
||||
void Pane::Close()
|
||||
{
|
||||
// Pane has two events, CloseRequested and Closed. CloseRequested is raised by the content asking to be closed,
|
||||
// but also by the window who owns the tab when it's closing. The event is then caught by the TerminalTab which
|
||||
// calls Close() which then raises the Closed event. Now, if this is the last pane in the window, this will result
|
||||
// in the window raising CloseRequested again which leads to infinite recursion, so we need to guard against that.
|
||||
// Ideally we would have just a single event in the future.
|
||||
if (_closed)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
_closed = true;
|
||||
// Fire our Closed event to tell our parent that we should be removed.
|
||||
Closed.raise(nullptr, nullptr);
|
||||
}
|
||||
@ -1341,7 +1352,7 @@ std::shared_ptr<Pane> Pane::DetachPane(std::shared_ptr<Pane> pane)
|
||||
detached->_ApplySplitDefinitions();
|
||||
|
||||
// Trigger the detached event on each child
|
||||
detached->WalkTree([](auto pane) {
|
||||
detached->WalkTree([](const auto& pane) {
|
||||
pane->Detached.raise(pane);
|
||||
});
|
||||
|
||||
@ -1542,7 +1553,7 @@ void Pane::_CloseChild(const bool closeFirst)
|
||||
{
|
||||
// update our path to our first remaining leaf
|
||||
_parentChildPath = _firstChild;
|
||||
_firstChild->WalkTree([](auto p) {
|
||||
_firstChild->WalkTree([](const auto& p) {
|
||||
if (p->_IsLeaf())
|
||||
{
|
||||
return true;
|
||||
@ -2398,7 +2409,7 @@ void Pane::Id(uint32_t id) noexcept
|
||||
bool Pane::FocusPane(const uint32_t id)
|
||||
{
|
||||
// Always clear the parent child path if we are focusing a leaf
|
||||
return WalkTree([=](auto p) {
|
||||
return WalkTree([=](const auto& p) {
|
||||
p->_parentChildPath.reset();
|
||||
if (p->_id == id)
|
||||
{
|
||||
@ -2421,7 +2432,7 @@ bool Pane::FocusPane(const uint32_t id)
|
||||
// - true if focus was set
|
||||
bool Pane::FocusPane(const std::shared_ptr<Pane> pane)
|
||||
{
|
||||
return WalkTree([&](auto p) {
|
||||
return WalkTree([&](const auto& p) {
|
||||
if (p == pane)
|
||||
{
|
||||
p->_Focus();
|
||||
|
||||
@ -248,7 +248,7 @@ private:
|
||||
|
||||
std::optional<uint32_t> _id;
|
||||
std::weak_ptr<Pane> _parentChildPath{};
|
||||
|
||||
bool _closed{ false };
|
||||
bool _lastActive{ false };
|
||||
winrt::event_token _firstClosedToken{ 0 };
|
||||
winrt::event_token _secondClosedToken{ 0 };
|
||||
|
||||
@ -739,7 +739,7 @@ namespace winrt::TerminalApp::implementation
|
||||
}
|
||||
|
||||
// Clean read-only mode to prevent additional prompt if closing the pane triggers closing of a hosting tab
|
||||
pane->WalkTree([](auto p) {
|
||||
pane->WalkTree([](const auto& p) {
|
||||
if (const auto control{ p->GetTerminalControl() })
|
||||
{
|
||||
if (control.ReadOnly())
|
||||
|
||||
@ -3812,7 +3812,7 @@ namespace winrt::TerminalApp::implementation
|
||||
// recipe for disaster. We won't ever open up a tab in this window.
|
||||
newTerminalArgs.Elevate(false);
|
||||
const auto newPane = _MakePane(newTerminalArgs, nullptr, connection);
|
||||
newPane->WalkTree([](auto pane) {
|
||||
newPane->WalkTree([](const auto& pane) {
|
||||
pane->FinalizeConfigurationGivenDefault();
|
||||
});
|
||||
_CreateNewTabFromPane(newPane);
|
||||
|
||||
@ -41,7 +41,7 @@ namespace winrt::TerminalApp::implementation
|
||||
|
||||
auto firstId = _nextPaneId;
|
||||
|
||||
_rootPane->WalkTree([&](std::shared_ptr<Pane> pane) {
|
||||
_rootPane->WalkTree([&](const auto& pane) {
|
||||
// update the IDs on each pane
|
||||
if (pane->_IsLeaf())
|
||||
{
|
||||
@ -203,7 +203,7 @@ namespace winrt::TerminalApp::implementation
|
||||
{
|
||||
ASSERT_UI_THREAD();
|
||||
|
||||
_rootPane->WalkTree([&](std::shared_ptr<Pane> pane) {
|
||||
_rootPane->WalkTree([&](const auto& pane) {
|
||||
// Attach event handlers to each new pane
|
||||
_AttachEventHandlersToPane(pane);
|
||||
if (auto content = pane->GetContent())
|
||||
@ -275,7 +275,7 @@ namespace winrt::TerminalApp::implementation
|
||||
_UpdateHeaderControlMaxWidth();
|
||||
|
||||
// Update the settings on all our panes.
|
||||
_rootPane->WalkTree([&](auto pane) {
|
||||
_rootPane->WalkTree([&](const auto& pane) {
|
||||
pane->UpdateSettings(settings);
|
||||
return false;
|
||||
});
|
||||
@ -534,7 +534,7 @@ namespace winrt::TerminalApp::implementation
|
||||
|
||||
// Add the new event handlers to the new pane(s)
|
||||
// and update their ids.
|
||||
pane->WalkTree([&](auto p) {
|
||||
pane->WalkTree([&](const auto& p) {
|
||||
_AttachEventHandlersToPane(p);
|
||||
if (p->_IsLeaf())
|
||||
{
|
||||
@ -624,7 +624,7 @@ namespace winrt::TerminalApp::implementation
|
||||
// manually.
|
||||
_rootPane->Closed(_rootClosedToken);
|
||||
auto p = _rootPane;
|
||||
p->WalkTree([](auto pane) {
|
||||
p->WalkTree([](const auto& pane) {
|
||||
pane->Detached.raise(pane);
|
||||
});
|
||||
|
||||
@ -650,7 +650,7 @@ namespace winrt::TerminalApp::implementation
|
||||
|
||||
// Add the new event handlers to the new pane(s)
|
||||
// and update their ids.
|
||||
pane->WalkTree([&](auto p) {
|
||||
pane->WalkTree([&](const auto& p) {
|
||||
_AttachEventHandlersToPane(p);
|
||||
if (p->_IsLeaf())
|
||||
{
|
||||
@ -949,26 +949,20 @@ namespace winrt::TerminalApp::implementation
|
||||
|
||||
events.CloseRequested = content.CloseRequested(
|
||||
winrt::auto_revoke,
|
||||
[dispatcher, weakThis](auto sender, auto&&) -> winrt::fire_and_forget {
|
||||
// Don't forget! this ^^^^^^^^ sender can't be a reference, this is a async callback.
|
||||
|
||||
// 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{ weakThisCopy.get() })
|
||||
[this](auto&& sender, auto&&) {
|
||||
if (const auto content{ sender.try_as<TerminalApp::IPaneContent>() })
|
||||
{
|
||||
if (const auto content{ sender.try_as<TerminalApp::IPaneContent>() })
|
||||
// Calling Close() while walking the tree is not safe, because Close() mutates the tree.
|
||||
const auto pane = _rootPane->_FindPane([&](const auto& p) -> std::shared_ptr<Pane> {
|
||||
if (p->GetContent() == content)
|
||||
{
|
||||
return p;
|
||||
}
|
||||
return {};
|
||||
});
|
||||
if (pane)
|
||||
{
|
||||
tab->_rootPane->WalkTree([content](std::shared_ptr<Pane> pane) {
|
||||
if (pane->GetContent() == content)
|
||||
{
|
||||
pane->Close();
|
||||
}
|
||||
});
|
||||
pane->Close();
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user