Skip to content

Commit

Permalink
Revert "Explicit ImageList ownership management" (dotnet#4277)
Browse files Browse the repository at this point in the history 
Revert "Explicit ImageList ownership management. (dotnet#3601)"
This reverts commit 03db3fb.

Revert "Fix `ListView` no longer displays images (dotnet#4184)"
This reverts commit d0608e7.

We have observed an instability of tests under stress (and reasonably
high degree of concurrency) presumably caused by ImageList lifetime
handling (notably dotnet#3358).
The changes introduced in dotnet#3601 have helped with tests stability,
however resulted in a number of regressions, such as dotnet#4169 and dotnet#4275.

Restore the original implementation given it worked reasonably well for
the past so many years.

(cherry picked from commit b0ee5da)
  • Loading branch information
@RussKie
RussKie committed Dec 2, 2020
1 parent 200d740 commit 005055d
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 180 deletions.
7 changes: 0 additions & 7 deletions src/System.Windows.Forms/src/System/Windows/Forms/ImageList.ImageCollection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,6 @@ public Image this[int index]
bitmap.Dispose();
}
}

_owner.OnRecreateHandle(EventArgs.Empty);
}
}

Expand Down Expand Up @@ -381,7 +379,6 @@ private int Add(Original original, ImageInfo imageInfo)
if (!_isBatchAdd)
{
_owner.OnChangeHandle(EventArgs.Empty);
_owner.OnRecreateHandle(EventArgs.Empty);
}

return index;
Expand All @@ -402,7 +399,6 @@ public void AddRange(Image[] images)

_isBatchAdd = false;
_owner.OnChangeHandle(EventArgs.Empty);
_owner.OnRecreateHandle(EventArgs.Empty);
}

/// <summary>
Expand Down Expand Up @@ -453,7 +449,6 @@ public void Clear()
}

_owner.OnChangeHandle(EventArgs.Empty);
_owner.OnRecreateHandle(EventArgs.Empty);
}

[EditorBrowsable(EditorBrowsableState.Never)]
Expand Down Expand Up @@ -565,7 +560,6 @@ void IList.Remove(object value)
Remove(image);

_owner.OnChangeHandle(EventArgs.Empty);
_owner.OnRecreateHandle(EventArgs.Empty);
}
}

Expand All @@ -588,7 +582,6 @@ public void RemoveAt(int index)
_imageInfoCollection.RemoveAt(index);

_owner.OnChangeHandle(EventArgs.Empty);
_owner.OnRecreateHandle(EventArgs.Empty);
}
}

Expand Down
10 changes: 1 addition & 9 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 @@ -71,8 +71,7 @@ public void Dispose()
return;
}

var result = ComCtl32.ImageList.Destroy(Handle);
Debug.Assert(result.IsTrue());
ComCtl32.ImageList.Destroy(Handle);
Handle = IntPtr.Zero;
}
}
Expand All @@ -91,13 +90,6 @@ public void Dispose()
}
#endif

internal IntPtr TransferOwnership()
{
var handle = Handle;
Handle = IntPtr.Zero;
return handle;
}

internal NativeImageList Duplicate()
{
lock (s_syncLock)
Expand Down
13 changes: 1 addition & 12 deletions src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// 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.

Expand Down Expand Up @@ -140,17 +140,6 @@ public IntPtr Handle
}
}

internal IntPtr CreateUniqueHandle()
{
if (_nativeImageList is 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: 24 additions & 140 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 @@ -581,21 +581,11 @@ public bool CheckBoxes
{
if (CheckBoxes)
{ // we want custom checkboxes
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, _imageListState.Handle);
}
else
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero);
}
}

Expand Down Expand Up @@ -676,9 +666,7 @@ protected override CreateParams CreateParams
cp.Style |= (currentStyle & (int)(User32.WS.HSCROLL | User32.WS.VSCROLL));
}

// disabled until ownership management of list handles is fixed
// https://github.com/dotnet/winforms/issues/3531
//cp.Style |= (int)LVS.SHAREIMAGELISTS;
cp.Style |= (int)LVS.SHAREIMAGELISTS;

switch (alignStyle)
{
Expand Down Expand Up @@ -1000,13 +988,8 @@ public ImageList GroupImageList
return;
}

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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER,
value is null ? IntPtr.Zero : value.Handle);
}
}

Expand Down Expand Up @@ -1274,13 +1257,7 @@ public ImageList LargeImageList
return;
}

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());
}

User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, value is null ? IntPtr.Zero : value.Handle);
if (AutoArrange && !listViewState1[LISTVIEWSTATE1_disposingImageLists])
{
UpdateListViewItemsLocations();
Expand Down Expand Up @@ -1519,12 +1496,7 @@ public ImageList SmallImageList
return;
}

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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, value is null ? IntPtr.Zero : value.Handle);

if (View == View.SmallIcon)
{
Expand Down Expand Up @@ -1632,12 +1604,7 @@ public ImageList StateImageList

if (IsHandleCreated)
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, value is null ? IntPtr.Zero : value.Handle);
}
}
else
Expand All @@ -1652,12 +1619,7 @@ 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
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero);
}

_imageListState = value;
Expand All @@ -1675,13 +1637,8 @@ public ImageList StateImageList
}
else
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE,
(_imageListState is null || _imageListState.Images.Count == 0) ? IntPtr.Zero : _imageListState.Handle);
}

// Comctl should handle auto-arrange for us, but doesn't
Expand Down Expand Up @@ -3754,13 +3711,8 @@ private void GroupImageListRecreateHandle(object sender, EventArgs e)
return;
}

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());
}
IntPtr handle = (GroupImageList is null) ? IntPtr.Zero : GroupImageList.Handle;
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, handle);
}

public ListViewHitTestInfo HitTest(Point point)
Expand Down Expand Up @@ -4330,13 +4282,8 @@ private void LargeImageListRecreateHandle(object sender, EventArgs e)
return;
}

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());
}
IntPtr handle = (LargeImageList is null) ? IntPtr.Zero : LargeImageList.Handle;
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, handle);

ForceCheckBoxUpdate();
}
Expand Down Expand Up @@ -4679,34 +4626,6 @@ 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 @@ -4963,42 +4882,22 @@ protected void RealizeProperties()

if (_imageListLarge != null)
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, _imageListLarge.Handle);
}

if (_imageListSmall != null)
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, _imageListSmall.Handle);
}

if (_imageListState != null)
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, _imageListState.Handle);
}

if (_imageListGroup != null)
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.GROUPHEADER, _imageListGroup.Handle);
}
}

Expand Down Expand Up @@ -5525,13 +5424,8 @@ private void SmallImageListRecreateHandle(object sender, EventArgs e)
return;
}

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());
}
IntPtr handle = (SmallImageList is null) ? IntPtr.Zero : SmallImageList.Handle;
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.SMALL, handle);

ForceCheckBoxUpdate();
}
Expand Down Expand Up @@ -5563,13 +5457,8 @@ private void StateImageListRecreateHandle(object sender, EventArgs e)
return;
}

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());
}
IntPtr handle = (StateImageList is null) ? IntPtr.Zero : StateImageList.Handle;
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, handle);
}

/// <summary>
Expand Down Expand Up @@ -6334,12 +6223,7 @@ internal void RecreateHandleInternal()
// (Yes, it does exactly that even though our wrapper sets LVS_SHAREIMAGELISTS on the native listView.)
if (IsHandleCreated && StateImageList != null)
{
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());
}
User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.STATE, IntPtr.Zero);
}

RecreateHandle();
Expand Down
Loading

0 comments on commit 005055d

Please sign in to comment.