Pump the message queue on frozen windows by lhecker · Pull Request #16588 · microsoft/terminal

lhecker · 2024-01-22T15:25:04Z

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 · 2024-01-22T16:00:12Z

Wait, I don't understand the difference between these two identically-named PRs

lhecker · 2024-01-22T16:55:11Z

I keep forgetting that GitHub doesn't fix the PR title when you change the base. 😑

DHowett · 2024-01-22T17:04:33Z

src/cascadia/WindowsTerminal/WindowThread.cpp

+        {
+            return false;
+        }
+        if (msg.message == AppHost::WM_REFRIGERATE)


fwiw mike referred to the act of waking back up as Microwaving; refrigerating is when it goes down to sleep

Oh, I see - you have repurposed it to be a single flag. Transition between the states. etc

Yeah I wasn't sure what to name it... "refrigerate" has the benefit that it only comes up in this context in the project, so it should be easier to find related code.

zadjii-msft

would approve, but I'm a little curious on the ClearPersistedWindowState thing first

zadjii-msft · 2024-01-23T16:02:05Z

src/cascadia/WindowsTerminal/AppHost.cpp

-        _windowLogic.ClearPersistedWindowState();
-    }
-
    // Remove ourself from the list of peasants so that we aren't included in


Very sus that this is just gone now - why? Is this related?

If you expand the surrounding code in the diff viewer you'll notice that this block of code is a duplicate! I think it might have been an incorrect merge in the past.

oh what the heck

src/cascadia/WindowsTerminal/WindowThread.cpp

zadjii-msft · 2024-01-23T16:10:36Z

src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp

 {
 }

+NonClientIslandWindow::~NonClientIslandWindow()


Just added redundancy? I don't think we've got any code that was removed that was calling Close before, right?

Oh, it's terrible... This change fixes a bug and you might already know what I mean when I say: IslandWindow::Close is a virtual function which the ~IslandWindow destructor calls. NonClientIslandWindow overloads Close. boom

But in my opinion this sorely belongs into the "only obvious in hindsight" category of arcane C++ footnukes (can't have enough of those in a language!), so let me explain in more detail:

No matter whether you define a destructor or not, C++ will define it's own "real-destructor", because that one is responsible for destroying non-trival class members (like std::strings and such). So, while NonClientIslandWindow doesn't have an explicit destructor, it gets one anyways which virtually overloads the IslandWindow destructor.

With inheritance, destructors are called in reverse starting from child classes up to parent classes. In other words, first ~NonClientIslandWindow is called and then ~IslandWindow.

In short, when ~IslandWindow gets called and it tries to call the virtual function Close, the child class NonClientIslandWindow is already destroyed, which means that the vtable pointer now points to IslandWindow, which means that IslandWindow::Close is called and not NonClientIslandWindow::Close. The above change fixes this.

Co-authored-by: Mike Griese <[email protected]>

src/cascadia/WindowsTerminal/WindowThread.cpp

The base branch was changed.

…p-frozen-queue

lhecker · 2024-01-23T17:50:23Z

FYI this PR sort of depends on #16575 because I had to (chose to?) remove ThrowAway. I think it will be fine even if ExitProcess is called, but I believe it'll be much safer with TerminateProcess so that the UI threads truly don't get a chance to destroy themselves.

zadjii-msft · 2024-01-29T22:00:31Z

src/cascadia/WindowsTerminal/AppHost.cpp

-        _windowLogic.ClearPersistedWindowState();
-    }
-
    // Remove ourself from the list of peasants so that we aren't included in


oh what the heck

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

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

lhecker added 2 commits January 22, 2024 16:16

Avoid timer ticks on frozen windows

5511a11

Pump the message queue on frozen windows

4008a90

lhecker mentioned this pull request Jan 22, 2024

Avoid timer ticks on frozen windows #16587

Merged

lhecker changed the title ~~Avoid timer ticks on frozen windows~~ Pump the message queue on frozen windows Jan 22, 2024

DHowett reviewed Jan 22, 2024

View reviewed changes

DHowett previously approved these changes Jan 22, 2024

View reviewed changes

zadjii-msft reviewed Jan 23, 2024

View reviewed changes

Apply suggestions from code review

2244e60

Co-authored-by: Mike Griese <[email protected]>

github-advanced-security bot found potential problems Jan 23, 2024

View reviewed changes

src/cascadia/WindowsTerminal/WindowThread.cpp Fixed Show fixed Hide fixed

src/cascadia/WindowsTerminal/WindowThread.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

Base automatically changed from dev/lhecker/16332-avoid-idle-timers to main January 23, 2024 17:00

lhecker added 2 commits January 23, 2024 18:01

Merge remote-tracking branch 'origin/main' into dev/lhecker/16332-pum…

2caa43a

…p-frozen-queue

Fix spelling

0c6469a

This comment has been minimized.

Sign in to view

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Windowing Window frame, quake mode, tearout Product-Terminal The new Windows Terminal. labels Jan 23, 2024

Remove ThrowAway

b463df5

DHowett approved these changes Jan 25, 2024

View reviewed changes

zadjii-msft approved these changes Jan 29, 2024

View reviewed changes

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 29, 2024

DHowett merged commit 5d2fa47 into main Jan 29, 2024

DHowett deleted the dev/lhecker/16332-pump-frozen-queue branch January 29, 2024 22:01

zadjii-msft mentioned this pull request Feb 1, 2024

Ghostly 'Close' button hovering on my Desktop #16100

Closed

fredswims mentioned this pull request Mar 8, 2024

Windows Terminal v1.18.10291.0 #16843

Closed

Conversation

lhecker commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Jan 22, 2024

Uh oh!

lhecker commented Jan 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

lhecker commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhecker commented Jan 22, 2024 •

edited

Loading

lhecker commented Jan 23, 2024 •

edited

Loading