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.

Sign up for GitHub

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

Tray Icon PR followup #10938

Merged
4 commits merged into from
Aug 19, 2021
Merged

Tray Icon PR followup #10938

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c2a880
accidentally kept monarch as defapp server, destroy tray icon if monarch
leonMSFT Aug 12, 2021
fdd14b3
move tray icon cleanup to somewhere that's not the destructor
leonMSFT Aug 14, 2021
b94e199
ensure we're not dead when we want to cleanup, move to the bg thread …
leonMSFT Aug 16, 2021
dd8d07e
formatting
leonMSFT Aug 17, 2021
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
11 changes: 6 additions & 5 deletions src/cascadia/WindowsTerminal/AppHost.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,13 @@ AppHost::AppHost() noexcept :

AppHost::~AppHost()
{
if (_window->IsQuakeWindow())
if (_windowManager.IsMonarch())
{
_windowManager.RequestHideTrayIcon();
_DestroyTrayIcon();
}
else if (_window->IsQuakeWindow())
{
_HideTrayIconRequested();
Copy link
Member

Choose a reason for hiding this comment

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

This is a bad idea; _HideTrayIconRequested is a coroutine that takes an implicit reference to this, and you're dispatching it out of the destructor. By the time the resume_backround resumes you, this will have been destroyed.

This needs to be restructured such that (1) the destructor never uses anything that requires a living this to persist after the destructor returns and (2) it is as small as possible 😄

(Also, there's no guarantee that AppHost will be destructed.)

Copy link
Member

@lhecker lhecker Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also, there's no guarantee that AppHost will be destructed.

It's a regular instance in wWinMain. It should be destroyed properly unless the app abort()s, right?

We could make a _HideTrayIconRequestedAsync and have _HideTrayIconRequested run synchronously.
BTW _HideTrayIconRequested accesses settings from a background thread which is a race condition... I think I should add thread asserts to CascadiaSettings. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

It's a regular instance in wWinMain. It should be destroyed properly unless the app abort()s, right?

Yes. That does assume that main exits normally. If we were more like a standard UWP Xaml application, we would actually ExitProcess/TerminateProcess when the window was destroyed. The fact that we do not makes us uncommon in the world of Xaml, and is the cause for some of our "this object was already disposed of!" error traces on shutdown in debug mode. 😄

(Because of that, we've entertained the idea multiple times of switching -- it would clear up almost all of our on-exit crashes that originate deep inside Windows.UI.Xaml.dll!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm then I guess an alternative might be to do this particular Tray Icon cleanup in something like LastTabClosed?

}

// destruction order is important for proper teardown here
Expand Down Expand Up @@ -656,9 +660,6 @@ void AppHost::_BecomeMonarch(const winrt::Windows::Foundation::IInspectable& /*s
{
_setupGlobalHotkeys();

// The monarch is just going to be THE listener for inbound connections.
_listenForInboundConnections();
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. Otherwise I'd approve this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this was originally removed as part of this defapp bug fix #10823, and my bad merge left it in 😥. In short, the fix made it so that instead of making the monarch receive default terminal handoffs by default, the monarch should first figure out which peasant (or itself) should handle a default terminal handoff and tell that window to receive the connection.


if (_windowManager.DoesQuakeWindowExist() ||
_window->IsQuakeWindow() ||
(_logic.GetAlwaysShowTrayIcon() || _logic.GetMinimizeToTray()))
Expand Down