[regression] Button GDI (Font) leak #11037
Comments
kirsan31
added
the
untriaged
merriemcgaw
removed
the
untriaged
|
@kirsan31 every usage in GdiCache is in a using so I'm not sure how it could be the source of the leak. |
kirsan31
changed the title
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.FromFontThe problem is in Leaking object is HFONT:Must be disposed here: But if we look at dispose ending RemoveRef code: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 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 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 || font.IsSystemFont is the bug.
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. |
.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: