Solve image list woes

src/System.Windows.Forms.Primitives/src/Interop/ComCtl32/Interop.ImageList.Duplicate.cs

@@ -14,13 +14,6 @@ public static partial class ImageList
         {
             [DllImport(Libraries.Comctl32, ExactSpelling = true, EntryPoint = "ImageList_Duplicate")]
             public static extern IntPtr Duplicate(IntPtr himl);
-
-            public static IntPtr Duplicate(IHandle himl)
-            {
-                IntPtr result = Duplicate(himl.Handle);
-                GC.KeepAlive(himl);
-                return result;
-            }
         }
     }
 }

src/System.Windows.Forms/src/Resources/SR.resx

@@ -3344,6 +3344,9 @@ Stack trace where the illegal operation occurred was:
   <data name="ImageListCreateFailed" xml:space="preserve">
     <value>Creation of the ImageList handle did not succeed.</value>
   </data>
+  <data name="ImageListDuplicateFailed" xml:space="preserve">
+    <value>Duplication of the ImageList handle did not succeed.</value>
+  </data>
   <data name="ImageListEntryType" xml:space="preserve">
     <value>Image added to an ImageList must either derive from Image or be an Icon.</value>
   </data>

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

@@ -1,48 +1,112 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
+using System.Diagnostics;
+using System.Drawing;
 using static Interop;
+using static Interop.ComCtl32;
 
 namespace System.Windows.Forms
 {
     public sealed partial class ImageList
     {
         internal class NativeImageList : IDisposable, IHandle
         {
-            private IntPtr _himl;
-#if DEBUG
-            private readonly string _callStack;
-#endif
+            private const int GrowBy = 4;
+            private const int InitialCapacity = 4;
+
+            private static readonly object s_syncLock = new object();
+
+            public NativeImageList(Ole32.IStream pstm)
+            {
+                IntPtr himl;
+                lock (s_syncLock)
+                {
+                    himl = ComCtl32.ImageList.Read(pstm);
+                    Init(himl);
+                }
+            }
 
-            internal NativeImageList(IntPtr himl)
+            public NativeImageList(Size imageSize, ILC flags)
             {
-                _himl = himl;
+                IntPtr himl;
+                lock (s_syncLock)
+                {
+                    himl = ComCtl32.ImageList.Create(imageSize.Width, imageSize.Height, flags, InitialCapacity, GrowBy);
+                    Init(himl);
+                }
+            }
+
+            private NativeImageList(IntPtr himl)
+            {
+                Handle = himl;
+            }
+
+            private void Init(IntPtr himl)
+            {
+                if (himl != IntPtr.Zero)
+                {
+                    Handle = himl;
+                    return;
+                }
+
 #if DEBUG
-                _callStack = Environment.StackTrace;
+                Debug.Fail($"{nameof(NativeImageList)} could not be created. Originating stack:\n{_callStack}");
 #endif
+                throw new InvalidOperationException(SR.ImageListCreateFailed);
             }
 
-            public IntPtr Handle => _himl;
+            public IntPtr Handle { get; private set; }
 
             public void Dispose()
             {
-                Dispose(true);
+                lock (s_syncLock)
+                {
+                    if (Handle == IntPtr.Zero)
+                    {
+                        return;
+                    }
+
+                    ComCtl32.ImageList.Destroy(Handle);
+                    Handle = IntPtr.Zero;
+                }
+
+#if DEBUG
                 GC.SuppressFinalize(this);
+#endif
+            }
+
+#if DEBUG
+            private readonly string _callStack = new StackTrace().ToString();
+
+            ~NativeImageList()
+            {
+                Debug.Fail($"{nameof(NativeImageList)} was not disposed properly. Originating stack:\n{_callStack}");
+
+                // We can't do anything with the fields when we're on the finalizer as they're all classes. If any of
+                // them become structs they'll be a part of this instance and possible to clean up. Ideally we fix
+                // the leaks and never come in on the finalizer.
+                return;
             }
+#endif
 
-            public void Dispose(bool disposing)
+            internal NativeImageList Duplicate()
             {
-                if (_himl != IntPtr.Zero)
+                lock (s_syncLock)
                 {
-                    ComCtl32.ImageList.Destroy(_himl);
-                    _himl = IntPtr.Zero;
+                    IntPtr himl = ComCtl32.ImageList.Duplicate(Handle);
+                    if (himl != IntPtr.Zero)
+                    {
+                        return new NativeImageList(himl);
+                    }
                 }
-            }
 
-            ~NativeImageList() => Dispose(false);
+#if DEBUG
+                Debug.Fail($"{nameof(NativeImageList)} could not be duplicated. Originating stack:\n{_callStack}");
+#endif
+                throw new InvalidOperationException(SR.ImageListDuplicateFailed);
+            }
         }
     }
 }

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

@@ -33,9 +33,6 @@ public sealed partial class ImageList : Component, IHandle
         private static readonly Color s_fakeTransparencyColor = Color.FromArgb(0x0d, 0x0b, 0x0c);
         private static readonly Size s_defaultImageSize = new Size(16, 16);
 
-        private const int InitialCapacity = 4;
-        private const int GrowBy = 4;
-
         private const int MaxDimension = 256;
         private static int s_maxImageWidth = MaxDimension;
         private static int s_maxImageHeight = MaxDimension;
@@ -235,7 +232,7 @@ public ImageListStreamer ImageStream
                     bool recreatingHandle = HandleCreated; // We only need to fire RecreateHandle if there was a previous handle
                     DestroyHandle();
                     _originals = null;
-                    _nativeImageList = new NativeImageList(ComCtl32.ImageList.Duplicate(himl));
+                    _nativeImageList = himl.Duplicate();
                     if (ComCtl32.ImageList.GetIconSize(new HandleRef(this, _nativeImageList.Handle), out int x, out int y).IsTrue())
                     {
                         _imageSize = new Size(x, y);
@@ -460,18 +457,20 @@ private void CreateHandle()
             try
             {
                 ComCtl32.InitCommonControls();
-                _nativeImageList = new NativeImageList(ComCtl32.ImageList.Create(_imageSize.Width, _imageSize.Height, flags, InitialCapacity, GrowBy));
+
+                if (_nativeImageList != null)
+                {
+                    _nativeImageList.Dispose();
+                    _nativeImageList = null;
+                }
+
+                _nativeImageList = new NativeImageList(_imageSize, flags);
             }
             finally
             {
                 ThemingScope.Deactivate(userCookie);
             }
 
-            if (Handle == IntPtr.Zero)
-            {
-                throw new InvalidOperationException(SR.ImageListCreateFailed);
-            }
-
             ComCtl32.ImageList.SetBkColor(this, ComCtl32.CLR.NONE);
 
             Debug.Assert(_originals != null, "Handle not yet created, yet original images are gone");
@@ -532,6 +531,8 @@ protected override void Dispose(bool disposing)
                     }
                 }
 
+                ImageStream?.Dispose();
+
                 DestroyHandle();
             }
 

src/System.Windows.Forms/src/System/Windows/Forms/ImageListStreamer.cs

@@ -54,12 +54,11 @@ private ImageListStreamer(SerializationInfo info, StreamingContext context)
 
                             try
                             {
-                                MemoryStream ms = new MemoryStream(Decompress(dat));
-
+                                using MemoryStream ms = new MemoryStream(Decompress(dat));
                                 lock (internalSyncObject)
                                 {
                                     ComCtl32.InitCommonControls();
-                                    nativeImageList = new ImageList.NativeImageList(ComCtl32.ImageList.Read(new Ole32.GPStream(ms)));
+                                    nativeImageList = new ImageList.NativeImageList(new Ole32.GPStream(ms));
                                 }
                             }
                             finally