Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use weak_ptrs for AppHost for coroutines #16065

Merged
merged 1 commit into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to run on the BG thread in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it on a background thread so that a hung Monarch wouldn't hang a peasant process when the peasant gets activated.

I suppose these days, it's all the same process, so we could pull this one.


// 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
Loading