Skip to content
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

[Release/5.0] Fix GDI handle leak in Graphics.DrawIcon #48189

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

safern
Copy link
Member

@safern safern commented Feb 11, 2021

Backport of #47836
Fixes #46789

Customer Impact

Customer reported a GDI resource leak in their WinForms application after they targeted it to NET 5.0.

In any application that draws a lot of icons to a graphics object associated to the current device context we would leak GDI+ handles as observed in the issue, which could potentially cause application crashes if we exceed the GDI handles limit imposed by the OS.

This impacts winforms as System.Drawing.Common is part of their shared framework.

Regression?

Yes, caused by dotnet/corefx@f807df6 in 5.0.

Testing

Manual testing and also included a regression test which was failing before the fix, the test and fix have been in master for about a week already and the test is pretty stable, we haven't seen failures from it or any other System.Drawing test.

Risk

Low, the fix is just making sure we call GDI.DeleteObject on a handle that is no longer used.

cc: @danmoseley @ericstj @Anipik

@safern safern added the Servicing-consider Issue for next servicing release review label Feb 11, 2021
@safern safern added this to the 5.0.x milestone Feb 11, 2021
@safern safern requested a review from JeremyKuhne February 11, 2021 22:25
@ghost
Copy link

ghost commented Feb 11, 2021

Tagging subscribers to this area: @safern, @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #47836
Fixes #46789

Customer Impact

In any application that draws a lot of icons to a graphics object associated to the current device context we would leak GDI+ handles as observed in the issue, which could potentially cause application crashes if we exceed the GDI handles limit imposed by the OS.

Regression?

Yes, caused by dotnet/corefx@f807df6 in 5.0.

Testing

Manual testing and also included a regression test which was failing before the fix, the test and fix have been in master for about a week already and the test is pretty stable, we haven't seen failures from it or any other System.Drawing test.

Risk

Low, the fix is just making sure we call GDI.DeleteObject on a handle that is no longer used.

cc: @danmoseley @ericstj @Anipik

Author: safern
Assignees: -
Labels:

Servicing-consider, area-System.Drawing

Milestone: 5.0.x

@danmoseley
Copy link
Member

Updated template to note this was customer reported.

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@safern safern added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 12, 2021
@safern
Copy link
Member Author

safern commented Feb 12, 2021

It was approved over email

@safern safern merged commit 5111aa1 into dotnet:release/5.0 Feb 12, 2021
@safern safern deleted the PortGdiMemoryLeakFix branch February 12, 2021 01:41
@safern
Copy link
Member Author

safern commented Feb 12, 2021

Failures are unrelated.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2021
@danmoseley danmoseley modified the milestones: 5.0.x, 5.0.4 Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Drawing Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants