Repository: microsoft/terminal (language: cpp)

Issue:
INVALID_POINTER_READ_c0000005_WindowsTerminal.exe!AppHost::_WindowInitializedHandler$_ResumeCoro$1
MSFT:46763065.

Failure ID `8ab19ec3-2532-a209-0ca4-5e6b9aa9133b`

8.70% of our crashes in 1.19 so far.

Bug is here, on L1396.

https://github.com/microsoft/terminal/blob/4382a17352869bb1f33e020ac281d04aa15a8e8f/src/cascadia/WindowsTerminal/AppHost.cpp#L1390-L1402

Undoubtably, this is us not using a "weak ref". The app host got dtored during a `Refrigerate`, and so `this` is invalid on the other side of the `co_await`. Yikes. 


Discussion / hints:

Fix patch:
diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp
index 81218dbf4c9..36d47211b79 100644
--- a/src/cascadia/WindowsTerminal/AppHost.cpp
+++ b/src/cascadia/WindowsTerminal/AppHost.cpp
@@ -931,8 +931,17 @@ winrt::fire_and_forget AppHost::_peasantNotifyActivateWindow()
     const auto peasant = _peasant;
     const auto hwnd = _window->GetHandle();
 
+    auto weakThis{ weak_from_this() };
+
     co_await winrt::resume_background();
 
+    // If we're gone on the other side of this co_await, well, that's fine. Just bail.
+    const auto strongThis = weakThis.lock();
+    if (!strongThis)
+    {
+        co_return;
+    }
+
     GUID currentDesktopGuid{};
     if (FAILED_LOG(desktopManager->GetWindowDesktopId(hwnd, &currentDesktopGuid)))
     {
@@ -963,8 +972,18 @@ winrt::fire_and_forget AppHost::_peasantNotifyActivateWindow()
 winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> AppHost::_GetWindowLayoutAsync()
 {
     winrt::hstring layoutJson = L"";
+
+    auto weakThis{ weak_from_this() };
+
     // Use the main thread since we are accessing controls.
     co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher());
+
+    const auto strongThis = weakThis.lock();
+    if (!strongThis)
+    {
+        co_return layoutJson;
+    }
+
     try
     {
         const auto pos = _GetWindowLaunchPosition();
@@ -1021,10 +1040,19 @@ void AppHost::_HandleSummon(const winrt::Windows::Foundation::IInspectable& /*se
 winrt::fire_and_forget AppHost::_IdentifyWindowsRequested(const winrt::Windows::Foundation::IInspectable /*sender*/,
                                                           const winrt::Windows::Foundation::IInspectable /*args*/)
 {
+    auto weakThis{ weak_from_this() };
+
     // We'll be raising an event that may result in a RPC call to the monarch -
     // make sure we're on the background thread, or this will silently fail
     co_await winrt::resume_background();
 
+    // If we're gone on the other side of this co_await, well, that's fine. Just bail.
+    const auto strongThis = weakThis.lock();
+    if (!strongThis)
+    {
+        co_return;
+    }
+
     if (_peasant)
     {
         _peasant.RequestIdentifyWindows();
@@ -1234,9 +1262,16 @@ void AppHost::_IsQuakeWindowChanged(const winrt::Windows::Foundation::IInspectab
 winrt::fire_and_forget AppHost::_QuitRequested(const winrt::Windows::Foundation::IInspectable&,
                                                const winrt::Windows::Foundation::IInspectable&)
 {
+    auto weakThis{ weak_from_this() };
     // Need to be on the main thread to close out all of the tabs.
     co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher());
 
+    const auto strongThis = weakThis.lock();
+    if (!strongThis)
+    {
+        co_return;
+    }
+
     _windowLogic.Quit();
 }
 
@@ -1387,12 +1422,20 @@ winrt::fire_and_forget AppHost::_WindowInitializedHandler(const winrt::Windows::
         nCmdShow = SW_MAXIMIZE;
     }
 
+    auto weakThis{ weak_from_this() };
     // For inexplicable reasons, again, hop to the BG thread, then back to the
     // UI thread. This is shockingly load bearing - without this, then
     // sometimes, we'll _still_ show the HWND before the XAML island actually
     // paints.
     co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher(), winrt::Windows::UI::Core::CoreDispatcherPriority::Low);
 
+    // If we're gone on the other side of this co_await, well, that's fine. Just bail.
+    const auto strongThis = weakThis.lock();
+    if (!strongThis)
+    {
+        co_return;
+    }
+
     ShowWindow(_window->GetHandle(), nCmdShow);
 
     // If we didn't start the window hidden (in one way or another), then try to
diff --git a/src/cascadia/WindowsTerminal/AppHost.h b/src/cascadia/WindowsTerminal/AppHost.h
index dabe517394b..fd912aee67d 100644
--- a/src/cascadia/WindowsTerminal/AppHost.h
+++ b/src/cascadia/WindowsTerminal/AppHost.h
@@ -6,7 +6,7 @@
 #include "NotificationIcon.h"
 #include <ThrottledFunc.h>
 
-class AppHost
+class AppHost : public std::enable_shared_from_this<AppHost>
 {
 public:
     AppHost(const winrt::TerminalApp::AppLogic& logic,
diff --git a/src/cascadia/WindowsTerminal/WindowThread.cpp b/src/cascadia/WindowsTerminal/WindowThread.cpp
index 25fb94d5b07..2c70741ad1c 100644
--- a/src/cascadia/WindowsTerminal/WindowThread.cpp
+++ b/src/cascadia/WindowsTerminal/WindowThread.cpp
@@ -24,7 +24,7 @@ void WindowThread::CreateHost()
     assert(_warmWindow == nullptr);
 
     // Start the AppHost HERE, on the actual thread we want XAML to run on
-    _host = std::make_unique<::AppHost>(_appLogic,
+    _host = std::make_shared<::AppHost>(_appLogic,
                                         _args,
                                         _manager,
                                         _peasant);
@@ -164,7 +164,7 @@ void WindowThread::Microwave(
     _peasant = std::move(peasant);
     _args = std::move(args);
 
-    _host = std::make_unique<::AppHost>(_appLogic,
+    _host = std::make_shared<::AppHost>(_appLogic,
                                         _args,
                                         _manager,
                                         _peasant,
diff --git a/src/cascadia/WindowsTerminal/WindowThread.h b/src/cascadia/WindowsTerminal/WindowThread.h
index 0e8bc5429d1..a1af2db6500 100644
--- a/src/cascadia/WindowsTerminal/WindowThread.h
+++ b/src/cascadia/WindowsTerminal/WindowThread.h
@@ -35,7 +35,11 @@ class WindowThread : public std::enable_shared_from_this<WindowThread>
     winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs _args{ nullptr };
     winrt::Microsoft::Terminal::Remoting::WindowManager _manager{ nullptr };
 
-    std::unique_ptr<::AppHost> _host{ nullptr };
+    // This is a "shared_ptr", but it should be treated as a unique, owning ptr.
+    // It's shared, because there are edge cases in refrigeration where internal
+    // co_awaits inside AppHost might resume after we've dtor'd it, and there's
+    // no other way for us to let the AppHost know it has passed on.
+    std::shared_ptr<::AppHost> _host{ nullptr };
     winrt::event_token _UpdateSettingsRequestedToken;
 
     std::unique_ptr<::IslandWindow> _warmWindow{ nullptr };


Test patch: