Repository: microsoft/terminal (language: cpp)

Issue:
`winrt::apartment_context` considered harmful
We should try to avoid using `winrt::apartment_context` as it's inherently flawed unfortunately. 😥
As Raymond Chen wrote:
> Unfortunately, it's true. co_await'ing an `apartment_context` is blocking [the background thread]. That's because it's built out of COM's `IContextCallback`, [which supports only synchronous transfer](https://devblogs.microsoft.com/oldnewthing/20191128-00/?p=103157).
> [...]
> Note that it's even worse: Even after you get to the other side, the side you came **\*from\*** is still blocked until you finish doing your other-side work (which could take the form of another co_await).
> [...]
> If the UI thread is hung, this will lead to threadpool starvation and eventually a deadlock.

`_GetWindowLayoutAsync()` in particular is quite dangerous because it uses an `apartment_context` to switch back to a background thread and blocks the UI thread for that duration...

_Originally posted by @lhecker in https://github.com/microsoft/terminal/pull/12979#discussion_r858680237_


Discussion / hints:
We should fix any uses of that.

Fix patch:
diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp
index ddbbafdbe9b..2865a021c5d 100644
--- a/src/cascadia/WindowsTerminal/AppHost.cpp
+++ b/src/cascadia/WindowsTerminal/AppHost.cpp
@@ -900,8 +900,6 @@ winrt::fire_and_forget AppHost::_peasantNotifyActivateWindow()
 // - The window layout as a json string.
 winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> AppHost::_GetWindowLayoutAsync()
 {
-    winrt::apartment_context peasant_thread;
-
     winrt::hstring layoutJson = L"";
     // Use the main thread since we are accessing controls.
     co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher());
@@ -912,9 +910,6 @@ winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> AppHost::_GetWindowL
     }
     CATCH_LOG()
 
-    // go back to give the result to the peasant.
-    co_await peasant_thread;
-
     co_return layoutJson;
 }
 
@@ -990,9 +985,6 @@ void AppHost::_DisplayWindowId(const winrt::Windows::Foundation::IInspectable& /
 winrt::fire_and_forget AppHost::_RenameWindowRequested(const winrt::Windows::Foundation::IInspectable /*sender*/,
                                                        const winrt::TerminalApp::RenameWindowRequestedArgs args)
 {
-    // Capture calling context.
-    winrt::apartment_context ui_thread;
-
     // Switch to the BG thread - anything x-proc must happen on a BG thread
     co_await winrt::resume_background();
 
@@ -1002,12 +994,9 @@ winrt::fire_and_forget AppHost::_RenameWindowRequested(const winrt::Windows::Fou
 
         _peasant.RequestRename(requestArgs);
 
-        // Switch back to the UI thread. Setting the WindowName needs to happen
-        // on the UI thread, because it'll raise a PropertyChanged event
-        co_await ui_thread;
-
         if (requestArgs.Succeeded())
         {
+            co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher());
             _windowLogic.WindowName(args.ProposedName());
         }
         else


Test patch: