Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue 12495: Infinite loop in ToolStripItemCollection.AddRange #12513

Merged
merged 5 commits into from
Nov 27, 2024
Merged

Fix issue 12495: Infinite loop in ToolStripItemCollection.AddRange #12513

merged 5 commits into from
Nov 27, 2024

Conversation

Olina-Zhang
Copy link
Member

@Olina-Zhang Olina-Zhang commented Nov 20, 2024 •
edited
Loading

Fixes #12495 and #4454

Proposed changes

  • Early return for empty collection
  • Converts the ToolStripItemCollection into a temporary array (using ToArray()) to avoid modifying the original collection during iteration. This ensures that items can be safely added to the new collection without causing exceptions or unintended behavior, especially when items are removed from the original collection if they have a different owner control.
  • Add unit test for issue Infinite loop in ToolStripItemCollection.AddRange #12495 case

Regression?

  • Yes

Test methodology

  • Test fixing for GH issues: 12495 and 4454 manually
  • Unit test
Microsoft Reviewers: Open in CodeFlow

@Olina-Zhang Olina-Zhang requested a review from a team as a code owner November 20, 2024 06:01
@codecov Codecov
Copy link

codecov bot commented Nov 20, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.73237%. Comparing base (af49f16) to head (2f4c086).
Report is 41 commits behind head on main.

Additional details and impacted files 
@@                 Coverage Diff                 @@
##                main      #12513         +/-   ##
===================================================
- Coverage   75.73242%   75.73237%   -0.00006%     
===================================================
  Files           3153        3157          +4     
  Lines         635807      636123        +316     
  Branches       46975       47004         +29     
===================================================
+ Hits          481512      481751        +239     
- Misses        150870      150900         +30     
- Partials        3425        3472         +47
Flag Coverage Δ
Debug 75.73237% <100.00000%> (-0.00006%) ⬇️
integration 18.16407% <0.00000%> (-0.09047%) ⬇️
production 49.28844% <100.00000%> (-0.01708%) ⬇️
test 97.04462% <100.00000%> (-0.00532%) ⬇️
unit 46.52041% <100.00000%> (+0.25733%) ⬆️

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

@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Nov 20, 2024
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Nov 21, 2024
Tanya-Solyanik
Tanya-Solyanik reviewed Nov 21, 2024
View reviewed changes
Copy link
Member

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, added minor comments in additions to Leaf's one

...System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs Outdated Show resolved Hide resolved
...System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs Show resolved Hide resolved
@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Nov 21, 2024
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Nov 22, 2024
LeafShi1
LeafShi1 approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@LeafShi1 LeafShi1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Tanya-Solyanik
Copy link
Member

/backport to release/9.0

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/winforms/actions/runs/12041223814

@github-actions GitHub Actions
Copy link
Contributor

@Tanya-Solyanik backporting to release/9.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix issue 12495: Infinite loop in ToolStripItemCollection.AddRange
Using index info to reconstruct a base tree...
M	src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs
M	src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs
CONFLICT (content): Merge conflict in src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemCollectionTests.cs
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripItemCollection.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix issue 12495: Infinite loop in ToolStripItemCollection.AddRange
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@Tanya-Solyanik Tanya-Solyanik merged commit c56379f into dotnet:main Nov 27, 2024
8 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0 Preview1 milestone Nov 27, 2024
This was referenced Nov 28, 2024
Merged
Merged
@Olina-Zhang Olina-Zhang deleted the Fix_Issue12495 branch December 3, 2024 01:22
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kirsan31 kirsan31 kirsan31 left review comments

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik approved these changes

@LeafShi1 LeafShi1 LeafShi1 approved these changes

Assignees

@Olina-Zhang Olina-Zhang

Labels
None yet
Projects
None yet
Milestone
10.0 Preview1
Development

Successfully merging this pull request may close these issues.

Infinite loop in ToolStripItemCollection.AddRange
4 participants
@Olina-Zhang @Tanya-Solyanik @kirsan31 @LeafShi1