Fix GDI handle leak in Icon.DrawImage #47836
Conversation
|
Tagging subscribers to this area: @safern, @tannergooding Issue DetailsFixes: #46789 This was a regression from 3.1 -> 5.0 caused in: dotnet/corefx@f807df6
|
|
Could we test for this and maybe other leaks? |
Let me think on some ideas with @JeremyKuhne how we can test this. |
|
GetGuiResources should let you check on handle usage. You'd have to not run the regression test concurrently or kick it off out-of-proc. I haven't used this API myself yet, but given this regression I'm now committing some thought to it. For 6.0 it might be worth considering introducing disposables to help ensure closure of handles. I call them "scopes" in WinForms, and I flip between disposable ref structs in release and finalizable classes in debug to help shake out where we didn't use them right (didn't put in a using). I found that the majority of usages in WinForms could be scoped to the stack with not much refactoring. In rolling scopes out in WinForms I found a number of leaks, particularly in cases where we were trying to juggle more than one resource. It also fixed some ordering issues in those cases- ensuring we release in the reverse order of when they were acquired. |
|
Maybe we can add a test for |
safern
force-pushed
the
FixGdiHandleLeak
branch
from
b79a0dc
to
299a5fd
Compare
|
I added a test for this specific scenario. Validated that without the fix the test fails, for other possible leaks, I will open an issue to address for 6.0. PTAL. |
safern
force-pushed
the
FixGdiHandleLeak
branch
from
299a5fd
to
d568908
Compare
|
Hmmm interesting that |
Is it possible that the GC kicked in before that assertion and cleaned up all your unreferenced drawing types since they are no longer used in the rest of the method body? Maybe try adding GC.KeepAlive on them after the assertion. |
I'll try that and see if that's it. |
|
Interesting, it was not the GC. It was an error on the native call to get the handle count as I was caching the current process handle and reusing it. Calling |
Sounds like just another type of GC problem. Process itself is disposable, you captured the IntPtr handle but didn't keep Process rooted. So Process was finalized which closed the handle. This makes more sense, the GC had plenty of time to run while you were looping. A better fix would be to add a using for the Process. |
|
Makes sense. |
|
Not sure why Helix thinks these tests failed. I see all tests passing and runner returning 0. Is something going on with Helix on OSX? cc @MattGal |
Taking a look, @safern was just showing me some network flakiness on two mac work items and this may be them. |
|
@ericstj yes these are the same two strange timeouts @safern showed me. Given the symptoms, it's most likely network flakiness in the lab where these macs are hosted while communicating with Event Hub (in such a way that it never returns). If it happens more than twice (or, say, on the same machines repeatedly) we can open up an issue on dotnet/core-eng to have DDFUN investigate the hardware. One possible thing you could do quickly would be to stop using XUnit reporter by setting EnableXunitReporter=false, since the main workflow seems to look at test failures in AzDO anyways. This of course depends on whether folks still use this mode of test result analysis. |
Would disabling that, stop pushing results to Kusto? Cause we do use Kusto still for some things. |
yes, that's what XUnitReporter does. |
ghost
locked as resolved and limited conversation to collaborators
Fixes: #46789
This was a regression from 3.1 -> 5.0 caused in: dotnet/corefx@f807df6
I will need to port this to 5.0 as this is affecting winforms costumers directly and apps could just die if we exceed the GDI+ handle limit.