From 666c446bc38f741a7fc17b481ea1c665a4f31d13 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 6 Sep 2022 23:36:59 +0200 Subject: [PATCH] Fix a ControlCore race condition on connection close (#13882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As noted by the `winrt::event` documentation: > [...] But for asynchronous events, even after revoking [...], an in-flight > event might reach your object after it has started destructing. This is because while adding/removing/calling event handlers might be thread-safe, there's no guarantee that they run mutually exclusive. This commit fixes the issue by reverting 6f0f245. Since we never checked the result of closing a terminal connection anyways, this commit simply drops the wait on the connection being teared down to ensure #1996 doesn't regress. Closes #13880 ## Validation Steps Performed * Open tab, close tab, open tab, close tab, open tab, close tab * ConPTY ✅ * Azure ✅ * Closing a tab with a huge amount of panes ✅ * Opening a bunch of tabs and then closing the window ✅ * Closing a tab while it's busy with VT ✅ * `wtd -w 0 nt cmd /c exit` ✅ * `wtd -w -1 cmd /c exit` * No WerFault spawns ✅ --- .../TerminalConnection/AzureConnection.cpp | 7 ++-- .../TerminalConnection/ConptyConnection.cpp | 17 ++++------ src/cascadia/TerminalControl/ControlCore.cpp | 34 +------------------ src/cascadia/TerminalControl/ControlCore.h | 2 -- 4 files changed, 12 insertions(+), 48 deletions(-) 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 2e3baf89e98..49773625ae3 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -1509,32 +1509,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: - // - - // Return Value: - // - - 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()) @@ -1544,13 +1518,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 6f765736354..07b9b46228b 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -282,8 +282,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation std::unique_ptr> _updatePatternLocations; std::shared_ptr> _updateScrollBar; - winrt::fire_and_forget _asyncCloseConnection(); - bool _setFontSizeUnderLock(int fontSize); void _updateFont(const bool initialUpdate = false); void _refreshSizeUnderLock();