Skip to content

Commit

Permalink
Explicit ImageList ownership management. (#3601)
Browse files Browse the repository at this point in the history
  • Loading branch information
@weltkante
weltkante authored Jul 22, 2020
1 parent c7040f2 commit 03db3fb
Show file tree
Hide file tree
Showing 5 changed files with 175 additions and 37 deletions.
17 changes: 12 additions & 5 deletions src/System.Windows.Forms/src/System/Windows/Forms/ImageList.NativeImageList.cs
View file
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
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
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

0 comments on commit 03db3fb

Please sign in to comment.