Fix crash when closing scratchpad pane#20195
Conversation
| // - <none> | ||
| void Pane::Close() | ||
| { | ||
| // GH#20187: Make Close() idempotent. The actual removal of this pane |
There was a problem hiding this comment.
It's OK for us to leave briefer comments and not refer to github issues.
This whole thing could be...
// Make Close() idempotent
or just no comment
(because the current comment encodes implementation details from everywhere else in the project and will be incorrect the moment somebody fixes the double-close bug)
| // shortcut, focus moves off the closed pane while keys | ||
| // are still depressed. The corresponding KeyUp events | ||
| // arrive at TabRowControl, whose KeyUp is wired to | ||
| // TerminalPage::_KeyDownHandler (see TerminalPage.xaml). | ||
| // _KeyDownHandler can't tell it was invoked from KeyUp | ||
| // and dispatches the close action a second time. By | ||
| // then the parent's close animation in | ||
| // _CloseChildRoutine is still running, so | ||
| // Tab::_activePane still points at the now-closed leaf | ||
| // ("corpse"). Detect that here and bail before we | ||
| // dereference its null content in GetTerminalArgsForPane. |
| // Tab::_activePane still points at the now-closed leaf | ||
| // ("corpse"). Detect that here and bail before we | ||
| // dereference its null content in GetTerminalArgsForPane. | ||
| if (pane->IsClosed()) |
There was a problem hiding this comment.
it should be sufficient to fix Close, right? I know this is "defense in depth" but it feels wrong to add a public API to pane to say "Are you closed?"
In general, we should not need to interrogate objects to say "are you half-dead?" because in the best case scenario they're HALF DEAD and should not be answering questions...
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
Bot
added
the
No-Recent-Activity
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
2 participants
Summary of the Pull Request
Fixes a crash when closing a scratchpad pane. The crash would occur in
Pane::GetTerminalArgsForPane()via a null-pointer exception on_contentwhen calling_content.GetNewTerminalArgs(kind);.My root cause analysis revealed that this was happening:
Pane::Close()is synchronously called:_contentis cleared andClosedis raised_CloseChildRoutine) performs a close animation, and then calls_CloseChildto actually mutate the pane tree once the animation is complete._isLeaf()is true_lastActiveis trueTab::_activePanein the tab'sClosedhandler_activePaneonly updates when a parent of the active pane closes, never when the active pane itself closes_contentis nullclosePaneis dispatched due to the ctrl+shift+w keypress. This is due to PR Add a keyboard shortcut handler to the TabRowControl #12260 (ugh). This occurs during the animation!_CloseFocusedPanecallsactiveTab->GetActivePane()which returns the "corpse"_activePane._HandleClosePaneRequestedthen callsGetTerminalArgsForPane(), which dereferences the null_content, and the crash occurs.The minimum fix is:
Pane::IsClosed()helperTerminalPage::_CloseFocusedPane: return early if pane is closedThe other changes are additional safeguards so that this doesn't happen in the future.
Validation Steps Performed
✅ Close scratchpad pane with ctrl+shift+w --> no crash!
PR Checklist
Closes #20187