Stop NativeImageList GDI leak

[RussKie]

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

After

Microsoft Reviewers: Open in CodeFlow

[RussKie]

/cc: @kirsan31 @weltkante

[kirsan31]

I think this in scope (may be I am wrong). Was introduced in same PR #3526. Is it does anything?

winforms/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs

Line 529 in 706b64a

ImageStream?.Dispose();

Due to ImageStream property implementation:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs

Lines 197 to 209 in 706b64a

	public ImageListStreamer ImageStream
	{
	get
	{
	if (Images.Empty)
	{
	return null;
	}
	// No need for us to create the handle, because any serious attempts to use the
	// ImageListStreamer will do it for us.
	return new ImageListStreamer(this);
	}

[RussKie]

I think this in scope (may be I am wrong). Was introduced in same PR #3526. Is it does anything?

Apologies, I am struggling to follow your train of thought/question. Could you please rephrase?

[kirsan31]

I think this in scope (may be I am wrong). Was introduced in same PR #3526. Is it does anything?

Apologies, I am struggling to follow your train of thought/question. Could you please rephrase?

The regression was introduced in #3526. Also in this PR this line of code was added to ImageList.Dispose(bool disposing) method:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs

Line 529 in 706b64a

ImageStream?.Dispose();

and this line does nothing (useful), due to ImageStream property implementation:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs

Lines 197 to 209 in 706b64a

	public ImageListStreamer ImageStream
	{
	get
	{
	if (Images.Empty)
	{
	return null;
	}
	// No need for us to create the handle, because any serious attempts to use the
	// ImageListStreamer will do it for us.
	return new ImageListStreamer(this);
	}

🤗

[RussKie]

The regression was introduced in #3526. Also in this PR this line of code was added to ImageList.Dispose(bool disposing) method:
...
and this line does nothing (useful), due to ImageStream property implementation:

Ah, yes! Thank you. Removed

[JeremyKuhne]

It would be nice if we could avoid introducing more BinaryFormatter usage here (even in tests). We'll need an answer at some point for ImageListStreamer. I'd suggest adding an internal void ImageListStreamer.ToStream(Stream stream) and internal ImageListStream(Stream stream) to avoid the BF. Should be straight forward to implement and we can follow up with an API proposal for it.

Outside of that there are a few minor comments.

[RussKie]

It would be nice if we could avoid introducing more BinaryFormatter usage here (even in tests). We'll need an answer at some point for ImageListStreamer. I'd suggest adding an internal void ImageListStreamer.ToStream(Stream stream) and internal ImageListStream(Stream stream) to avoid the BF. Should be straight forward to implement and we can follow up with an API proposal for it.

I'm reusing the existing test infra that is relying on the BF. Since this is a separate piece of work not directly related to this fix I'm going to do this in a separate PR.

[RussKie]

Reworked the BF in #4966

[kirsan31]

@RussKie Is this still servicing-consider for 5.0.x?

[RussKie]

I've requested a permission to service, will find out soon.

[dreddy-work]

Approved for servicing. We need a servicing PR. @RussKie