From 8b6c7c2e1ff8a60dc0f452a77b6b1b68f6da6300 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 29 Sep 2023 13:35:50 -0500 Subject: [PATCH] Use `weak_ptr`s for `AppHost` for coroutines See MSFT:46763065. Looks like we're in the middle of being `Refrigerate`d, we're pumping messages, and as we pump messages, we get to a `co_await` in `AppHost::_WindowInitializedHandler`. When we resume, we just try to use `this` like everything's fine but OH NO, IT'S NOT. To fix this, I'm * Adding `enable_shared_from_this` to `AppHost` * Holding the `AppHost` in a shared_ptr in WindowThread - though, this is a singular owning `shared_ptr`. This is probably ripe for other footguns, but there's little we can do about this. * whenever we `co_await` in `AppHost`, make sure we grab a weak ref first, and check it on the other side. This is another "squint and yep that's a bug" fix, that I haven't been able to verify locally. This is [allegedly](https://media.tenor.com/VQi3bktwLdIAAAAC/allegedly-supposedly.gif) about 10% of our 1.19 crashes after 3 days. Closes #16061 --- src/cascadia/WindowsTerminal/AppHost.cpp | 43 +++++++++++++++++++ src/cascadia/WindowsTerminal/AppHost.h | 2 +- src/cascadia/WindowsTerminal/WindowThread.cpp | 4 +- src/cascadia/WindowsTerminal/WindowThread.h | 6 ++- 4 files changed, 51 insertions(+), 4 deletions(-) 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, ¤tDesktopGuid))) { @@ -963,8 +972,18 @@ winrt::fire_and_forget AppHost::_peasantNotifyActivateWindow() winrt::Windows::Foundation::IAsyncOperation 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 -class AppHost +class AppHost : public std::enable_shared_from_this { 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 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 };