Guard related currency manager against re-entrant item changes

[FahmiFuzi]

Why

CargoWise Group 7 net10 failures exposed a stack overflow path in related WinForms binding managers.

The recursive path is:

1. RelatedCurrencyManager.ParentManager_CurrentItemChanged responds to the parent manager's CurrentItemChanged event.
2. It calls PullData() to push dirty bound-control values back into the current child object.
3. The bound child setter/list-change path raises another parent item change while the first handler is still running.
4. The same RelatedCurrencyManager re-enters ParentManager_CurrentItemChanged, pulls the same dirty binding again, and repeats until stack overflow.

Why .NET 8 Works But .NET 10 Fails

The root cause is not a new event subscription in RelatedCurrencyManager itself — both net8 and net10 wire related managers to ParentManager_CurrentItemChanged. The difference is in how BindingContext stores managers, which let CargoWise intercept and rewire that subscription in net8 but silently loses the interception in net10.

Net 8: Hashtable storage — interception works

BindingContext._listManagers is a plain Hashtable in the net8 fork (BindingContext.cs line 18, net8.0_legacy):

private readonly Hashtable _listManagers;

CargoWise's ZBindingContext constructor uses reflection to replace this field with a custom BindingContextHashtable subclass (ZBindingContext.cs lines 19–22):

var listManagersField = typeof(BindingContext).GetField("listManagers", ...)
                     ?? typeof(BindingContext).GetField("_listManagers", ...);
listManagersField.SetValue(this, new BindingContextHashtable(this));

When EnsureListManager creates a new RelatedCurrencyManager and caches it via _listManagers.Add(key, new WeakReference(bindingManagerBase)) (BindingContext.cs line 331, net8), the swapped-in hashtable intercepts that Add call. The BindingContextHashtable.Add override then:

1. Unwraps the WeakReference to get the new RelatedCurrencyManager
2. Reflects the manager's private parentManager field
3. Removes the default parentManager.CurrentItemChanged -= ParentManager_CurrentItemChanged subscription
4. Installs a narrower CargoWise CurrentChanged handler via ParentCurrentChangedHandler (ZBindingContext.cs ~line 220)

CurrentChanged fires only when the parent's current object actually changes, not on every item/property notification. The CargoWise handler also tracks lastCurrent and skips redundant updates, so dirty-binding pushes from PullData() cannot re-trigger it.

Net 10: Dictionary storage — interception silently fails

In the net10/master fork _listManagers is strongly typed (BindingContext.cs line 16, master):

private readonly Dictionary<HashKey, WeakReference> _listManagers;

The reflection assignment now fails silently: a Hashtable subclass cannot be set into a Dictionary<HashKey, WeakReference> field. ZBindingContext has a #if !NETFRAMEWORK fallback that tries to rewire in OnCollectionChanged, but EnsureListManager does not call OnCollectionChanged when it caches internally created managers (BindingContext.cs line 278, master), so the fallback is never reached for related managers created this way.

Result: the related manager's ParentManager_CurrentItemChanged subscription on the broad CurrentItemChanged event remains in place, unmodified.

Re-entrancy execution flow (net10)

1. Parent item changes
   → CurrencyManager fires CurrentItemChanged
   → RelatedCurrencyManager.ParentManager_CurrentItemChanged enters  (RelatedCurrencyManager.cs line 139)

2. Handler calls PullData()  (line 159)
   → Binding.PullData pushes dirty control value into child object's setter

3. Child setter executes:
   - Writes backing field
   - Calls callback: parentList.ResetItem(0)
   - ResetItem → BindingList fires ListChanged
   - Parent CurrencyManager handles ListChanged → fires CurrentItemChanged again  (CurrencyManager.cs line 720)

4. RelatedCurrencyManager.ParentManager_CurrentItemChanged is invoked a second time
   while the first invocation is still inside PullData()
   → No guard exists → handler body executes again
   → PullData() is called a second time

5. Repeat from step 3 until StackOverflowException

This is the exact scenario reproduced by the regression test: the ReentrantChildDataSource.Value setter calls parentList.ResetItem(0) while the first PullData() is still on the call stack, and ValueSetCount > 1 would throw InvalidOperationException without the guard (BindingContextTests.cs lines 57–72).

Fix

Add an instance-level re-entrancy guard around RelatedCurrencyManager.ParentManager_CurrentItemChanged.

This fixes the unsafe recursion at the forked WinForms source, so every package consumer gets the corrected behavior without CargoWise-specific reflection against BindingContext internals. The outer handler still performs the normal PullData, related-list refresh, position update, and current-change notifications; only nested invocations against the same related manager are skipped while the handler is already active.

Tests

Added regression coverage that binds through BindingContext to a related child list, marks the binding dirty, and raises a parent item change. The child setter raises another parent item change while PullData() is active. The test verifies the setter runs once and no DataError is raised.

Validation:

• GitHub Actions Build - Debug passed for this PR.
• Local focused direct xUnit execution in the upstream-style unit project is blocked on this machine by the repo AxHost COM prerequisite (AxWMPLib missing while building src\test\unit\.NET Framework\AxHosts\AxHosts.csproj).
• A local Release WTG.Windows.Forms.0.1.1-pr.16.1 package was built from this branch and used to validate CargoWise.
• CargoWise representative accounting basher test passed on net10 with the Release PR package assemblies loaded: MatchEPaymentRecipientsBulkFormAccountingZFormBasherTest.TestBashingForm.

[zuizuihao]

we will need to avoid change winforms source code, why cannot we fix in Cargowise?

[Copilot]

Pull request overview

This PR addresses a stack-overflow recursion path in WinForms related binding managers by adding a re-entrancy guard to RelatedCurrencyManager.ParentManager_CurrentItemChanged, and adds regression tests to ensure related manager updates do not re-enter during PullData().

Changes:

• Add an instance-level re-entrancy guard to RelatedCurrencyManager.ParentManager_CurrentItemChanged.
• Add a regression unit test reproducing re-entrant parent item changes during PullData() for related lists.
• Add equivalent regression coverage in the legacy test project.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/System.Windows.Forms/System/Windows/Forms/DataBinding/RelatedCurrencyManager.cs	Adds a guard intended to prevent re-entrant CurrentItemChanged handling and resulting recursion.
src/test/unit/System.Windows.Forms/System/Windows/Forms/BindingContextTests.cs	Adds a unit test and helper types to reproduce re-entrant updates via BindingContext and related managers.
src/System.Windows.Forms.Legacy/System.Windows.Forms.Legacy.Tests/BindingContextTests.cs	Adds legacy test coverage mirroring the new regression scenario.

Comments suppressed due to low confidence (3)

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

• Avoid this. qualification for field access here (enforced as error via .editorconfig dotnet_style_qualification_for_field=false:error). Use the unqualified field name in the setter.

                this.value = value;
                valueSetCallback();
            }

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

• This new field is missing an explicit accessibility modifier (required by .editorconfig IDE0040), which will fail the build. Mark it private and keep field access unqualified.

    private class ReentrantBindableComponent : BindableComponent
    {
        string value;

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

• Avoid this. qualification for field access in the equality check and assignment (enforced as error via .editorconfig). Use the unqualified field name instead.

            {
                if (this.value == value)
                {
                    return;
                }

                this.value = value;
                ValueChanged?.Invoke(this, EventArgs.Empty);