From 03db3fbfcc6884356f70141f882433638b23bb49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20K=C3=A4s?= Date: Wed, 22 Jul 2020 07:41:18 +0200 Subject: [PATCH] Explicit ImageList ownership management. (#3601) --- .../Forms/ImageList.NativeImageList.cs | 17 +- .../src/System/Windows/Forms/ImageList.cs | 11 ++ .../src/System/Windows/Forms/ListView.cs | 164 +++++++++++++++--- .../src/System/Windows/Forms/TreeView.cs | 16 +- .../System/Windows/Forms/ListViewTests.cs | 4 +- 5 files changed, 175 insertions(+), 37 deletions(-) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.NativeImageList.cs b/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.NativeImageList.cs index 800304c3e41..5ae55105cf8 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.NativeImageList.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.NativeImageList.cs @@ -61,6 +61,9 @@ private void Init(IntPtr himl) public void Dispose() { +#if DEBUG + GC.SuppressFinalize(this); +#endif lock (s_syncLock) { if (Handle == IntPtr.Zero) @@ -68,13 +71,10 @@ public void Dispose() return; } - ComCtl32.ImageList.Destroy(Handle); + var result = ComCtl32.ImageList.Destroy(Handle); + Debug.Assert(result.IsTrue()); Handle = IntPtr.Zero; } - -#if DEBUG - GC.SuppressFinalize(this); -#endif } #if DEBUG @@ -91,6 +91,13 @@ public void Dispose() } #endif + internal IntPtr TransferOwnership() + { + var handle = Handle; + Handle = IntPtr.Zero; + return handle; + } + internal NativeImageList Duplicate() { lock (s_syncLock) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs b/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs index 5670a99d6a8..9e44835c61b 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs @@ -142,6 +142,17 @@ public IntPtr Handle } } + internal IntPtr CreateUniqueHandle() + { + if (_nativeImageList == null) + { + CreateHandle(); + } + + using var iml = _nativeImageList.Duplicate(); + return iml.TransferOwnership(); + } + /// /// Whether or not the underlying Win32 handle has been created. /// diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs b/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs index 2d8353f0741..a9add845ed2 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs @@ -579,11 +579,21 @@ public bool CheckBoxes { if (CheckBoxes) { // we want custom checkboxes - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, _imageListState.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, _imageListState.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } else { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } } @@ -664,7 +674,9 @@ protected override CreateParams CreateParams cp.Style |= (currentStyle & (int)(User32.WS.HSCROLL | User32.WS.VSCROLL)); } - cp.Style |= (int)LVS.SHAREIMAGELISTS; + // disabled until ownership management of list handles is fixed + // https://github.com/dotnet/winforms/issues/3531 + //cp.Style |= (int)LVS.SHAREIMAGELISTS; switch (alignStyle) { @@ -966,8 +978,13 @@ public ImageList GroupImageList return; } - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, - value is null ? IntPtr.Zero : value.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, + value is null ? IntPtr.Zero : value.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } } @@ -1235,7 +1252,13 @@ public ImageList LargeImageList return; } - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, value is null ? IntPtr.Zero : value.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, value is null ? IntPtr.Zero : value.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } + if (AutoArrange && !listViewState1[LISTVIEWSTATE1_disposingImageLists]) { UpdateListViewItemsLocations(); @@ -1474,7 +1497,12 @@ public ImageList SmallImageList return; } - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, value is null ? IntPtr.Zero : value.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, value is null ? IntPtr.Zero : value.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } if (View == View.SmallIcon) { @@ -1582,7 +1610,12 @@ public ImageList StateImageList if (IsHandleCreated) { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, value is null ? IntPtr.Zero : value.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, value is null ? IntPtr.Zero : value.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } } else @@ -1597,7 +1630,12 @@ public ImageList StateImageList // (Yes, it does exactly that even though our wrapper sets LVS_SHAREIMAGELISTS on the native listView.) // So we make the native listView forget about its StateImageList just before we recreate the handle. // Likely related to https://devblogs.microsoft.com/oldnewthing/20171128-00/?p=97475 - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } _imageListState = value; @@ -1615,8 +1653,13 @@ public ImageList StateImageList } else { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, - (_imageListState is null || _imageListState.Images.Count == 0) ? IntPtr.Zero : _imageListState.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, + (_imageListState is null || _imageListState.Images.Count == 0) ? IntPtr.Zero : _imageListState.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } // Comctl should handle auto-arrange for us, but doesn't @@ -3672,8 +3715,13 @@ private void GroupImageListRecreateHandle(object sender, EventArgs e) return; } - IntPtr handle = (GroupImageList is null) ? IntPtr.Zero : GroupImageList.Handle; - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, handle); + IntPtr handle = (GroupImageList is null) ? IntPtr.Zero : GroupImageList.CreateUniqueHandle(); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, handle); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } public ListViewHitTestInfo HitTest(Point point) @@ -4243,8 +4291,13 @@ private void LargeImageListRecreateHandle(object sender, EventArgs e) return; } - IntPtr handle = (LargeImageList is null) ? IntPtr.Zero : LargeImageList.Handle; - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, handle); + IntPtr handle = (LargeImageList is null) ? IntPtr.Zero : LargeImageList.CreateUniqueHandle(); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, handle); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } ForceCheckBoxUpdate(); } @@ -4587,6 +4640,34 @@ protected override void OnHandleCreated(EventArgs e) protected override void OnHandleDestroyed(EventArgs e) { + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } + + previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } + + previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } + + previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } + // don't save the list view items state when in virtual mode : it is the responsability of the // user to cache the list view items in virtual mode if (!Disposing && !VirtualMode) @@ -4822,22 +4903,42 @@ protected void RealizeProperties() if (_imageListLarge != null) { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, _imageListLarge.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, _imageListLarge.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } if (_imageListSmall != null) { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, _imageListSmall.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, _imageListSmall.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } if (_imageListState != null) { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, _imageListState.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, _imageListState.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } if (_imageListGroup != null) { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, _imageListGroup.Handle); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, _imageListGroup.CreateUniqueHandle()); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } } @@ -5364,8 +5465,13 @@ private void SmallImageListRecreateHandle(object sender, EventArgs e) return; } - IntPtr handle = (SmallImageList is null) ? IntPtr.Zero : SmallImageList.Handle; - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, handle); + IntPtr handle = (SmallImageList is null) ? IntPtr.Zero : SmallImageList.CreateUniqueHandle(); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, handle); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } ForceCheckBoxUpdate(); } @@ -5396,8 +5502,13 @@ private void StateImageListRecreateHandle(object sender, EventArgs e) return; } - IntPtr handle = (StateImageList is null) ? IntPtr.Zero : StateImageList.Handle; - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, handle); + IntPtr handle = (StateImageList is null) ? IntPtr.Zero : StateImageList.CreateUniqueHandle(); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, handle); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } /// @@ -6164,7 +6275,12 @@ internal void RecreateHandleInternal() // (Yes, it does exactly that even though our wrapper sets LVS_SHAREIMAGELISTS on the native listView.) if (IsHandleCreated && StateImageList != null) { - User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero); + var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero); + if (previousHandle != IntPtr.Zero) + { + var result = ComCtl32.ImageList.Destroy(previousHandle); + Debug.Assert(result.IsTrue()); + } } RecreateHandle(); diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs b/src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs index ae8b7aad024..3c2b177ac3a 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs @@ -662,7 +662,7 @@ public ImageList ImageList value == null ? IntPtr.Zero : value.Handle); if (StateImageList != null && StateImageList.Images.Count > 0 && internalStateImageList != null) { - SetStateImageList(internalStateImageList.Handle); + SetStateImageList(internalStateImageList.CreateUniqueHandle()); } } UpdateCheckedState(root, true); @@ -1819,7 +1819,7 @@ private void StateImageListRecreateHandle(object sender, EventArgs e) IntPtr handle = IntPtr.Zero; if (internalStateImageList != null) { - handle = internalStateImageList.Handle; + handle = internalStateImageList.CreateUniqueHandle(); } SetStateImageList(handle); } @@ -1859,7 +1859,7 @@ private void StateImageListChangedHandle(object sender, EventArgs e) internalStateImageList.ImageSize = (Size)ScaledStateImageSize; } - SetStateImageList(internalStateImageList.Handle); + SetStateImageList(internalStateImageList.CreateUniqueHandle()); } } else //stateImageList == null || stateImageList.Images.Count = 0; @@ -2050,7 +2050,7 @@ private void UpdateNativeStateImageList() images[i] = stateImageList.Images[i - 1]; } newImageList.Images.AddRange(images); - User32.SendMessageW(this, (User32.WM)TVM.SETIMAGELIST, (IntPtr)TVSIL.STATE, newImageList.Handle); + User32.SendMessageW(this, (User32.WM)TVM.SETIMAGELIST, (IntPtr)TVSIL.STATE, newImageList.CreateUniqueHandle()); if (internalStateImageList != null) { @@ -2067,7 +2067,8 @@ private void SetStateImageList(IntPtr handle) IntPtr handleOld = User32.SendMessageW(this, (User32.WM)TVM.SETIMAGELIST, (IntPtr)TVSIL.STATE, handle); if ((handleOld != IntPtr.Zero) && (handleOld != handle)) { - ComCtl32.ImageList.Destroy(new HandleRef(this, handleOld)); + var result = ComCtl32.ImageList.Destroy(new HandleRef(this, handleOld)); + Debug.Assert(result.IsTrue()); } } @@ -2078,7 +2079,8 @@ private void DestroyNativeStateImageList(bool reset) IntPtr handle = User32.SendMessageW(this, (User32.WM)TVM.GETIMAGELIST, (IntPtr)TVSIL.STATE); if (handle != IntPtr.Zero) { - ComCtl32.ImageList.Destroy(new HandleRef(this, handle)); + var result = ComCtl32.ImageList.Destroy(new HandleRef(this, handle)); + Debug.Assert(result.IsTrue()); if (reset) { User32.SendMessageW(this, (User32.WM)TVM.SETIMAGELIST, (IntPtr)TVSIL.STATE); @@ -2619,7 +2621,7 @@ internal override void UpdateStylesCore() // user's images. if (internalStateImageList != null) { - SetStateImageList(internalStateImageList.Handle); + SetStateImageList(internalStateImageList.CreateUniqueHandle()); } } } diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ListViewTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ListViewTests.cs index 3d655d373e9..c3330b37db4 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ListViewTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ListViewTests.cs @@ -170,7 +170,9 @@ public void ListView_CreateParams_GetDefault_ReturnsExpected() Assert.Equal(97, createParams.Height); Assert.Equal(IntPtr.Zero, createParams.Parent); Assert.Null(createParams.Param); - Assert.Equal(0x56010148, createParams.Style); + // LVS.SHAREIMAGELISTS is temporarily removed from style until ownership management is fixed + // https://github.com/dotnet/winforms/issues/3531 + Assert.Equal(0x56010108, createParams.Style); Assert.Equal(121, createParams.Width); Assert.Equal(0, createParams.X); Assert.Equal(0, createParams.Y);