Skip to content

Commit

Permalink
Ignore closeOnExit when a conn. moves from Connecting to Failed (#10263)
Browse files Browse the repository at this point in the history
ConptyConnection has two different failure modes:

1. We failed to initialize the pseudoconsole or create the process
2. The process exited with an error code.

Until this commit, they were treated the same way: closeOnExit=always
would force the pane/tab to be destroyed. This was very bad in case 1,
where we would display a (possibly useful) error message and then
immediately close the window.

This was made even worse by the change in #10045. We removed
startingDirectory validation and promoted it to an error message (so
that we could eventually let the connection handle startingDirectory in
its own way.) This of course revealed that a number of users had set
invalid starting directories… and those users included some who set
closeOnExit to always. Boom: instant "terminal opens and crashes"¹

In this commit, we introduce detection for a connection that fails
before it's been established. When that happens, we will ignore the
user's closeOnExit mode.

¹ It only looks like a crash; it's actually _technically_ functioning
properly.

Closes #10225.

(cherry picked from commit 31d78dc)
  • Loading branch information
DHowett committed May 28, 2021
1 parent fbc59b7 commit 3533aa2
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -329,13 +329,22 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
}

const auto newConnectionState = _control.ConnectionState();
const auto previousConnectionState = std::exchange(_connectionState, newConnectionState);

if (newConnectionState < ConnectionState::Closed)
{
// Pane doesn't care if the connection isn't entering a terminal state.
return;
}

if (previousConnectionState < ConnectionState::Connected && newConnectionState >= ConnectionState::Failed)
{
// A failure to complete the connection (before it has _connected_) is not covered by "closeOnExit".
// This is to prevent a misconfiguration (closeOnExit: always, startingDirectory: garbage) resulting
// in Terminal flashing open and immediately closed.
return;
}

const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
auto paneProfile = settings.FindProfile(_profile.value());
if (paneProfile)
Expand Down Expand Up @@ -709,6 +718,7 @@ void Pane::_CloseChild(const bool closeFirst)

// take the control, profile and id of the pane that _wasn't_ closed.
_control = remainingChild->_control;
_connectionState = remainingChild->_connectionState;
_profile = remainingChild->_profile;
_id = remainingChild->Id();

Expand Down Expand Up @@ -1476,6 +1486,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
// Move our control, guid into the first one.
// Move the new guid, control into the second.
_firstChild = std::make_shared<Pane>(_profile.value(), _control);
_firstChild->_connectionState = std::exchange(_connectionState, ConnectionState::NotConnected);
_profile = std::nullopt;
_control = { nullptr };
_secondChild = std::make_shared<Pane>(profile, control);
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/Pane.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ class Pane : public std::enable_shared_from_this<Pane>
winrt::Windows::UI::Xaml::Controls::Grid _root{};
winrt::Windows::UI::Xaml::Controls::Border _border{};
winrt::Microsoft::Terminal::Control::TermControl _control{ nullptr };
winrt::Microsoft::Terminal::TerminalConnection::ConnectionState _connectionState{ winrt::Microsoft::Terminal::TerminalConnection::ConnectionState::NotConnected };
static winrt::Windows::UI::Xaml::Media::SolidColorBrush s_focusedBorderBrush;
static winrt::Windows::UI::Xaml::Media::SolidColorBrush s_unfocusedBorderBrush;

Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalConnection/ConptyConnection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
void ConptyConnection::Start()
try
{
_transitionToState(ConnectionState::Connecting);

if (!_inPipe)
{
const COORD dimensions{ gsl::narrow_cast<SHORT>(_initialCols), gsl::narrow_cast<SHORT>(_initialRows) };
Expand Down

0 comments on commit 3533aa2

Please sign in to comment.