Do not crash when an invalid VirtualMode==true ListView is destroyed

[SimonZhao888]

Fixes #11663

In VirtualMode = true mode, the behavior of the ListView throwing a missing valid ListViewItem exception is correct, but this should not include the delete operation, which is completely unconcerned about data integrity and validity when the user wants to delete the ListView, and we can't create an accessibility object for a null object.

In the ReleaseUiaProvider method, we used accessor to get the ListViewItem which did not filter for null, it will resulte in the ListView throwing a NullReferenceException exception and not being able to be deleted when the user performs the delete operation, which did not make sense, so we add a new internal access method which will be used in ReleaseUiaProvider method, to make sure when the ListViewItem is null, we don't need execute release method.

Improve the property grid description of the VirtualListSize property to make users more aware of how VirtualListSize is used.

Proposed changes

• Improve property grid description.
• Add a new internal access method that allows to return a nullable ListViewItem object, this method is used in ReleaseUiaProvider(HWND handle).

Customer Impact

• Deleting an invalid ListView does not throw any exception in VirtualMode mode,

Regression?

• Yes

Risk

• Minimal

Screenshots

Before

After

Test methodology

• Manually

Test environment(s)

• 9.0.100-preview.4.24267.66

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

Attention: Patch coverage is 84.88372% with 13 lines in your changes missing coverage. Please review.

Project coverage is 74.84571%. Comparing base (9319f17) to head (bdd2fd3).
Report is 86 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11679         +/-   ##
===================================================
+ Coverage   74.51156%   74.84571%   +0.33414%     
===================================================
  Files           3040        3014         -26     
  Lines         629560      629336        -224     
  Branches       46839       46698        -141     
===================================================
+ Hits          469095      471031       +1936     
+ Misses        157096      154947       -2149     
+ Partials        3369        3358         -11     

Flag	Coverage Δ
Debug	74.84571% <84.88372%> (+0.33414%)	⬆️
integration	18.05083% <66.66667%> (+0.17225%)	⬆️
production	47.84806% <76.92308%> (+0.44572%)	⬆️
test	97.01179% <91.48936%> (+0.05964%)	⬆️
unit	44.87143% <76.92308%> (+0.35172%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[LeafShi1]

If the user only sets VirtualListSize but does not provide virtual item, the item.count here should not be greater than one.

[lonitra]

We should definitely fix surfacing a null reference exception to users, but it seems like we should still be calling ReleaseUiaProvider on items in a ListView during destroy, even if the ListView is in VirtualMode = true. The issue here looks more like RetrieveVirtualItem event was not handled on user end and we should probably surface a more helpful exception so users understand what went wrong. @Tanya-Solyanik could you correct me if I'm wrong here?

[Tanya-Solyanik]

The issue here looks more like RetrieveVirtualItem event was not handled on user end and we should probably surface a more helpful exception so users understand what went wrong

@lonitra - you are right, we can say that this is an invalid scenario, but I don't think we should surface a message about populating virtual items on control disposal. We already have exception - https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.listview.virtuallistsize?view=windowsdesktop-8.0#exceptions about this. We could perhaps improve property grid description of the VirtualListSize property by adding "If VirtualMode is set to true, and the VirtualListSize property is greater than 0, you must handle the RetrieveVirtualItem event providing valid items."

I would just make sure that if items were created, their providers should be released. @SimonZhao888 - make sure that you don't create new items on the disposal stack.

[SimonZhao888]

I would just make sure that if items were created, their providers should be released. @SimonZhao888 - make sure that you don't create new items on the disposal stack.

Yes, if the items were created, their providers will be released. I used this demo for testing. https://learn.microsoft.com/zh-cn/dotnet/api/system.windows.forms.listview.virtualmode?view=windowsdesktop-8.0#--

make sure that you don't create new items on the disposal stack.

I didn't find any codes of create new items on the disposal stack.

And this issue does not repo on .NET Framework.

[Tanya-Solyanik]

Yes, if the items were created, their providers will be released. I used this demo for testing. https://learn.microsoft.com/zh-cn/dotnet/api/system.windows.forms.listview.virtualmode?view=windowsdesktop-8.0#--

Thank you! we also have samples with virtual items in our winforms controls app integration test in the repo.
But now that you reverted the virtual mode check, you will have the crash again, is it possible to avoid it?

[lonitra]

we can say that this is an invalid scenario, but I don't think we should surface a message about populating virtual items on control disposal. We already have exception - https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.listview.virtuallistsize?view=windowsdesktop-8.0#exceptions about this. We could perhaps improve property grid description of the VirtualListSize property by adding "If VirtualMode is set to true, and the VirtualListSize property is greater than 0, you must handle the RetrieveVirtualItem event providing valid items."

@Tanya-Solyanik - I think that's a good idea to improve the description, but this doesn't resolve issue that we are surfacing a NRE to users. Based on the callstack in the issue, it seems like we are getting NRE here because we had assumed that it would be nonnull. Should we perhaps throw exception there and add a try/catch here? Catching to avoid exception during destroy.

[Tanya-Solyanik]

@lonitra - yes, this exception is a regression and will block migration, we must fix it. I commented on the same at the same time as you did.
We can't call item indexer (

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ListView/ListView.ListViewNativeItemCollection.cs

Lines 35 to 48 in 1288baa

	public ListViewItem this[int displayIndex]
	{
	get
	{
	_owner.ApplyUpdateCachedItems();
	if (_owner.VirtualMode)
	{
	// if we are showing virtual items, we need to get the item from the user
	RetrieveVirtualItemEventArgs rVI = new(displayIndex);
	_owner.OnRetrieveVirtualItem(rVI);
	rVI.Item!.SetItemIndex(_owner, displayIndex);
	return rVI.Item;
	}

) in virtual mode from the dispose stack because it might create items that had not been created previously. And yes, the null forgiving operator is unwarranted in that indexer - we received data from user code with no guarantees of it being properly implemented. Indexer should throw InvalidOperationException, as per docs.
Instead of proactively accessing indexer, we need to cache information about items being initialized in virtual mode( might be already available, I hadn't checked) and access only items that have been successfully created.

[Tanya-Solyanik]

Calling listView.AccessibilityObject property will associate accessible object with the control and you will avoid calling private methods.

[Tanya-Solyanik]

Why do you have to special-case the null? Calling Control.ReleaseUiaProviders method better simulates the WM_DESTROY scenario

[SimonZhao888]

In virtual mode, whether we create the control handle first and then add the ListViewItem with null value, or we set up the ListView that contains the null value and then get the control handle, they will both throw exceptions.

1. The first one will throws the SR.ListViewVirtualListSizeDescr.
2. The second one will throws An unhandled exception was encountered during a user callback.
   

   winforms/src/System.Windows.Forms/src/System/Windows/Forms/NativeWindow.cs

   Lines 437 to 449 in bacb9f8

   	createResult = PInvoke.CreateWindowEx(
   	(WINDOW_EX_STYLE)cp.ExStyle,
   	windowClass._windowClassName,
   	cp.Caption,
   	(WINDOW_STYLE)cp.Style,
   	cp.X,
   	cp.Y,
   	cp.Width,
   	cp.Height,
   	(HWND)cp.Parent,
   	HMENU.Null,
   	modHandle,
   	cp.Param);

In either case, these exception will cause the test program to end prematurely and not be able to calling Control.ReleaseUiaProviders method, neither of which meets the purpose of our test, which is to ensure that a null ListViewItem can be released normally in virtual mode, so I made the null as special-case.

[Tanya-Solyanik]

I see, Control.ReleaseUiaProvider takes a Handle as an argument. But you don't have to create a Handle when invoking this method. You can pass in a zero or InternalHandle, because we are only testing how the item providers are released, and items are not windows.

In general Control.HWND property is a dangerous one and should be used only when you must create a window, most often we use InternalHandle instead.

[Tanya-Solyanik]

@Epica3055 - when you call InternalHandle, you don't have to special-case the null item case. Lines 5244-5247 will work for null item

[Tanya-Solyanik]

@Epica3055 , @SimonZhao888 Sorry about the confusion. I was suggesting to
1 avoid the private reflection because we have public property that can be used instead (AccessibilityObject)
2. invoke ReleaseUiaProviders in the same way as it is invoked in applications, by calling Control.ReleaseUiaProveders method.
3. validate that control handle had not been created.

   [WinFormsTheory]
   [MemberData(nameof(GetListViewItemTheoryData))]
   // Regression test for https://github.com/dotnet/winforms/issues/11663.
   public void ListView_VirtualMode_ReleaseUiaProvider_Success(ListViewItem listItem)
   {
       using ListView listView = new()
       {
           VirtualMode = true,
           VirtualListSize = 1
       };

       listView.RetrieveVirtualItem += (s, e) =>
       {
           e.Item = e.ItemIndex switch
           {
               0 => listItem,
               _ => throw new NotImplementedException()
           };
       };

       listView.AccessibilityObject.Should().NotBeNull();

       Action action = () => listView.ReleaseUiaProvider(listView.InternalHandle);
       action.Should().NotThrow();
       listView.IsAccessibilityObjectCreated.Should().BeFalse();
       listView.IsHandleCreated.Should().BeFalse();
   }

[Epica3055]

looks good .

[lonitra]

Just a comment about catching specific exceptions, otherwise LGTM 👍