Infinite loop in ToolStripItemCollection.AddRange

[ottoman52]

.NET version

.NET 8.0

Did it work in .NET Framework?

Yes

Did it work in any of the earlier releases of .NET Core or .NET 5+?

I dont know

Issue description

There is an infinite loop in ToolStripItemCollection.cs:

public void AddRange(ToolStripItemCollection toolStripItems)
{
	ArgumentNullException.ThrowIfNull(toolStripItems, "toolStripItems");
	if (IsReadOnly)
	{
		throw new NotSupportedException(System.SR.ToolStripItemCollectionIsReadOnly);
	}
	using (new LayoutTransaction(_owner, _owner, "Items"))
	{
		for (int i = 0; i < toolStripItems.Count; i++)
		{
			Add(toolStripItems[i--]);
		}
	}
}

for (int i = 0; i < toolStripItems.Count; i++) counts up from 0 but decrements 'i' down in Add(toolStripItems[i--]);

Any call to add items using this override will infinite loop with i going from 0 -> -1 -> 0 -> -1 etc...

Steps to reproduce

Any call to ToolStripItemCollection.AddRange(ToolStripItemCollection toolStripItems) with items.

[elachlan]

@Olina-Zhang can your team test this please?

[Olina-Zhang]

@Olina-Zhang can your team test this please?

We can repro this issue from .NET 6.0 to .NET 10 latest build with following testing code and sample application, not .NET 5.0 and .NET framework 4.8.1

public Form1()
{
    InitializeComponent();
    // Step 1: Add a ToolStrip to form designer, named mainToolStrip
    // Step 2: Create a second ToolStripItemCollection (you can also create individual ToolStripItems)
    ToolStripItemCollection itemCollection = new ToolStripItemCollection(mainToolStrip, new ToolStripItem[] {
    new ToolStripButton("Button 1"),
    new ToolStripButton("Button 2")});

    // Step 3: Call AddRange with the second collection
    mainToolStrip.Items.AddRange(itemCollection);  // This will cause an infinite loop
}

WinFormsApp26.zip

[elachlan]

@Olina-Zhang since the fix is pretty straight forward. Did your team want to take it on?

[Olina-Zhang]

@elachlan Tried to modify AddRange method with following, although this issue is resolved, but the issue #4454 is not fixed well. Need to investigate further...

 public void AddRange(ToolStripItemCollection toolStripItems)
 {
     ArgumentNullException.ThrowIfNull(toolStripItems);

     if (IsReadOnly)
     {
         throw new NotSupportedException(SR.ToolStripItemCollectionIsReadOnly);
     }

     using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items))
     {
         for (int i = 0; i < toolStripItems.Count; i++)
         {
             Add(toolStripItems[i]);
         }
     }
 }

[elachlan]

@Olina-Zhang use a reverse for loop.

public void AddRange(ToolStripItemCollection toolStripItems)
{
    ArgumentNullException.ThrowIfNull(toolStripItems);

    if (IsReadOnly)
    {
        throw new NotSupportedException(SR.ToolStripItemCollectionIsReadOnly);
    }

    // ToolStripDropDown will look for PropertyNames.Items to determine if it needs
    // to resize itself.
    using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items))
    {
        // Iterate in reverse to avoid index shifting issues.
        for (int i = toolStripItems.Count - 1; i >= 0; i--)
        {
            Add(toolStripItems[i]);
        }
    }
}

[Olina-Zhang]

Looks good, it can resolve both of issues.

[elachlan]

We should make sure we have test coverage of both issues.

[Olina-Zhang]

for (int i = toolStripItems.Count - 1; i >= 0; i--)
{
Add(toolStripItems[i]);
}

this will cause the order of items is reversed, see following:

can we do toolStripItems.InnerList.Reverse() firstly?

using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items))
    {
        // Iterate in reverse to avoid index shifting issues.
        toolStripItems.InnerList.Reverse();
        for (int i = toolStripItems.Count - 1; i >= 0; i--)
        {
            Add(toolStripItems[i]);
        }
    }

[merriemcgaw]

Great work @Olina-Zhang and @elachlan ! Feel free to submit the PR 😄

[Tanya-Solyanik]

Consider converting collection to an array (.ToArray()) instead of reversing and reading backwards.

[Tanya-Solyanik]

@merriemcgaw - are we servicing this hang?

[Zheng-Li01]

Verified the issues on latest .NET SDK 10.0.100-alpha.1.24573.1 + private dlls built from dotnet/winforms main branch, the #12495 and #4454 issues have been fixed as below screenshots.

12495

4454

[MelonWang1]

Verified the issues on latest .NET SDK 9.0.200-preview.0.24575.35 + private dlls built from dotnet/winforms release/9.0 branch, the #12495 and #4454 issues have been fixed. Test result is same as above.

[Nora-Zhou01]

Verified this issue in the 8.0.11 sdk + release/8.0 private dll,, the #12495 and #4454 issues have been fixed. Test result is same as above.

[Philip-Wang01]

Verified with .NET SDK 8.0.12 test pass build, the #12495 and #4454 issues have been fixed. Test result is same as above.

[John-Qiao]

Verified with .NET SDK 9.0.1 test pass build, the #12495 and #4454 issues have been fixed. Test result is same as above.