Skip to content

Commit

Permalink
Fix pane event handlers being unbound (#17750)
Browse files Browse the repository at this point in the history
I don't know what has changed between #17450 and now, but that fix
doesn't seem necessary anymore. If you add this action:
```json
{
    "keys": "ctrl+a",
    "command":
    {
        "action": "splitPane",
        "commandline": "cmd /c exit"
    }
}
```

and repeatedly spam Ctrl-A it used to lead to crashes. That doesn't
happen anymore, because some other PR must've fixed that.

Reverting #17450 fixes the issue found in #17578: Because the content
pointer didn't get reset to null anymore it meant that the root
pane retained the pointer after a split. After closing the split off
pane, it would assign the remaining one back to the root, which would
cause the still existing content pointer to be closed. That pointer
is potentially the same as the remaining pane and so no close events
would get received anymore.

Closes #17578

## Validation Steps Performed
* Add the above action and spam it ✅
* Start with an empty window, split pane, type `exit` in the new pane
  then type it in the original pane. It closes the window ✅
  • Loading branch information
lhecker authored Aug 21, 2024
1 parent 7b39d24 commit 056af83
Showing 1 changed file with 3 additions and 5 deletions.
8 changes: 3 additions & 5 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1711,9 +1711,7 @@ void Pane::_SetupChildCloseHandlers()
IPaneContent Pane::_takePaneContent()
{
_closeRequestedRevoker.revoke();
// we cannot return std::move(_content) because we don't want _content to be null,
// since _content gets accessed even after Close is called
return _content;
return std::move(_content);
}

// This method safely sets the content of the Pane. It'll ensure to revoke and
Expand All @@ -1723,9 +1721,9 @@ 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 (_takePaneContent())
if (const auto c = _takePaneContent())
{
_content.Close();
c.Close();
}

if (content)
Expand Down

0 comments on commit 056af83

Please sign in to comment.