Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -997,6 +997,16 @@ void Pane::_ContentLostFocusHandler(const winrt::Windows::Foundation::IInspectab
// - <none>
void Pane::Close()
{
// GH#20187: Make Close() idempotent. The actual removal of this pane

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

// from the parent's tree is deferred until the close animation in
// _CloseChildRoutine completes (~200ms). Without this guard, a
// second close request that reaches this same pane during the
// animation window would re-raise Closed and kick off a redundant
// close animation on the parent.
if (!_content)
{
return;
}
_setPaneContent(nullptr);
// Fire our Closed event to tell our parent that we should be removed.
Closed.raise(nullptr, nullptr);
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalApp/Pane.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ class Pane : public std::enable_shared_from_this<Pane>
winrt::Windows::UI::Xaml::Controls::Grid GetRootElement();
winrt::TerminalApp::IPaneContent GetContent() const noexcept { return _IsLeaf() ? _content : nullptr; }

// GH#20187: True if this is a leaf pane whose content has already been
// torn down by Pane::Close() but which has not yet been removed from
// its parent (the close animation in _CloseChildRoutine is still
// running). Such a "corpse" pane is not safe to operate on.
bool IsClosed() const noexcept { return _IsLeaf() && _content == nullptr; }

bool WasLastFocused() const noexcept;
void UpdateVisuals();
void ClearActive();
Expand Down
17 changes: 17 additions & 0 deletions src/cascadia/TerminalApp/TabManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,23 @@ namespace winrt::TerminalApp::implementation

if (const auto pane{ activeTab->GetActivePane() })
{
// GH#20187: When the user closes a pane with a keyboard
// 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.
Comment on lines +818 to +828

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

way too much detail

if (pane->IsClosed())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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...

{
co_return;
}

const auto weak = get_weak();

// Check if we should warn before closing a single pane
Expand Down
Loading