Fix font preview for conhost #16324
Fix font preview for conhost #16324
Conversation
microsoft-github-policy-service
bot
added
Issue-Bug
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
|
I've tried hard to find the leak as reported initially and just couldn't spot anything even after a long time. Here's the original GDI object leak trace from the internal report: It's unlikely that the tool is incorrect, but clearly the conclusions we've drawn initially were wrong. This isn't how to fix the leak. But what other code could cause this? Apart from this function the only other culprits I can find are |
microsoft-github-policy-service
bot
added
Needs-Attention
lhecker
added
AutoMerge
|
I do not think we can fix regression here by introducing another regression. The origin regression caused the app to crash so it was way worse than the current issue. I think you can fix it better way by finding where the GDI object address has been copied and only delete it when you no longer need it. Also can you please share with me the steps to repro the issue? i want to repro locally? |
FWIW the reason the leak-fix works is because it frees the previously added handle while a new one is being added. Then, during exit, we'll free all those already freed handles again (
I'm not entirely sure I understand what you mean. The GDI objects that caused the leak are only found in that particular list and they aren't copied anywhere else. So theoretically, the "reference count" so to say should always be 1. They are however copied around inside the list a lot via
See the original issue #16297. I'm not entirely sure in which build the original leak fix is available, but the most recent internal dev builds should definitely have it. If you're on such a 25xxx build you can just launch the inbox |
|
I cannot repro issue. in my local dev branch which has changes
|
That is the issue. That font is not Consolas. That is an impostor. 😄 |
|
Actually you can! Look closely at the first screenshot. That's not Consolas, but rather a bitmap font. 🙂 |
|
Okey my short answer here is that we know we have two problems, you cannot fix one regression and introduce another regression you know of. This is not a fix. You need to find a way to release GDI objects and avoid app crashes for reaching GDI objects limit that the first PR was for instead of just removing code. |
|
Okay, here's the skinny.
We're committing it for now and following up with an investigation into 3-4. |
|
Ah, this failed cherry-pick into 1.19 because 1.19 hadn't merged inbox. I've brought inbox back into 1.19 and am bringing this PR in, to bring it back into inbox. 😵💫 |
After exiting the main loop in this function the invariant
`nFont <= NumberOfFonts` still holds true. Additionally,
preceding this removed code is this (paraphrased):
```cpp
if (nFont < NumberOfFonts) {
RtlMoveMemory(...);
}
```
It ensures that the given slot `nFont` is always unoccupied by moving
it and all following items upwards if needed. As such, the call to
`DeleteObject` is always incorrect, as the slot is always "empty",
but may contain a copy of the previous occupant due to the `memmove`.
This regressed in 154ac2b.
Closes #16297
## Validation Steps Performed
* All fonts have a unique look in the preview panel ✅
(cherry picked from commit 35240f2)
Service-Card-Id: 91120871
Service-Version: 1.19
After exiting the main loop in this function the invariant
nFont <= NumberOfFontsstill holds true. Additionally,preceding this removed code is this (paraphrased):
It ensures that the given slot
nFontis always unoccupied by movingit and all following items upwards if needed. As such, the call to
DeleteObjectis always incorrect, as the slot is always "empty",but may contain a copy of the previous occupant due to the
memmove.This regressed in 154ac2b.
Closes #16297
Validation Steps Performed