-
Notifications
You must be signed in to change notification settings - Fork 976
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
[regression] Button GDI (Font) leak #11037
Comments
@kirsan31 every usage in GdiCache is in a using so I'm not sure how it could be the source of the leak. |
You are right - last night my head wasn't working well and I jumped to conclusions. |
This comment was marked as resolved.
This comment was marked as resolved.
Never mind, I can download the project for now. thanks |
I did a research (with remote debugging) and still the problem is in System.Windows.Forms.Control.PaintWithErrorHandling
System.Windows.Forms.ButtonBase.OnPaint
System.Windows.Forms.ButtonInternal.ButtonStandardAdapter.PaintWorker
System.Windows.Forms.ButtonInternal.ButtonBaseAdapter+LayoutOptions.Layout
System.Windows.Forms.ButtonInternal.ButtonBaseAdapter+LayoutOptions.LayoutTextAndImage
System.Windows.Forms.ButtonInternal.ButtonBaseAdapter+LayoutOptions.GetTextSize
System.Windows.Forms.TextRenderer.MeasureTextInternal
System.Windows.Forms.GdiCache.GetHFONT
System.Windows.Forms.GdiCache.GetHFONT
System.Windows.Forms.FontCache.GetEntry
System.Windows.Forms.RefCountedCache`3
System.Windows.Forms.RefCountedCache`3
System.Windows.Forms.FontCache.CreateEntry
System.Windows.Forms.FontCache+Data.ctor
System.Windows.Forms.FontCache+Data.FromFont The problem is in winforms/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/RefCountedCache.cs Lines 128 to 134 in 5ccae85
Leaking object is HFONT :winforms/src/System.Windows.Forms/src/System/Windows/Forms/Internal/Gdi/FontCache.Data.cs Line 28 in 5ccae85
Must be disposed here:
But if we look at dispose ending RemoveRef code:winforms/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/RefCountedCache.CacheEntry.cs Lines 51 to 56 in 5ccae85
we will not clear because of _cached is true here (remember that we call CreateEntry(key, cached: true) ).
So, we will cache a new object in ----UPD---- This rise 2 additional questions:
Personally I don't know how to deal with creating a new default font on every reconnection - expert needed :) But about 1 question It seems to me that it makes more sense to cache fonts by value rather than by instance (compare fonts in |
@kirsan31 It was probably paranoia on my part to not compare via Equals. As long as we can be confident that we get the same HFONT for any Fonts that match that way we can probably change it. |
winforms/src/System.Drawing.Common/src/System/Drawing/Font.cs Lines 244 to 249 in 2c4d171
winforms/src/System.Windows.Forms/src/System/Windows/Forms/Internal/Gdi/FontCache.Data.cs Line 66 in 5ccae85
using FontFamily , Style , GdiCharSet and SizeInPoints . I am a bit worry about SizeInPoints and Unit != GraphicsUnit.Point and PM DPI aware app:
winforms/src/System.Drawing.Common/src/System/Drawing/Font.cs Lines 730 to 748 in 2c4d171
But I think that as we calculate result |
@kirsan31 We're kind of stuck with One of the difficulties here now that I think about it is related to performance. Doing a full field comparison is going to slow the cache down significantly. I'm not sure we want to do that. Scaling the default font seems like a place we can do something that might help your scenario. I'm not sure why we're messing with it when system colors change, but we could at least not reset the thing if it doesn't actually change. Just change |
This comment was marked as outdated.
This comment was marked as outdated.
This was changed on Main for now: winforms/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs Lines 12878 to 12899 in 561af9e
winforms/src/System.Windows.Forms/src/System/Windows/Forms/Application.cs Lines 1217 to 1223 in 561af9e
winforms/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/ScaleHelper.cs Lines 221 to 225 in 561af9e
But the problem still exists. I think must be like this: // in Control.cs
if (Application.DefaultFont is null)
{
if (s_defaultFont is not null)
{
Font font = SystemFonts.MessageBoxFont!;
if (!s_defaultFont.Equals(font))
{
s_defaultFont = font;
}
else
{
font.Dispose();
}
}
}
else
{
Application.ScaleDefaultFont();
s_defaultFont = Application.DefaultFont;
}
// in Application.cs
internal static void ScaleDefaultFont()
{
Font? font = ScaleHelper.ScaleToSystemTextSize(s_defaultFont);
if (font is null || !font.Equals(s_defaultFontScaled))
{
s_defaultFontScaled?.Dispose();
s_defaultFontScaled = font;
}
else
{
font.Dispose();
}
} Also I found 1 bug and 1 potential bug in winforms/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/ScaleHelper.cs Lines 221 to 226 in 561af9e
|| font.IsSystemFont is the bug.
winforms/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/ScaleHelper.cs Lines 234 to 240 in 561af9e
I think here is more safter to use newSystemFont.Equals(font) than newSystemFont.Size == font.Size ?
I can open a pull request on all of this, if you don't mind. |
@kirsan31 opening a PR would be great. |
But for FontConfigTest.mp4 |
If the fix already included in 9.0.100-rc.1.24406.14 then everything is correct. Otherwise, our discission on this topic stopped here: #11206 (comment) |
We checked the fixing flow process, 9.0.100-rc.1.24406.14 SDK contains your PR: #11206. |
Then everything are ok. Before my fix, |
Got it, thanks for confirm. |
Verified the issue with .NET 9.0.100-rc.1.24422.10 test pass build that the issue has been fixed, which have the same results as above. |
Verified this issue with .NET 9.0.100-rc.1.24452.12 test pass build, it has been fixed and the test result is same as above. |
.NET version
5.0+
Did it work in .NET Framework?
Yes
Did it work in any of the earlier releases of .NET Core or .NET 5+?
Work in all versions before 5.0.
Issue description
Every RDP connect (WM_SETTINGCHANGE I think) we leak 1 GDI Font object for Button.
HUD stack trace for this leaked font:
Steps to reproduce
GDI_test2.zip
The text was updated successfully, but these errors were encountered: