Guard related currency manager against re-entrant item changes

src/System.Windows.Forms.Legacy/System.Windows.Forms.Legacy.Tests/BindingContextTests.cs

@@ -0,0 +1,100 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.ComponentModel;
+using System.Windows.Forms;
+
+namespace System.Windows.Forms.Legacy.Tests;
+
+public class BindingContextTests
+{
+    [Fact]
+    public void BindingContext_UpdateBinding_RelatedManagerCurrentItemChangedDuringPullData_DoesNotReenter()
+    {
+        BindingContext context = [];
+        BindingList<ReentrantParentDataSource> parentList = null!;
+        ReentrantChildDataSource child = new(() => parentList.ResetItem(0));
+        ReentrantParentDataSource parent = new(child);
+        parentList = [parent];
+
+        ReentrantBindableComponent component = new() { BindingContext = context };
+        Binding binding = component.DataBindings.Add(
+            nameof(ReentrantBindableComponent.Value),
+            parentList,
+            $"{nameof(ReentrantParentDataSource.Children)}.{nameof(ReentrantChildDataSource.Value)}");
+
+        CurrencyManager relatedManager = Assert.IsAssignableFrom<CurrencyManager>(context[parentList, nameof(ReentrantParentDataSource.Children)]);
+        Assert.Same(relatedManager, binding.BindingManagerBase);
+
+        Exception? dataError = null;
+        relatedManager.DataError += (sender, e) => dataError = e.Exception;
+
+        component.Value = "updated";
+        parentList.ResetItem(0);
+
+        Assert.Null(dataError);
+        Assert.Equal(1, child.ValueSetCount);
+        Assert.Equal("updated", child.Value);
+    }
+
+    private class ReentrantParentDataSource
+    {
+        public ReentrantParentDataSource(ReentrantChildDataSource child)
+        {
+            Children = [child];
+        }
+
+        public BindingList<ReentrantChildDataSource> Children { get; }
+    }
+
+    private class ReentrantChildDataSource
+    {
+        private readonly Action _valueSetCallback;
+        private string _value = "initial";
+
+        public ReentrantChildDataSource(Action valueSetCallback)
+        {
+            _valueSetCallback = valueSetCallback;
+        }
+
+        public int ValueSetCount { get; private set; }
+
+        public string Value
+        {
+            get => _value;
+            set
+            {
+                ValueSetCount++;
+                if (ValueSetCount > 1)
+                {
+                    throw new InvalidOperationException("Related manager pulled data re-entrantly.");
+                }
+
+                _value = value;
+                _valueSetCallback();
+            }
+        }
+    }
+
+    private class ReentrantBindableComponent : BindableComponent
+    {
+        private string? _value;
+
+        public string? Value
+        {
+            get => _value;
+            set
+            {
+                if (_value == value)
+                {
+                    return;
+                }
+
+                _value = value;
+                ValueChanged?.Invoke(this, EventArgs.Empty);
+            }
+        }
+
+        public event EventHandler? ValueChanged;
+    }
+}

src/System.Windows.Forms/System/Windows/Forms/DataBinding/RelatedCurrencyManager.cs

@@ -14,6 +14,7 @@ internal class RelatedCurrencyManager : CurrencyManager
 {
     private BindingManagerBase _parentManager;
     private PropertyDescriptor _fieldInfo;
+    private bool _handlingCurrentItemChanged;
     private static List<BindingManagerBase> IgnoreItemChangedTable { get; } = [];
 
     internal RelatedCurrencyManager(BindingManagerBase parentManager, string dataField)
@@ -142,69 +143,82 @@ private void ParentManager_CurrentItemChanged(object? sender, EventArgs e)
             return;
         }
 
-        int oldlistposition = listposition;
-
-        // we only pull the data from the controls into the backEnd. we do not care about keeping the lastGoodKnownRow
-        // when we are about to change the entire list in this currencymanager.
-        try
-        {
-            PullData();
-        }
-        catch (Exception ex)
+        if (_handlingCurrentItemChanged)
         {
-            OnDataError(ex);
+            return;
         }
 
-        if (_parentManager is CurrencyManager currencyManager)
+        _handlingCurrentItemChanged = true;
+        try
         {
-            if (currencyManager.Count > 0)
+            int oldlistposition = listposition;
+
+            // we only pull the data from the controls into the backEnd. we do not care about keeping the lastGoodKnownRow
+            // when we are about to change the entire list in this currencymanager.
+            try
             {
-                // Parent list has a current row, so get the related list from the relevant property on that row.
-                SetDataSource(_fieldInfo.GetValue(currencyManager.Current));
-                listposition = (Count > 0 ? 0 : -1);
+                PullData();
             }
-            else
+            catch (Exception ex)
             {
-                // APPCOMPAT: bring back the Everett behavior where the currency manager adds an item and
-                // then it cancels the addition.
-                //
-                // really, really hocky.
-                // will throw if the list in the curManager is not IBindingList
-                // and this will fail if the IBindingList does not have list change notification. read on....
-                // when a new item will get added to an empty parent table,
-                // the table will fire OnCurrentChanged and this method will get executed again
-                // allowing us to set the data source to an object with the right properties (so we can show
-                // metadata at design time).
-                // we then call CancelCurrentEdit to remove the dummy row, but making sure to ignore any
-                // OnCurrentItemChanged that results from this action (to avoid infinite recursion)
-                currencyManager.AddNew();
-                try
+                OnDataError(ex);
+            }
+
+            if (_parentManager is CurrencyManager currencyManager)
+            {
+                if (currencyManager.Count > 0)
                 {
-                    IgnoreItemChangedTable.Add(currencyManager);
-                    currencyManager.CancelCurrentEdit();
+                    // Parent list has a current row, so get the related list from the relevant property on that row.
+                    SetDataSource(_fieldInfo.GetValue(currencyManager.Current));
+                    listposition = (Count > 0 ? 0 : -1);
                 }
-                finally
+                else
                 {
-                    if (IgnoreItemChangedTable.Contains(currencyManager))
+                    // APPCOMPAT: bring back the Everett behavior where the currency manager adds an item and
+                    // then it cancels the addition.
+                    //
+                    // really, really hocky.
+                    // will throw if the list in the curManager is not IBindingList
+                    // and this will fail if the IBindingList does not have list change notification. read on....
+                    // when a new item will get added to an empty parent table,
+                    // the table will fire OnCurrentChanged and this method will get executed again
+                    // allowing us to set the data source to an object with the right properties (so we can show
+                    // metadata at design time).
+                    // we then call CancelCurrentEdit to remove the dummy row, but making sure to ignore any
+                    // OnCurrentItemChanged that results from this action (to avoid infinite recursion)
+                    currencyManager.AddNew();
+                    try
+                    {
+                        IgnoreItemChangedTable.Add(currencyManager);
+                        currencyManager.CancelCurrentEdit();
+                    }
+                    finally
                     {
-                        IgnoreItemChangedTable.Remove(currencyManager);
+                        if (IgnoreItemChangedTable.Contains(currencyManager))
+                        {
+                            IgnoreItemChangedTable.Remove(currencyManager);
+                        }
                     }
                 }
             }
-        }
-        else
-        {
-            // Case where the parent is not a list, but a single object
-            SetDataSource(_fieldInfo.GetValue(_parentManager.Current));
-            listposition = (Count > 0 ? 0 : -1);
-        }
+            else
+            {
+                // Case where the parent is not a list, but a single object
+                SetDataSource(_fieldInfo.GetValue(_parentManager.Current));
+                listposition = (Count > 0 ? 0 : -1);
+            }
+
+            if (oldlistposition != listposition)
+            {
+                OnPositionChanged(EventArgs.Empty);
+            }
 
-        if (oldlistposition != listposition)
+            OnCurrentChanged(EventArgs.Empty);
+            OnCurrentItemChanged(EventArgs.Empty);
+        }
+        finally
         {
-            OnPositionChanged(EventArgs.Empty);
+            _handlingCurrentItemChanged = false;
         }
-
-        OnCurrentChanged(EventArgs.Empty);
-        OnCurrentItemChanged(EventArgs.Empty);
     }
 }

src/test/unit/System.Windows.Forms/System/Windows/Forms/BindingContextTests.cs

@@ -958,6 +958,35 @@ public void BindingContext_UpdateBinding_NullBindingContext_Success()
         Assert.Null(binding.BindingManagerBase);
     }
 
+    [Fact]
+    public void BindingContext_UpdateBinding_RelatedManagerCurrentItemChangedDuringPullData_DoesNotReenter()
+    {
+        BindingContext context = [];
+        BindingList<ReentrantParentDataSource> parentList = null;
+        ReentrantChildDataSource child = new(() => parentList.ResetItem(0));
+        ReentrantParentDataSource parent = new(child);
+        parentList = [parent];
+
+        ReentrantBindableComponent component = new() { BindingContext = context };
+        Binding binding = component.DataBindings.Add(
+            nameof(ReentrantBindableComponent.Value),
+            parentList,
+            $"{nameof(ReentrantParentDataSource.Children)}.{nameof(ReentrantChildDataSource.Value)}");
+
+        CurrencyManager relatedManager = Assert.IsAssignableFrom<CurrencyManager>(context[parentList, nameof(ReentrantParentDataSource.Children)]);
+        Assert.Same(relatedManager, binding.BindingManagerBase);
+
+        Exception dataError = null;
+        relatedManager.DataError += (sender, e) => dataError = e.Exception;
+
+        component.Value = "updated";
+        parentList.ResetItem(0);
+
+        Assert.Null(dataError);
+        Assert.Equal(1, child.ValueSetCount);
+        Assert.Equal("updated", child.Value);
+    }
+
     [Fact]
     public void BindingContext_InvokeCircularWithoutComponent_ThrowsArgumentException()
     {
@@ -1042,6 +1071,67 @@ private class ObjectDataSource
         public object Property { get; set; }
     }
 
+    private class ReentrantParentDataSource
+    {
+        public ReentrantParentDataSource(ReentrantChildDataSource child)
+        {
+            Children = [child];
+        }
+
+        public BindingList<ReentrantChildDataSource> Children { get; }
+    }
+
+    private class ReentrantChildDataSource
+    {
+        readonly Action valueSetCallback;
+        string value = "initial";
+
+        public ReentrantChildDataSource(Action valueSetCallback)
+        {
+            this.valueSetCallback = valueSetCallback;
+        }
+
+        public int ValueSetCount { get; private set; }
+
+        public string Value
+        {
+            get => value;
+            set
+            {
+                ValueSetCount++;
+                if (ValueSetCount > 1)
+                {
+                    throw new InvalidOperationException("Related manager pulled data re-entrantly.");
+                }
+
+                this.value = value;
+                valueSetCallback();
+            }
+        }
+    }
+
+    private class ReentrantBindableComponent : BindableComponent
+    {
+        string value;
+
+        public string Value
+        {
+            get => value;
+            set
+            {
+                if (this.value == value)
+                {
+                    return;
+                }
+
+                this.value = value;
+                ValueChanged?.Invoke(this, EventArgs.Empty);
+            }
+        }
+
+        public event EventHandler ValueChanged;
+    }
+
     private class SubBindingContext : BindingContext
     {
         public new void Add(object dataSource, BindingManagerBase listManager) => base.Add(dataSource, listManager);