winforms icon indicating copy to clipboard operation
winforms copied to clipboard
verified publisher icon dotnet

[regression] Button GDI (Font) leak

Open kirsan31 opened this issue 1 year ago • 4 comments

.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:

# Handles	Call Stack	Module	Source	Type	StackTag
1	RtlUserThreadStart	ntdll.dll		Native	
1	BaseThreadInitThunk	kernel32.dll		Native	
1	00007FF6AB2112E8	gdi_test.exe		unknown	
1	00007FF6AB20FDA6	gdi_test.exe		unknown	
1	00007FF6AB20F998	gdi_test.exe		unknown	
1	hostfxr_main_startupinfo	hostfxr.dll	D:\a\_work\1\s\src\native\corehost\fxr\hostfxr.cpp(62)	Native	
1	fx_muxer_t::execute	hostfxr.dll	D:\a\_work\1\s\src\native\corehost\fxr\fx_muxer.cpp(578)	Native	
1	fx_muxer_t::handle_exec_host_command	hostfxr.dll	D:\a\_work\1\s\src\native\corehost\fxr\fx_muxer.cpp(1007)	Native	
1	std::pair<wchar_t const * __ptr64,std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > >::~pair<wchar_t const * __ptr64,std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > >	hostfxr.dll	D:\a\_work\1\s\src\native\corehost\fxr\fx_muxer.cpp(532)	Native	
1	propagate_error_writer_t::~propagate_error_writer_t	hostfxr.dll	D:\a\_work\1\s\src\native\corehost\fxr\fx_muxer.cpp(145)	Native	
1	corehost_main	hostpolicy.dll	D:\a\_work\1\s\src\native\corehost\hostpolicy\hostpolicy.cpp(426)	Native	
1	run_app	hostpolicy.dll	D:\a\_work\1\s\src\native\corehost\hostpolicy\hostpolicy.cpp(285)	Native	
1	run_app_for_context	hostpolicy.dll	D:\a\_work\1\s\src\native\corehost\hostpolicy\hostpolicy.cpp(256)	Native	
1	coreclr_execute_assembly	coreclr.dll	D:\a\_work\1\s\src\coreclr\dlls\mscoree\exports.cpp(504)	Native	
1	CorHost2::ExecuteAssembly	coreclr.dll	D:\a\_work\1\s\src\coreclr\vm\corhost.cpp(349)	Native	
1	Assembly::ExecuteMainMethod	coreclr.dll	D:\a\_work\1\s\src\coreclr\vm\assembly.cpp(1504)	Native	
1	RunMain	coreclr.dll	D:\a\_work\1\s\src\coreclr\vm\assembly.cpp(1375)	Native	
1	CorHost2::ExecuteAssembly	coreclr.dll	D:\a\_work\1\s\src\coreclr\vm\assembly.cpp(1304)	Native	
1	MethodDescCallSite::CallTargetWorker	coreclr.dll	D:\a\_work\1\s\src\coreclr\vm\callhelpers.cpp(570)	Native	
1	CallDescrWorkerInternal	coreclr.dll	D:\a\_work\1\s\src\coreclr\vm\amd64\CallDescrWorkerAMD64.asm(100)	Native	
1	Designer_Chart_test.Program.Main			Managed (NGEN)	
1	System.Windows.Forms.Application+ThreadContext.RunMessageLoop	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.Application+ThreadContext.RunMessageLoopInner	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.Application+ComponentManager.Microsoft.Office.IMsoComponentManager.FPushMessageLoop	system.windows.forms.dll		Managed (NGEN)	
1	00007FF8D35F05A9	system.windows.forms.primitives.dll		unknown	
1	DispatchMessageWorker	user32.dll		Native	
1	NtUserDispatchMessage	win32u.dll		Native	
1	KiSystemServiceCopyEnd	ntoskrnl.exe		Native (Kernel)	
1	NtUserDispatchMessage	win32k.sys		Native (Kernel)	
1	NtUserDispatchMessage	win32kfull.sys		Native (Kernel)	
1	xxxDispatchMessage	win32kfull.sys		Native (Kernel)	
1	SfnDWORD	win32kfull.sys		Native (Kernel)	
1	KeUserModeCallback	ntoskrnl.exe		Native (Kernel)	
1	KiUserCallbackDispatcherContinue	ntdll.dll		Native	
1	__fnDWORD	user32.dll		Native	
1	DispatchClientMessage	user32.dll		Native	
1	UserCallWinProcCheckWow	user32.dll		Native	
1	dynamicClass.IL_STUB_ReversePInvoke			Managed (NGEN)	
1	System.Windows.Forms.NativeWindow.Callback			Managed (NGEN)	
1	System.Windows.Forms.ButtonBase.WndProc			Managed (NGEN)	
1	System.Windows.Forms.Control.WndProc			Managed (NGEN)	
1	System.Windows.Forms.Control.WmPaint	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.Control.PaintWithErrorHandling			Managed (NGEN)	
1	System.Windows.Forms.ButtonBase.OnPaint	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.ButtonInternal.ButtonStandardAdapter.PaintWorker	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.ButtonInternal.ButtonBaseAdapter+LayoutOptions.Layout	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.ButtonInternal.ButtonBaseAdapter+LayoutOptions.LayoutTextAndImage	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.ButtonInternal.ButtonBaseAdapter+LayoutOptions.GetTextSize	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.TextRenderer.MeasureTextInternal	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.GdiCache.GetHFONT	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.GdiCache.GetHFONT	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.FontCache.GetEntry	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.RefCountedCache`3[Windows.Win32.Graphics.Gdi.HFONT,System.Windows.Forms.FontCache+Data,System.ValueTuple`2[System.__Canon,Windows.Win32.Graphics.Gdi.FONT_QUALITY]].GetEntry			Managed (JIT)	
1	System.Windows.Forms.RefCountedCache`3[Windows.Win32.Graphics.Gdi.HFONT,System.Windows.Forms.FontCache+Data,System.ValueTuple`2[System.__Canon,Windows.Win32.Graphics.Gdi.FONT_QUALITY]].<GetEntry>g__Add|8_1			Managed (JIT)	
1	System.Windows.Forms.FontCache.CreateEntry	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.FontCache+Data.ctor	system.windows.forms.dll		Managed (NGEN)	
1	System.Windows.Forms.FontCache+Data.FromFont	system.windows.forms.dll		Managed (NGEN)	
1	00007FF8D35E8AA9	system.windows.forms.primitives.dll		unknown	
1	CreateFontIndirectWImpl	gdi32full.dll		Native	
1	CreateFontIndirectExW	gdi32full.dll		Native	
1	NtGdiHfontCreate	win32u.dll		Native	
1	KiSystemServiceCopyEnd	ntoskrnl.exe		Native (Kernel)	
1	NtGdiHfontCreate	win32k.sys		Native (Kernel)	
1	NtGdiHfontCreate	win32kfull.sys		Native (Kernel)	
1	hfontCreate	win32kfull.sys		Native (Kernel)	
1	HmgInsertObjectHelper::Insert	win32kfull.sys		Native (Kernel)	
1	HmgInsertObjectInternal	win32kbase.sys		Native (Kernel)	
1	DirectComposition::CSharedWriteScalarMarshaler::`vector deleting destructor'	win32kbase.sys		Native (Kernel)	
1	McTemplateK0pqqq_EtwWriteTransfer	win32kbase.sys		Native (Kernel)	
1	McGenEventWrite_EtwWriteTransfer	win32kbase.sys		Native (Kernel)	
1	EtwWriteTransfer	ntoskrnl.exe		Native (Kernel)

Steps to reproduce

GDI_test2.zip

kirsan31 avatar Mar 12 '24 16:03 kirsan31

@kirsan31 every usage in GdiCache is in a using so I'm not sure how it could be the source of the leak.

JeremyKuhne avatar Mar 12 '24 20:03 JeremyKuhne

@JeremyKuhne

@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. More of it when I looked at stack trace again - I saw that the problem was in the Button and not in the PropertyGrid. I updated 1 post and sample app too.

kirsan31 avatar Mar 13 '24 06:03 kirsan31

I see that sample app can't be downloaded from 1 post and reuploading didn't help :( GDI_test.zip Repac and adding it here didn't help too - GitHub problem? Where else I can upload test app?

kirsan31 avatar Mar 13 '24 07:03 kirsan31

I see that sample app can't be downloaded from 1 post and reuploading didn't help :( GDI_test.zip Repac and adding it here didn't help too - GitHub problem? Where else I can upload test app?

Never mind, I can download the project for now. thanks

Zheng-Li01 avatar Mar 13 '24 07:03 Zheng-Li01

@JeremyKuhne

every usage in GdiCache is in a using so I'm not sure how it could be the source of the leak.

I did a research (with remote debugging) and still the problem is in FontCache despite the using... From call stack:

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 FontCache (note that cached is true):
https://github.com/dotnet/winforms/blob/5ccae85490be97a3a597c7e0df99dd9f89078999/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/RefCountedCache.cs#L128-L134 Leaking object is HFONT:
https://github.com/dotnet/winforms/blob/5ccae85490be97a3a597c7e0df99dd9f89078999/src/System.Windows.Forms/src/System/Windows/Forms/Internal/Gdi/FontCache.Data.cs#L28 Must be disposed here:
https://github.com/dotnet/winforms/blob/5ccae85490be97a3a597c7e0df99dd9f89078999/src/System.Windows.Forms/src/System/Windows/Forms/TextRenderer.cs#L519 But if we look at dispose ending RemoveRef code:
https://github.com/dotnet/winforms/blob/5ccae85490be97a3a597c7e0df99dd9f89078999/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/RefCountedCache.CacheEntry.cs#L51-L56 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 FontCache every rdp reconnect until we reach _softLimit (in our case it's 20) after that FontCache will be cleared. So, strictly speaking this is not a leak, BUT... The main question remaining here is - why we create a new FontCache entry every rdp reconnect and not using existing entries?

----UPD----

image

This rise 2 additional questions:

  1. Is this by design that we cache instances and not values?
  2. Why (from where) we got new Font instances every rdp reconnect? The answer is that on every rdp connect we got this notification: https://github.com/dotnet/winforms/blob/5ccae85490be97a3a597c7e0df99dd9f89078999/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs#L11740-L11748 where we null out s_defaultFont therefore - create a new one on next Font query.

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 IsMatch with Font.Equals) 🤔 /cc @JeremyKuhne

kirsan31 avatar Mar 30 '24 19:03 kirsan31

@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.

JeremyKuhne avatar Apr 03 '24 20:04 JeremyKuhne

@JeremyKuhne

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.

Equals: https://github.com/dotnet/winforms/blob/2c4d1710171a361776bcd277a7dc1d1590fb959b/src/System.Drawing.Common/src/System/Drawing/Font.cs#L244-L249

https://github.com/dotnet/winforms/blob/5ccae85490be97a3a597c7e0df99dd9f89078999/src/System.Windows.Forms/src/System/Windows/Forms/Internal/Gdi/FontCache.Data.cs#L66 using FontFamily, Style, GdiCharSet and SizeInPoints. I am a bit worry about SizeInPoints and Unit != GraphicsUnit.Point and PM DPI aware app:

https://github.com/dotnet/winforms/blob/2c4d1710171a361776bcd277a7dc1d1590fb959b/src/System.Drawing.Common/src/System/Drawing/Font.cs#L730-L748

But I think that as we calculate result emHeightInPixels / pixelsPerPoint we will get monitor (DPI) independent value - right? Anyway - if we got different results from font1 and font2 when Font.Equals(font1, font2) == true then we have a bug in Equals isn't it?

kirsan31 avatar Apr 03 '24 20:04 kirsan31

@kirsan31 We're kind of stuck with Font.Equals behavior for all time.

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 ScaleDefaultFont to take s_defaultFont and have it return the passed in Font if it wouldn't actually change.

JeremyKuhne avatar Apr 03 '24 21:04 JeremyKuhne

deleted

kirsan31 avatar Apr 04 '24 13:04 kirsan31

@JeremyKuhne

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 ScaleDefaultFont to take s_defaultFont and have it return the passed in Font if it wouldn't actually change.

This was changed on Main for now: https://github.com/dotnet/winforms/blob/561af9e5eea9ed910db6caa9610ca794d69d979e/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs#L12878-L12899 https://github.com/dotnet/winforms/blob/561af9e5eea9ed910db6caa9610ca794d69d979e/src/System.Windows.Forms/src/System/Windows/Forms/Application.cs#L1217-L1223 https://github.com/dotnet/winforms/blob/561af9e5eea9ed910db6caa9610ca794d69d979e/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/ScaleHelper.cs#L221-L225

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 ScaleToSystemTextSize: https://github.com/dotnet/winforms/blob/561af9e5eea9ed910db6caa9610ca794d69d979e/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/ScaleHelper.cs#L221-L226 || font.IsSystemFont is the bug.

https://github.com/dotnet/winforms/blob/561af9e5eea9ed910db6caa9610ca794d69d979e/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/ScaleHelper.cs#L234-L240 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 avatar Apr 05 '24 07:04 kirsan31

@kirsan31 opening a PR would be great.

JeremyKuhne avatar Apr 05 '24 17:04 JeremyKuhne

Verified in the latest .NET 9.0 SDK build: 9.0.100-rc.1.24407.28 to check GDI objects for every rdp connection, it is not increased. image

Olina-Zhang avatar Aug 08 '24 08:08 Olina-Zhang

But for SysFont and SysFontDef config testing when change text size in windows (Settings-Accessibility-Text size) on VM, the results are still same:

https://github.com/user-attachments/assets/19934f46-78fd-4934-9eec-9ab7bc22f1bd

Olina-Zhang avatar Aug 08 '24 09:08 Olina-Zhang

But for SysFont and SysFontDef config testing when change text size in windows (Settings-Accessibility-Text size) on VM, the results are still same:

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: https://github.com/dotnet/winforms/pull/11206#issuecomment-2193962783

kirsan31 avatar Aug 08 '24 09:08 kirsan31

We checked the fixing flow process, 9.0.100-rc.1.24406.14 SDK contains your PR: https://github.com/dotnet/winforms/pull/11206.

Olina-Zhang avatar Aug 08 '24 09:08 Olina-Zhang

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, SysFontDef will not increase font: image

kirsan31 avatar Aug 08 '24 09:08 kirsan31

Got it, thanks for confirm.

Olina-Zhang avatar Aug 08 '24 09:08 Olina-Zhang

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.

Syareel-Sukeri avatar Aug 26 '24 09:08 Syareel-Sukeri