Fix a shutdown race condition in ControlCore by lhecker · Pull Request #18632 · microsoft/terminal

lhecker · 2025-02-26T15:32:38Z

I found multiple issues while investigating this:

Render thread shutdown is racy, because it doesn't actually stop the render thread.
Lifetime management in ControlCore failed to account for the circular dependency of render thread --> renderer --> render data --> terminal --> renderer --> render thread. Fixed by reordering the ControlCore members to ensure their correct destruction.
Ensured that the connection setter calls close on the previous connection.

(Hopefully) Closes #18598

Validation Steps Performed

Can't repro the original failure ❌
Opening and closing tabs as fast as possible doesn't crash anymore ✅
Detaching and reattaching a tab producing continuous output ✅

lhecker · 2025-02-26T15:33:19Z

src/cascadia/TerminalApp/DebugTapConnection.cpp

-        _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { this, &DebugTapConnection::_OutputHandler });
-        _stateChangedRevoker = wrappedConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*e*/) {
-            StateChanged.raise(*this, nullptr);
+        _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { get_weak(), &DebugTapConnection::_OutputHandler });


get_weak() to ensure pending calls during revocation can complete without this being deallocated.

does this by chance fix the issue where closing a connection while the debug tap is on it crashes terminal

I wasn't aware we had such an issue. It's quite likely that this fixes it.

lhecker · 2025-02-26T15:33:35Z

src/cascadia/TerminalControl/ControlCore.cpp

-            _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) {
-                ConnectionStateChanged.raise(*this, nullptr);
-            });
+            _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, { get_weak(), &ControlCore::_connectionStateChangedHandler });


get_weak() to ensure pending calls during revocation can complete without this being deallocated. (Same below.)

Fuck. I just realized this can't work because it allows this to be destructed on a background thread which breaks WinUI. Got to use a lock or something instead.

lhecker · 2025-02-26T15:33:51Z

src/cascadia/TerminalControl/ControlCore.cpp

+        // TODO: This manual event revoking doesn't make much sense.
+        // We could just drop the old connection. Why have all that Close() stuff?
+        // It also shouldn't need to be exposed to the outside. I suspect we can only
+        // improve this though, once drag/drop of tabs doesn't use "startup actions" anymore.


(For future code archealogists.)

lhecker · 2025-02-26T16:17:01Z

src/cascadia/TerminalControl/ControlCore.cpp

-            // First create the render thread.
-            // Then stash a local pointer to the render thread so we can initialize it and enable it
-            // to paint itself *after* we hand off its ownership to the renderer.
-            // We split up construction and initialization of the render thread object this way
-            // because the renderer and render thread have circular references to each other.
-            auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>();
-            auto* const localPointerToThread = renderThread.get();


This has always been a thorn in my eye, so I fixed it in this PR by moving the RenderThread into the Renderer. No more ownership issues.

lhecker · 2025-02-26T16:19:18Z

src/cascadia/TerminalControl/ControlCore.cpp

-
-        _renderer.reset();
-        _renderEngine.reset();


I removed this, because I fixed the member order of the class.

DHowett · 2025-02-26T16:26:58Z

src/renderer/base/renderer.cpp

-    {
-        AddRenderEngine(rgpEngines[i]);
-    }
+    THROW_IF_FAILED(_pThread->Initialize(this));


I always forget... is this guaranteed to be usable in the constructor? is Initialize safe for this if this is partially constructed?

this is a valid pointer in the constructor, it's just that the members aren't necessarily fully initialized yet.

DHowett · 2025-02-26T16:28:12Z

I know the tests probably interact with RenderThread in weird ways.

src/cascadia/TerminalControl/ControlCore.cpp

DHowett

Okay. I think I need you to explain how moving from the current architecture where we have "is the thread allowed to spin" and "does the thread need to actually produce a frame" is OK, and what implications all of this simplification should have on the application. I can see that there's real issues fixed here, but it is a big change in a touchy area.

DHowett · 2025-03-11T21:35:53Z

src/cascadia/TerminalControl/ControlCore.cpp

    void ControlCore::_sendInputToConnection(std::wstring_view wstr)
    {
-        _connection.WriteInput(winrt_wstring_to_array_view(wstr));
+        if (_connection)


does [[likely]] make a difference here? ergo, is input slower because of the branch? is that too much of a micro-optimization?

No, it won't. likely/unlikely rarely make a difference, but if they do it's primarily in tight code.

DHowett · 2025-03-11T21:37:27Z

src/cascadia/TerminalControl/ControlCore.h

+
+        // ↑ This one is tricky - all of these are raw pointers:
+        //   1. _terminal depends on _renderer (for invalidations)
+        //   2. _renderer depends on _terminal (for IRenderData)


the comments below say 3 3 and 1. there is no 2.

DHowett · 2025-03-11T21:40:38Z

src/renderer/base/renderer.cpp

        if (--tries == 0)
        {
            // Stop trying.
-            _pThread->DisablePainting();


It's way safer if this function simply signals the end of painting to the renderer via a return value. This reminds me that I didn't test this, shame on me.

lhecker · 2025-03-14T14:29:01Z

I have good news: this PR fixes another bug where restarting a connection would fail to terminate the previous one. (It would leak the connection by not calling Close on it, since it is unfortunately self-referential.)

Regarding why all this is necessary: I realized during development that the entire logic around _hPaintCompletedEvent in the RenderThread is flawed as it doesn't actually reliably protect against another frame being scheduled. This rewrite fixes the issue which prevents random AVs during shutdown.
…hopefully without any regressions though.

I found multiple issues while investigating this: * Render thread shutdown is racy, because it doesn't actually stop the render thread. * Lifetime management in `ControlCore` failed to account for the circular dependency of render thread --> renderer --> render data --> terminal --> renderer --> render thread. Fixed by reordering the `ControlCore` members to ensure their correct destruction. * Ensured that the connection setter calls close on the previous connection. (Hopefully) Closes #18598 ## Validation Steps Performed * Can't repro the original failure ❌ * Opening and closing tabs as fast as possible doesn't crash anymore ✅ * Detaching and reattaching a tab producing continuous output ✅ (cherry picked from commit 70f85a4) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgXhFbo Service-Version: 1.22

I found multiple issues while investigating this: * Render thread shutdown is racy, because it doesn't actually stop the render thread. * Lifetime management in `ControlCore` failed to account for the circular dependency of render thread --> renderer --> render data --> terminal --> renderer --> render thread. Fixed by reordering the `ControlCore` members to ensure their correct destruction. * Ensured that the connection setter calls close on the previous connection. (Hopefully) Closes #18598 ## Validation Steps Performed * Can't repro the original failure ❌ * Opening and closing tabs as fast as possible doesn't crash anymore ✅ * Detaching and reattaching a tab producing continuous output ✅ (cherry picked from commit 70f85a4) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgYdERw Service-Version: 1.23

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Quality Stability, Performance, Etc. Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Feb 26, 2025

lhecker force-pushed the dev/lhecker/18598-shutdown-crash branch from 5302525 to 45d2519 Compare February 26, 2025 16:15

lhecker commented Feb 26, 2025

View reviewed changes

DHowett reviewed Feb 26, 2025

View reviewed changes

lhecker mentioned this pull request Feb 26, 2025

reattaching a tab unexpectedly closes windows terminal #18614

Closed

DHowett added this to 1.23 Servicing Pipeline Feb 28, 2025

github-project-automation bot moved this to To Cherry Pick in 1.23 Servicing Pipeline Feb 28, 2025

DHowett moved this from To Cherry Pick to To Consider in 1.23 Servicing Pipeline Feb 28, 2025

lhecker requested review from DHowett, carlos-zamora and zadjii-msft March 11, 2025 14:38

Fix a shutdown race condition in ControlCore

8fe4350

lhecker force-pushed the dev/lhecker/18598-shutdown-crash branch from dc45cbb to 8fe4350 Compare March 11, 2025 16:04

DHowett reviewed Mar 11, 2025

View reviewed changes

src/cascadia/TerminalControl/ControlCore.cpp Show resolved Hide resolved

DHowett approved these changes Mar 11, 2025

View reviewed changes

Fix resume after a repeated render failure

87daf4d

DHowett approved these changes Mar 14, 2025

View reviewed changes

DHowett merged commit 70f85a4 into main Mar 14, 2025
19 checks passed

DHowett deleted the dev/lhecker/18598-shutdown-crash branch March 14, 2025 22:06

DHowett added this to 1.22 Servicing Pipeline Mar 14, 2025

github-project-automation bot moved this to To Cherry Pick in 1.22 Servicing Pipeline Mar 14, 2025

DHowett moved this from To Cherry Pick to To Consider in 1.22 Servicing Pipeline Mar 14, 2025

DHowett mentioned this pull request Mar 19, 2025

Crash when closing one terminal window, all other terminal instances get killed #18458

Open

DHowett moved this from To Consider to To Cherry Pick in 1.22 Servicing Pipeline Apr 21, 2025

DHowett moved this from To Consider to To Cherry Pick in 1.23 Servicing Pipeline Apr 21, 2025

DHowett moved this from To Cherry Pick to Cherry Picked in 1.23 Servicing Pipeline Apr 21, 2025

DHowett moved this from To Cherry Pick to Cherry Picked in 1.22 Servicing Pipeline Apr 21, 2025

lhecker mentioned this pull request Apr 23, 2025

Implement DECSET 2026 - Synchronized Output #18826

Merged

lhecker mentioned this pull request Jun 16, 2025

Windows Terminal crashes when dragging a tab #15689

Open

lhecker mentioned this pull request Jul 2, 2025

Trigger Redraw Crash #18275

Open

DHowett moved this from Cherry Picked to Shipped in 1.23 Servicing Pipeline Sep 5, 2025

Conversation

lhecker commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Feb 26, 2025

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker commented Mar 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lhecker commented Feb 26, 2025 •

edited

Loading

lhecker Mar 12, 2025 •

edited

Loading