Skip to content
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

Fix crash when closing panes very quickly #17450

Merged
merged 3 commits into from
Jun 20, 2024
Merged

Fix crash when closing panes very quickly #17450

merged 3 commits into from
Jun 20, 2024

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Jun 19, 2024

#17358 introduced a bug where if you open/close panes very rapidly Terminal will crash. This was because _content was being set to null and then a Close event was being emitted, but several functions attempt to access the pane's _content as part of the close routine. For example, TerminalTab tries to update the TaskbarProgress every time a pane is closed and as part of that update sequence it queries the pane - which has null content now - for the taskbar progress, resulting in a crash. This PR fixes that crash.

Refs #17358

@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Jun 19, 2024

This change works fully when animations are disabled, however when pane animations are enabled and you rapidly open/close panes, we could end up in a state like this, where the pane's content is gone but doesn't end up getting removed from the UI tree (not a crash though):
image

Still working on remedying that, @lhecker let me know if you have thoughts on how to approach this

@PankajBhojwani PankajBhojwani marked this pull request as draft June 19, 2024 18:39
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!

@@ -1711,7 +1719,7 @@ void Pane::_SetupChildCloseHandlers()
IPaneContent Pane::_takePaneContent()
{
_closeRequestedRevoker.revoke();
return std::move(_content);
return _content ? std::move(_content) : nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the old code should work even if _content == nullptr. It's like moving an empty vector<>.

@lhecker lhecker self-requested a review June 19, 2024 19:24
@lhecker
Copy link
Member

lhecker commented Jun 19, 2024

Ah lol, I started reviewing the PR straight from the mail notification and didn't notice your comment.

Did you try adding a co_await resume_foreground() into the CloseRequested callback here?

_closeRequestedRevoker = _content.CloseRequested(winrt::auto_revoke, [this](auto&&, auto&&) { Close(); });

Another thing I can think of is this place:

if (const auto c = _takePaneContent())
{
c.Close();

What if the call into IContent*::Close results in a callback back into the pane? Now that the _content has been "taken" (= nullptr) it may result in those issues you're trying to address in this PR. If that's the case, maybe we should simply not "take" the _content when calling close? Something like this:

void Pane::_setPaneContent(IPaneContent content)
{
    // Prevent reentrancy with the extra check?
    // Need to add _deferredClose as a new member.
    if (_content && _deferredClose != _content)
    {
        _deferredClose = _content;
        _content.Close();
        if (_deferredClose == _content)
        {
            _deferredClose = nullptr;
        }
    }

    // ...
}

@PankajBhojwani PankajBhojwani marked this pull request as ready for review June 20, 2024 01:29
@DHowett DHowett added this pull request to the merge queue Jun 20, 2024
Merged via the queue into main with commit 5d46e31 Jun 20, 2024
28 of 29 checks passed
@DHowett DHowett deleted the dev/pabhoj/pane_crash branch June 20, 2024 16:51
DHowett pushed a commit that referenced this pull request Jun 20, 2024
#17358 introduced a bug where if you open/close panes very rapidly
Terminal will crash. This was because `_content` was being set to `null`
and then a `Close` event was being emitted, but several functions
attempt to access the pane's `_content` as part of the close routine.
For example, `TerminalTab` tries to update the `TaskbarProgress` every
time a pane is closed and as part of that update sequence it queries the
pane - which has `null` content now - for the taskbar progress,
resulting in a crash. This PR fixes that crash.

Refs #17358

(cherry picked from commit 5d46e31)
Service-Card-Id: 92776947
Service-Version: 1.21
lhecker added a commit that referenced this pull request Aug 21, 2024
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 ✅
DHowett pushed a commit that referenced this pull request Aug 21, 2024
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 ✅

(cherry picked from commit 056af83)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSGCfE
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants