Skip to content

Stop NativeImageList GDI leak (servicing)#4975

Merged
RussKie merged 1 commit intodotnet:release/5.0from
RussKie:fix_3567_gdi_leak_50
May 26, 2021
Merged

Stop NativeImageList GDI leak (servicing)#4975
RussKie merged 1 commit intodotnet:release/5.0from
RussKie:fix_3567_gdi_leak_50

Conversation

@RussKie
Copy link
Contributor

@RussKie RussKie commented May 25, 2021

Fixes #3567
Fixes #4913
Closes #4316
Closes #4928

Proposed changes

There are certain code paths where we are unable to track the lifetime of the object,
for example in the following scenarios:

 this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));

or

 resources.ApplyResources(this.listView1, "listView1");

In those cases the loose instances will be collected by the GC.

Undo the regression introduced in #3526.

Customer Impact

  • Customer applications should no longer crash due to exhausted pool of GDI handles used by NativeImageList objects.

Regression?

  • Yes

Risk

  • Minimal

Test methodology

  • added unit and MAUI tests to verify both disposal and GC scenarios
  • a lot of manual tests

Manual tests harness:

diff --git a/src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/MainForm.cs b/src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/MainForm.cs
index 02ed97ff4..68d0ef6db 100644
--- a/src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/MainForm.cs
+++ b/src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/MainForm.cs
@@ -123,7 +123,18 @@ namespace WinformsControlsTest
             },
             {
                 MainFormControlsTabOrder.ListViewButton,
-                new InitInfo("ListVew", (obj, e) => new ListViewTest().Show())
+                new InitInfo("ListVew", (obj, e) =>
+                {
+                    int gdiHandleCount = Interop.User32.GetGuiResources(Process.GetCurrentProcess().Handle, Interop.User32.GR.GDIOBJECTS);
+                    Debug.WriteLine($"before: {gdiHandleCount}");
+                    using ListViewTest form = new ();
+                    form.ShowDialog();
+                    GC.Collect();
+                    GC.WaitForPendingFinalizers();
+                    GC.Collect(0);
+                    gdiHandleCount = Interop.User32.GetGuiResources(Process.GetCurrentProcess().Handle, Interop.User32.GR.GDIOBJECTS);
+                    Debug.WriteLine($"after: {gdiHandleCount}");
+                })
             },

Before

gdi1

After

gdi2

Microsoft Reviewers: Open in CodeFlow

There are certain code paths where we are unable to track the lifetime of the object,
for example in the following scenarios:

     this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));

or

     resources.ApplyResources(this.listView1, "listView1");

In those cases the loose instances will be collected by the GC.

Undo the regression introduced in  dotnet#3526.
@RussKie RussKie requested a review from a team as a code owner May 25, 2021 23:51
@ghost ghost assigned RussKie May 25, 2021
@RussKie RussKie added this to the 5.0.8 milestone May 25, 2021
@RussKie RussKie added the servicing-approved .NET Shiproom approved the PR for merge label May 25, 2021
@RussKie
Copy link
Contributor Author

RussKie commented May 25, 2021

Approved for servicing over the email

@RussKie RussKie changed the title Stop NativeImageList GDI leak Stop NativeImageList GDI leak (servicing) May 25, 2021
@RussKie RussKie linked an issue May 25, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #4975 (c02bf45) into release/5.0 (9fb50b3) will decrease coverage by 0.01164%.
The diff coverage is 91.66667%.

@@                  Coverage Diff                  @@
##           release/5.0       #4975         +/-   ##
=====================================================
- Coverage     68.41633%   68.40469%   -0.01164%     
=====================================================
  Files             1421        1422          +1     
  Lines           511052      511060          +8     
  Branches         41587       41586          -1     
=====================================================
- Hits            349643      349589         -54     
- Misses          154992      155057         +65     
+ Partials          6417        6414          -3     
Flag Coverage Δ
Debug 68.40469% <91.66667%> (-0.01164%) ⬇️
production 37.83160% <80.00000%> (-0.02446%) ⬇️
test 97.90758% <100.00000%> (+0.00005%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@RussKie RussKie merged commit c5171f5 into dotnet:release/5.0 May 26, 2021
@RussKie RussKie deleted the fix_3567_gdi_leak_50 branch May 26, 2021 01:26
@ghost ghost locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

servicing-approved .NET Shiproom approved the PR for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NET5 ImageList GDI object leak [regression] PropertyGrid GDI leak NativeImageList not disposed by ResourceManager Leak of NativeImageList

2 participants