Pump the message queue on frozen windows #16588
Conversation
|
Wait, I don't understand the difference between these two identically-named PRs |
lhecker
changed the title
|
I keep forgetting that GitHub doesn't fix the PR title when you change the base. 😑 |
| { | ||
| return false; | ||
| } | ||
| if (msg.message == AppHost::WM_REFRIGERATE) |
There was a problem hiding this comment.
fwiw mike referred to the act of waking back up as Microwaving; refrigerating is when it goes down to sleep
There was a problem hiding this comment.
Oh, I see - you have repurposed it to be a single flag. Transition between the states. etc
There was a problem hiding this comment.
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.
| { | ||
| _windowLogic.ClearPersistedWindowState(); | ||
| } | ||
|
|
||
| // Remove ourself from the list of peasants so that we aren't included in |
There was a problem hiding this comment.
Very sus that this is just gone now - why? Is this related?
There was a problem hiding this comment.
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.
| @@ -26,6 +26,11 @@ NonClientIslandWindow::NonClientIslandWindow(const ElementTheme& requestedTheme) | |||
| { | |||
| } | |||
|
|
|||
| NonClientIslandWindow::~NonClientIslandWindow() | |||
There was a problem hiding this comment.
Just added redundancy? I don't think we've got any code that was removed that was calling Close before, right?
There was a problem hiding this comment.
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, whileNonClientIslandWindowdoesn't have an explicit destructor, it gets one anyways which virtually overloads theIslandWindowdestructor. - With inheritance, destructors are called in reverse starting from child classes up to parent classes. In other words, first
~NonClientIslandWindowis 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 <migrie@microsoft.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
microsoft-github-policy-service
bot
added
Issue-Bug
|
FYI this PR sort of depends on #16575 because I had to (chose to?) remove |
| { | ||
| _windowLogic.ClearPersistedWindowState(); | ||
| } | ||
|
|
||
| // Remove ourself from the list of peasants so that we aren't included in |
lhecker
added
the
AutoMerge
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
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_APPmessage 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
AppHostinstance in the first place, as I imagined thatthis 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
AppHostand it could be something that maybe of interest in the future.
Closes #16332
Depends on #16587 and #16575