Repository: microsoft/terminal (language: cpp)

Issue:
Terminal crashes when exiting a bash shell
### Windows Terminal version

1.15.1862.0

### Windows build number

10.0.19044.1889

### Other Software

WSL with Ubuntu 20.04.4 LTS

### Steps to reproduce

1. Start Windows Terminal
2. Open two tabs, one of which is a bash shell.
3. Exit the bash shell (with `exit`).


### Expected Behavior

The terminal should remain open after the bash shell exits, with the other tab still working.

### Actual Behavior

The terminal crashes.

I was able to reproduce this in the debugger with a build derived from commit c12987af415c5e0911d7a0a81b8494fbe6307328, and the stack trace for the crash looked like this:

	ntdll.dll!TppRaiseInvalidParameter()	Unknown
 	ntdll.dll!TppTimerpValidateTimer()	Unknown
 	ntdll.dll!TpSetTimerEx()	Unknown
 	Microsoft.Terminal.Control.dll!winrt::Microsoft::Terminal::Control::implementation::ControlCore::_terminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize) Line 1308	C++
 	[External Code]	
 	Microsoft.Terminal.Control.dll!Microsoft::Terminal::Core::Terminal::_NotifyScrollEvent() Line 1305	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::VirtualTerminal::AdaptDispatch::_EraseScrollback() Line 1952	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::VirtualTerminal::AdaptDispatch::EraseInDisplay(const Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) Line 614	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::VirtualTerminal::OutputStateMachineEngine::ActionCsiDispatch(const Microsoft::Console::VirtualTerminal::VTID id, const Microsoft::Console::VirtualTerminal::VTParameters parameters) Line 492	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::VirtualTerminal::StateMachine::_SafeExecute<`Microsoft::Console::VirtualTerminal::StateMachine::_ActionCsiDispatch'::`2'::<lambda_1>>(Microsoft::Console::VirtualTerminal::StateMachine::_ActionCsiDispatch::__l2::<lambda_1> && lambda) Line 2039	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::VirtualTerminal::StateMachine::_ActionCsiDispatch(const wchar_t wch) Line 480	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::VirtualTerminal::StateMachine::ProcessCharacter(const wchar_t wch) Line 1778	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::VirtualTerminal::StateMachine::ProcessString(const std::basic_string_view<wchar_t,std::char_traits<wchar_t>> string) Line 1853	C++
 	Microsoft.Terminal.Control.dll!winrt::Microsoft::Terminal::Control::implementation::ControlCore::_connectionOutputHandler(const winrt::hstring & hstr) Line 1706	C++
 	Microsoft.Terminal.Control.dll!winrt::impl::delegate<winrt::Microsoft::Terminal::TerminalConnection::TerminalOutputHandler,`winrt::Microsoft::Terminal::TerminalConnection::TerminalOutputHandler::implementation<winrt::Microsoft::Terminal::Control::implementation::ControlCore,void (__cdecl winrt::Microsoft::Terminal::Control::implementation::ControlCore::*)(winrt::hstring const &)>'::`1'::<lambda_218_>>::Invoke(void * output) Line 174	C++
 	[External Code]	

The problem, as far as I can make out, is that the `ControlCore` instance is deleted at this point in time. If you look at `ControlCore::Close` method, we're closing the connection asynchronously, which I think means we're still receiving VT sequences (in this case `\e[3J`) after `ControlCore` is gone. Assuming I've understood that correctly, that would probably explain the crash.


Discussion / hints:
This might be a duplicate of #12068. Possibly also #13337 (they initially said they weren't doing anything when the terminal crashed, but later crashes occurred when "attempting to exit restored tabs" which sounds similar to what I'm seeing).

I'm surprised we don't have had more reports like this, though, because it's really easy for me to produce, but I guess there's an element of timing involved. I've also only noticed this recently, so I'm wondering if there's something we've changed that has made it easier to trigger.
This commit seems to be related (although I don't immediately see why that would be): https://github.com/microsoft/terminal/commit/23e4d313d5dae913ed16db3cfdb318a05b63ba6a

It tries to work around a deadlock issue by calling `UpdatePatternLocations` from a background thread. Even if I don't see how it might cause this, it does seem to be related because:
* It's a recent change (included in https://github.com/microsoft/terminal/commit/c12987af415c5e0911d7a0a81b8494fbe6307328)
* It's in `ControlCore`
* `_updatePatternLocations` is called in `_terminalScrollPositionChanged` which calls `TpSetTimerEx`

This PR is my attempt to fix one bug caused by that commit: https://github.com/microsoft/terminal/pull/13859
But yours seems to be another: The `TppRaiseInvalidParameter` call seems like an issue that might be caused if the `til::throttled_func` has been destroyed, making the `wil::unique_threadpool_timer` handle invalid. But why would this have worked before? The only difference I can think of is that the previous `ThrottledFunc` uses a `shared_ptr` to keep itself alive until the scheduled callback returns, but I don't see how that would be related here since no such `ThrottledFunc` is involved in `_connectionOutputHandler`. Did we just coincidentally get lucky up until now?

In either case _I believe_ the underlying issue is this: https://github.com/microsoft/terminal/blob/a85d9e69ed80bdce6abb3366a0d7e842731e60e7/src/cascadia/TerminalControl/ControlCore.cpp#L99-L100

The comment is wrong. Just because an event has been revoked, doesn't mean it's not currently being called by another thread, right? `TerminalOutput` is called from a background thread and so the callback's execution might outlive the main thread's strong reference to the `ControlCore` instance. The fix can't be to just use `get_weak()` instead of `this`, because `ControlCore` is WinUI-land and thus inherently thread-unsafe and can't be destroyed on a background thread. If I'm correct we have to block in `~ControlCore` until the `ITerminalConnection` can promise that all calls to `TerminalOutput` have returned.
> This commit seems to be related (although I don't immediately see why that would be): [23e4d31](https://github.com/microsoft/terminal/commit/23e4d313d5dae913ed16db3cfdb318a05b63ba6a)

I don't think that's to blame, because I'm also getting this crash in preview release 1.15.1862.0, which I think predates that commit.

> Did we just coincidentally get lucky up until now?

I was wondering that too. I don't fully understand what's going on in that code, but my initial impression was that it shouldn't ever have worked.

> If I'm correct we have to block in `~ControlCore` until the `ITerminalConnection` can promise that all calls to `TerminalOutput` have returned.

Yeah, I was thinking the same thing.

Fix patch:
diff --git a/src/cascadia/TerminalConnection/AzureConnection.cpp b/src/cascadia/TerminalConnection/AzureConnection.cpp
index 21ec8639f6c..f04e08bd74b 100644
--- a/src/cascadia/TerminalConnection/AzureConnection.cpp
+++ b/src/cascadia/TerminalConnection/AzureConnection.cpp
@@ -264,13 +264,14 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
             if (_state == AzureState::TermConnected)
             {
                 // Close the websocket connection
-                auto closedTask = _cloudShellSocket.close();
-                closedTask.wait();
+                _cloudShellSocket.close();
             }
 
             if (_hOutputThread)
             {
-                // Tear down our output thread
+                // Waiting for the output thread to exit ensures that all pending _TerminalOutputHandlers()
+                // calls have returned and won't notify our caller (ControlCore) anymore. This ensures that
+                // we don't call a destroyed event handler asynchronously from a background thread (GH#13880).
                 WaitForSingleObject(_hOutputThread.get(), INFINITE);
                 _hOutputThread.reset();
             }
diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp
index 2f0b55ca9c5..1f26ee965d2 100644
--- a/src/cascadia/TerminalConnection/ConptyConnection.cpp
+++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp
@@ -547,7 +547,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         if (_transitionToState(ConnectionState::Closing))
         {
             // EXIT POINT
-            _clientExitWait.reset(); // immediately stop waiting for the client to exit.
+
+            // _clientExitWait holds a CreateThreadpoolWait() which holds a weak reference to "this".
+            // This manual reset() ensures we wait for it to be teared down via WaitForThreadpoolWaitCallbacks().
+            _clientExitWait.reset();
 
             _hPC.reset(); // tear down the pseudoconsole (this is like clicking X on a console window)
 
@@ -556,19 +559,13 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 
             if (_hOutputThread)
             {
-                // Tear down our output thread -- now that the output pipe was closed on the
-                // far side, we can run down our local reader.
+                // Waiting for the output thread to exit ensures that all pending _TerminalOutputHandlers()
+                // calls have returned and won't notify our caller (ControlCore) anymore. This ensures that
+                // we don't call a destroyed event handler asynchronously from a background thread (GH#13880).
                 LOG_LAST_ERROR_IF(WAIT_FAILED == WaitForSingleObject(_hOutputThread.get(), INFINITE));
                 _hOutputThread.reset();
             }
 
-            if (_piClient.hProcess)
-            {
-                // Wait for the client to terminate (which it should do successfully)
-                LOG_LAST_ERROR_IF(WAIT_FAILED == WaitForSingleObject(_piClient.hProcess, INFINITE));
-                _piClient.reset();
-            }
-
             _transitionToState(ConnectionState::Closed);
         }
     }
diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index 3fdeb2ae442..91057b83041 100644
--- a/src/cascadia/TerminalControl/ControlCore.cpp
+++ b/src/cascadia/TerminalControl/ControlCore.cpp
@@ -1498,32 +1498,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         _FoundMatchHandlers(*this, *foundResults);
     }
 
-    // Method Description:
-    // - Asynchronously close our connection. The Connection will likely wait
-    //   until the attached process terminates before Close returns. If that's
-    //   the case, we don't want to block the UI thread waiting on that process
-    //   handle.
-    // Arguments:
-    // - <none>
-    // Return Value:
-    // - <none>
-    winrt::fire_and_forget ControlCore::_asyncCloseConnection()
-    {
-        if (auto localConnection{ std::exchange(_connection, nullptr) })
-        {
-            // Close the connection on the background thread.
-            co_await winrt::resume_background(); // ** DO NOT INTERACT WITH THE CONTROL CORE AFTER THIS LINE **
-
-            // Here, the ControlCore very well might be gone.
-            // _asyncCloseConnection is called on the dtor, so it's entirely
-            // possible that the background thread is resuming after we've been
-            // cleaned up.
-
-            localConnection.Close();
-            // connection is destroyed.
-        }
-    }
-
     void ControlCore::Close()
     {
         if (!_IsClosing())
@@ -1533,13 +1507,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             // Stop accepting new output and state changes before we disconnect everything.
             _connection.TerminalOutput(_connectionOutputEventToken);
             _connectionStateChangedRevoker.revoke();
-
-            // GH#1996 - Close the connection asynchronously on a background
-            // thread.
-            // Since TermControl::Close is only ever triggered by the UI, we
-            // don't really care to wait for the connection to be completely
-            // closed. We can just do it whenever.
-            _asyncCloseConnection();
+            _connection.Close();
         }
     }
 
diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h
index 63983f9be01..c82f9340674 100644
--- a/src/cascadia/TerminalControl/ControlCore.h
+++ b/src/cascadia/TerminalControl/ControlCore.h
@@ -271,8 +271,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         std::unique_ptr<til::throttled_func_trailing<>> _updatePatternLocations;
         std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> _updateScrollBar;
 
-        winrt::fire_and_forget _asyncCloseConnection();
-
         bool _setFontSizeUnderLock(int fontSize);
         void _updateFont(const bool initialUpdate = false);
         void _refreshSizeUnderLock();


Test patch: