-
Notifications
You must be signed in to change notification settings - Fork 8.7k
Fix crash when closing multiple panes simultaneously #19023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -1402,6 +1402,13 @@ void Pane::_CloseChild(const bool closeFirst) | |||
|
|||
// take the control, profile, id and isDefTermSession of the pane that _wasn't_ closed. | |||
_setPaneContent(remainingChild->_takePaneContent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still call _setPaneContent
if _takePaneContent
returned null?
(Thanks for fixing this btw!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terminal/src/cascadia/TerminalApp/Pane.cpp
Lines 1720 to 1734 in ad14922
void Pane::_setPaneContent(IPaneContent content) | |
{ | |
// The IPaneContent::Close() implementation may be buggy and raise the CloseRequested event again. | |
// _takePaneContent() avoids this as it revokes the event handler. | |
if (const auto c = _takePaneContent()) | |
{ | |
c.Close(); | |
} | |
if (content) | |
{ | |
_content = std::move(content); | |
_closeRequestedRevoker = _content.CloseRequested(winrt::auto_revoke, [this](auto&&, auto&&) { Close(); }); | |
} | |
} |
The whole function results in a no-op, so we can totally split this up into:
const auto& content = remainingChild->_takePaneContent();
if (!content)
{
Closed.raise(nullptr, nullptr);
return;
}
_setPaneContent(content);
Up to you. I'm indifferent to it, personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might actually need the _setPaneContent
call because that's what calls Close
on our current pane content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yey
Fixes a crash when multiple panes were closed simultaneously (i.e. using broadcast input). The root cause of this crash was that we would get a null pointer exception when trying to access a member/function off of a null `_content`. This is because `Pane::_CloseChild()` would always pass over the content from the non-closed pane and attempt to hook everything up to it. The fix was to operate similarly to `Pane::Close()` and raise a `Closed` event and return early. Since there's no alternative content to attach to, might as well just close the entire pane. This propagates up the stack of listeners to update the UI appropriately and close the parent pane and eventually the entire tab, if necessary. Closes #18071 Closes #17432 ## Validation Steps Performed 1. Open 2 panes 2. Use broadcast input to send "exit" to both panes 3. ✅ Terminal doesn't crash and the tab closes gracefully (cherry picked from commit 6bf315a) Service-Card-Id: PVTI_lADOAF3p4s4Axadtzgb3LNk Service-Version: 1.23
Fixes a crash when multiple panes were closed simultaneously (i.e. using broadcast input). The root cause of this crash was that we would get a null pointer exception when trying to access a member/function off of a null `_content`. This is because `Pane::_CloseChild()` would always pass over the content from the non-closed pane and attempt to hook everything up to it. The fix was to operate similarly to `Pane::Close()` and raise a `Closed` event and return early. Since there's no alternative content to attach to, might as well just close the entire pane. This propagates up the stack of listeners to update the UI appropriately and close the parent pane and eventually the entire tab, if necessary. Closes #18071 Closes #17432 ## Validation Steps Performed 1. Open 2 panes 2. Use broadcast input to send "exit" to both panes 3. ✅ Terminal doesn't crash and the tab closes gracefully (cherry picked from commit 6bf315a) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgb3LNo Service-Version: 1.22
Summary of the Pull Request
Fixes a crash when multiple panes were closed simultaneously (i.e. using broadcast input).
The root cause of this crash was that we would get a null pointer exception when trying to access a member/function off of a null
_content
. This is becausePane::_CloseChild()
would always pass over the content from the non-closed pane and attempt to hook everything up to it.The fix was to operate similarly to
Pane::Close()
and raise aClosed
event and return early. Since there's no alternative content to attach to, might as well just close the entire pane. This propagates up the stack of listeners to update the UI appropriately and close the parent pane and eventually the entire tab, if necessary.Closes #18071
Closes #17432
Validation Steps Performed