From 37f8fa494342f29226ed21a785a43b314b741aa1 Mon Sep 17 00:00:00 2001 From: "Olina Zhang (Beyondsoft Corporation)" Date: Wed, 20 Nov 2024 05:51:11 +0000 Subject: [PATCH 1/5] Fix issue 12495: Infinite loop in ToolStripItemCollection.AddRange --- .../ToolStrips/ToolStripItemCollection.cs | 15 +++++++++----- .../Forms/ToolStripItemCollectionTests.cs | 20 +++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs index a1a90cc1de4..e5398ce62eb 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs @@ -141,16 +141,21 @@ public void AddRange(ToolStripItemCollection toolStripItems) throw new NotSupportedException(SR.ToolStripItemCollectionIsReadOnly); } + // Return early if the collection is empty. + if (toolStripItems.Count == 0) + { + return; + } + // ToolStripDropDown will look for PropertyNames.Items to determine if it needs // to resize itself. using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items)) { - for (int i = 0; i < toolStripItems.Count; i++) + // Create a temporary list to avoid modifying the collection while iterating over it. + var itemsToAdd = toolStripItems.Cast().ToList(); + foreach (ToolStripItem item in itemsToAdd) { - // Items are removed from their origin when added to a different owner. - // Decrement the index to always add the items from index 0 which will preserve - // the original order and avoid a pesky ArgumentOutOfRangeException. - Add(toolStripItems[i--]); + Add(item); } } } diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs index ad73851a15b..b8d040e44ca 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs @@ -125,6 +125,26 @@ public void ToolStripItemCollection_AddRange_ToolStripItemCollection_Success() Assert.Equal("c", contextMenuStrip.Items[2].Text); } + [WinFormsFact] + public void ToolStripItemCollection_AddRange_ToolStripItemCollection_WithItems_Success() + { + using ToolStrip toolStrip = new(); + + // Create a ToolStripItemCollection with 2 items + ToolStripItemCollection itemCollection = new(toolStrip, new[] + { + new ToolStripButton("Button 1"), + new ToolStripButton("Button 2") + }); + + toolStrip.Items.AddRange(itemCollection); + + toolStrip.Items.Count.Should().Be(2); + + toolStrip.Items[0].Text.Should().Be("Button 1"); + toolStrip.Items[1].Text.Should().Be("Button 2"); + } + private ToolStripItem[] CreateToolStripItems(params (string Key, string Name)[] items) => items.Select(item => new ToolStripButton(item.Name) { Name = item.Key }).ToArray(); From 42b1a8d86c19248b7ff2a2e065c6cecfe1ac5249 Mon Sep 17 00:00:00 2001 From: "Olina Zhang (Beyondsoft Corporation)" Date: Wed, 20 Nov 2024 08:36:17 +0000 Subject: [PATCH 2/5] update code comment --- .../Forms/Controls/ToolStrips/ToolStripItemCollection.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs index e5398ce62eb..d62b49e1389 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs @@ -151,7 +151,9 @@ public void AddRange(ToolStripItemCollection toolStripItems) // to resize itself. using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items)) { - // Create a temporary list to avoid modifying the collection while iterating over it. + // Create a temporary list to avoid modifying the original collection during iteration. + // This ensures that the collection is not altered while it is being iterated over, + // preventing issues such as InvalidOperationException. var itemsToAdd = toolStripItems.Cast().ToList(); foreach (ToolStripItem item in itemsToAdd) { From 00c940a8d8741765f590c4007830922b40f8cfcd Mon Sep 17 00:00:00 2001 From: "Olina Zhang (Beyondsoft Corporation)" Date: Thu, 21 Nov 2024 07:43:51 +0000 Subject: [PATCH 3/5] Update test --- .../Windows/Forms/ToolStripItemCollectionTests.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs index b8d040e44ca..e56e9b3d764 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs @@ -126,19 +126,22 @@ public void ToolStripItemCollection_AddRange_ToolStripItemCollection_Success() } [WinFormsFact] - public void ToolStripItemCollection_AddRange_ToolStripItemCollection_WithItems_Success() + public void ToolStripItemCollection_AddRange_ToolStripItemCollection_SameOwner_Success() { using ToolStrip toolStrip = new(); // Create a ToolStripItemCollection with 2 items - ToolStripItemCollection itemCollection = new(toolStrip, new[] - { + ToolStripItemCollection itemCollection = new(toolStrip, + [ new ToolStripButton("Button 1"), new ToolStripButton("Button 2") - }); + ]); + + toolStrip.Items.Count.Should().Be(0); toolStrip.Items.AddRange(itemCollection); + itemCollection.Count.Should().Be(2); toolStrip.Items.Count.Should().Be(2); toolStrip.Items[0].Text.Should().Be("Button 1"); From 57cde8784e9feac79831538382a8143016652412 Mon Sep 17 00:00:00 2001 From: "Olina Zhang (Beyondsoft Corporation)" Date: Thu, 21 Nov 2024 07:57:34 +0000 Subject: [PATCH 4/5] Update source --- .../Forms/Controls/ToolStrips/ToolStripItemCollection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs index d62b49e1389..b6403fadf2d 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs @@ -154,7 +154,7 @@ public void AddRange(ToolStripItemCollection toolStripItems) // Create a temporary list to avoid modifying the original collection during iteration. // This ensures that the collection is not altered while it is being iterated over, // preventing issues such as InvalidOperationException. - var itemsToAdd = toolStripItems.Cast().ToList(); + var itemsToAdd = toolStripItems.InnerList.ToArray(); foreach (ToolStripItem item in itemsToAdd) { Add(item); From 2f4c08678433d1144272d3dc0d599e471738e4d7 Mon Sep 17 00:00:00 2001 From: "Olina Zhang (Beyondsoft Corporation)" Date: Fri, 22 Nov 2024 01:23:34 +0000 Subject: [PATCH 5/5] Update comment and test --- .../Forms/Controls/ToolStrips/ToolStripItemCollection.cs | 6 +++--- .../System/Windows/Forms/ToolStripItemCollectionTests.cs | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs index b6403fadf2d..371212c506f 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs @@ -151,9 +151,9 @@ public void AddRange(ToolStripItemCollection toolStripItems) // to resize itself. using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items)) { - // Create a temporary list to avoid modifying the original collection during iteration. - // This ensures that the collection is not altered while it is being iterated over, - // preventing issues such as InvalidOperationException. + // Create a temporary array to avoid modifying the original collection during iteration. + // Items will be removed from toolStripsItems collection when they are added to this collection + // if they had a different owner control. var itemsToAdd = toolStripItems.InnerList.ToArray(); foreach (ToolStripItem item in itemsToAdd) { diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs index e56e9b3d764..c587541231f 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs @@ -117,6 +117,8 @@ public void ToolStripItemCollection_AddRange_ToolStripItemCollection_Success() toolStripDropDownButton.DropDownItems.Add("b"); toolStripDropDownButton.DropDownItems.Add("c"); contextMenuStrip.Items.AddRange(toolStripDropDownButton.DropDownItems); + + Assert.Empty(toolStripDropDownButton.DropDownItems); Assert.Equal(3, contextMenuStrip.Items.Count); // Validate order. @@ -133,8 +135,8 @@ public void ToolStripItemCollection_AddRange_ToolStripItemCollection_SameOwner_S // Create a ToolStripItemCollection with 2 items ToolStripItemCollection itemCollection = new(toolStrip, [ - new ToolStripButton("Button 1"), - new ToolStripButton("Button 2") + new ToolStripButton("Button 1"), + new ToolStripButton("Button 2") ]); toolStrip.Items.Count.Should().Be(0);