Skip to content

Workaround for bad ImageList ownership management #3601

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.

Sign up for GitHub

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

Merged
JeremyKuhne merged 1 commit into from
Jul 22, 2020
Merged

Workaround for bad ImageList ownership management #3601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/System.Windows.Forms/src/System/Windows/Forms/ImageList.NativeImageList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,20 @@ private void Init(IntPtr himl)

public void Dispose()
{
#if DEBUG
GC.SuppressFinalize(this);
#endif
lock (s_syncLock)
{
if (Handle == IntPtr.Zero)
{
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
Expand All @@ -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)
Expand Down
11 changes: 11 additions & 0 deletions src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,17 @@ public IntPtr Handle
}
}

internal IntPtr CreateUniqueHandle()
{
if (_nativeImageList == null)
{
CreateHandle();
}

using var iml = _nativeImageList.Duplicate();
return iml.TransferOwnership();
}

/// <summary>
/// Whether or not the underlying Win32 handle has been created.
/// </summary>
Expand Down
164 changes: 140 additions & 24 deletions src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}

Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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());
}
}
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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());
}
}
}

Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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());
}
}

/// <summary>
Expand Down Expand Up @@ -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();
Expand Down
Loading