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

Avoid timer ticks on frozen windows #16587

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jan 22, 2024

At the time of writing, closing the last tab of a window inexplicably
doesn't lead to the destruction of the remaining TermControl instance.
On top of that, on Win10 we don't destroy window threads due to bugs in
DesktopWindowXamlSource. In other words, we leak TermControl instances.

Additionally, the XAML timer class is "self-referential".
Releasing all references to an instance will not stop the timer.
Only calling Stop() explicitly will achieve that.

The result is that the message loop of a frozen window thread has so
far received 1-2 messages per second due to the blink timer not being
stopped. This may have filled the message queue and lead to bugs as
described in #16332 where keyboard input stopped working.

@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Area-Windowing Window frame, quake mode, tearout labels Jan 22, 2024
@DHowett
Copy link
Member

DHowett commented Jan 22, 2024

@lhecker Is this a 1.18 backport candidate?

@lhecker
Copy link
Member Author

lhecker commented Jan 22, 2024

Yeah, this one and the sister-PR #16588. I'm not entirely sure how many are affected by this though, in particular since it's just a theory (hence me writing "may lead to #16332"), so we could also skip the backport if you feel like both PRs in combination are a bit too risky.

@lhecker
Copy link
Member Author

lhecker commented Jan 22, 2024

The PR fails to build because TerminalApp.UnitTests depends on ColorHelper.cpp which is in another project which includes a pch.h with WinRTUtils, but TerminalApp.UnitTests has no access to that. Not sure how to resolve that yet, so I'll do that a bit later today.

}

::winrt::Windows::UI::Xaml::DispatcherTimer _timer{ nullptr };
winrt::event_token _token;
Copy link
Member

Choose a reason for hiding this comment

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

somehow methinks there's an auto_revoke we could use here or something, but also, meh?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think winrt::event_token is better in this case. The auto_revoke classes are effectively just a pair<weak_ref, event_token> which, on destruction, turns it into a strong-ref and revokes it. Since this is such a tiny class, I think the extra work to get rid of the extra weak_ref is worth it. (It's just like 2 lines of extra code after all).

return _timer && _timer.IsEnabled();
}

void Tick(winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable> const& handler)
Copy link
Member

Choose a reason for hiding this comment

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

Okay so notably, SafeDispatcherTimer only supports a single callback at a time for Tick (whereas I'm pretty sure that the standard one supports many callbacks) (this is also fine, pretty sure we're not using multiple callbacks on a single timer anyways)

Copy link
Member Author

Choose a reason for hiding this comment

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

My intention was that this avoids accidentally setting the same callback over and over again on the same timer instance. A number of places don't revoke the callbacks after all and just replace the timer instance without calling Stop() (i.e. they leak the timer instances).


std::optional<Windows::UI::Xaml::DispatcherTimer> _cursorTimer;
Copy link
Member

Choose a reason for hiding this comment

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

thank you for killing these optional<nullable thing>s

cursorTimer.Interval(std::chrono::milliseconds(blinkTime));
cursorTimer.Tick({ get_weak(), &TermControl::_CursorTimerTick });
_cursorTimer.emplace(std::move(cursorTimer));
_cursorTimer.Interval(std::chrono::milliseconds(blinkTime));
Copy link
Member

Choose a reason for hiding this comment

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

note to self: Interval will correctly re-initialize the underlying Timer if it was already Destroy'ed


#include <winrt/windows.ui.core.h>
#include <winrt/Windows.UI.h>
Copy link
Member

Choose a reason for hiding this comment

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

jeepers I don't love this, presumably this should be in the pch for both this project and the tests, but also those tests really haven't had love in years. So, this is fine for now. (I can only hope that it also being in the pch.h for TerminalAppLib means it won't actually get re-parsed here)

Copy link
Member Author

Choose a reason for hiding this comment

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

The Windows.UI header is a smaller than the Windows.UI.Core header so this at least makes it a tiny bit better.

@lhecker lhecker merged commit 521a300 into main Jan 23, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/16332-avoid-idle-timers branch January 23, 2024 17:00
DHowett pushed a commit that referenced this pull request Jan 25, 2024
At the time of writing, closing the last tab of a window inexplicably
doesn't lead to the destruction of the remaining TermControl instance.
On top of that, on Win10 we don't destroy window threads due to bugs in
DesktopWindowXamlSource. In other words, we leak TermControl instances.

Additionally, the XAML timer class is "self-referential".
Releasing all references to an instance will not stop the timer.
Only calling Stop() explicitly will achieve that.

The result is that the message loop of a frozen window thread has so
far received 1-2 messages per second due to the blink timer not being
stopped. This may have filled the message queue and lead to bugs as
described in #16332 where keyboard input stopped working.

(cherry picked from commit 521a300)
Service-Card-Id: 91620224
Service-Version: 1.18
DHowett pushed a commit that referenced this pull request Jan 25, 2024
At the time of writing, closing the last tab of a window inexplicably
doesn't lead to the destruction of the remaining TermControl instance.
On top of that, on Win10 we don't destroy window threads due to bugs in
DesktopWindowXamlSource. In other words, we leak TermControl instances.

Additionally, the XAML timer class is "self-referential".
Releasing all references to an instance will not stop the timer.
Only calling Stop() explicitly will achieve that.

The result is that the message loop of a frozen window thread has so
far received 1-2 messages per second due to the blink timer not being
stopped. This may have filled the message queue and lead to bugs as
described in #16332 where keyboard input stopped working.

(cherry picked from commit 521a300)
Service-Card-Id: 91642474
Service-Version: 1.19
DHowett pushed a commit that referenced this pull request Jan 29, 2024
This changeset ensures that the message queue of frozen windows is
always being serviced. This should ensure that it won't fill up and
lead to deadlocks, freezes, or similar. I've tried _a lot_ of different
approaches before settling on this one. Introducing a custom `WM_APP`
message has the benefit of being the least intrusive to the existing
code base.

The approach that I would have favored the most would be to never
destroy the `AppHost` instance in the first place, as I imagined that
this would be more robust in general and resolve other (rare) bugs.
However, I found that this requires rewriting some substantial parts
of the code base around `AppHost` and it could be something that may
be of interest in the future.

Closes #16332
Depends on #16587 and #16575
DHowett pushed a commit that referenced this pull request Jan 29, 2024
This changeset ensures that the message queue of frozen windows is
always being serviced. This should ensure that it won't fill up and
lead to deadlocks, freezes, or similar. I've tried _a lot_ of different
approaches before settling on this one. Introducing a custom `WM_APP`
message has the benefit of being the least intrusive to the existing
code base.

The approach that I would have favored the most would be to never
destroy the `AppHost` instance in the first place, as I imagined that
this would be more robust in general and resolve other (rare) bugs.
However, I found that this requires rewriting some substantial parts
of the code base around `AppHost` and it could be something that may
be of interest in the future.

Closes #16332
Depends on #16587 and #16575

(cherry picked from commit 5d2fa47)
Service-Card-Id: 91642478
Service-Version: 1.18
DHowett pushed a commit that referenced this pull request Jan 29, 2024
This changeset ensures that the message queue of frozen windows is
always being serviced. This should ensure that it won't fill up and
lead to deadlocks, freezes, or similar. I've tried _a lot_ of different
approaches before settling on this one. Introducing a custom `WM_APP`
message has the benefit of being the least intrusive to the existing
code base.

The approach that I would have favored the most would be to never
destroy the `AppHost` instance in the first place, as I imagined that
this would be more robust in general and resolve other (rare) bugs.
However, I found that this requires rewriting some substantial parts
of the code base around `AppHost` and it could be something that may
be of interest in the future.

Closes #16332
Depends on #16587 and #16575

(cherry picked from commit 5d2fa47)
Service-Card-Id: 91642479
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants