Skip to content

Commit

Permalink
Use weak_ptrs for AppHost for coroutines (#16065)
Browse files Browse the repository at this point in the history
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

(cherry picked from commit 8521aae)
Service-Card-Id: 90731962
Service-Version: 1.19
  • Loading branch information
zadjii-msft authored and DHowett committed Oct 3, 2023
1 parent 47728dc commit 70ad670
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 4 deletions.
43 changes: 43 additions & 0 deletions src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
{
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/WindowsTerminal/AppHost.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/WindowsTerminal/WindowThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 5 additions & 1 deletion src/cascadia/WindowsTerminal/WindowThread.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down

0 comments on commit 70ad670

Please sign in to comment.