Harden BindingGroup against re-entrant binding-expression changes

[wnvko-msft]

Fixes #1690

Description

BindingGroup iterates its internal _bindingExpressions collection with downward index loops (for (int i = _bindingExpressions.Count - 1; i >= 0; --i)) in ObtainConvertedProposedValues, UpdateValues, CheckValidationRules, and the UpdateTarget loop in CancelEdit. During these loops WPF calls out to user code (value converters, property setters, validation rules). If that user code mutates the binding-expression collection - e.g. a binding leaves the group and the collection shrinks - the cached index becomes stale and _bindingExpressions[i] throws ArgumentOutOfRangeException.

This change hardens those loops: each now iterates over a snapshot taken before the loop (via a new CopyBindingExpressions helper) and skips any expression that has since left the group (bindingExpression.BindingGroup != this). Expressions that join mid-loop always append to the end and were never processed by the original downward loop, so that behavior is unchanged. Side-effecting calls (e.g. UpdateSource) are still invoked for every surviving member, preserving semantics.

Customer Impact

Applications using BindingGroup (commonly via DataGrid row editing/commit) can crash with an unhandled ArgumentOutOfRangeException when a commit or validation modifies the set of bindings in the group. Without this fix the crash takes down the app.

Regression

No. This is a long-standing latent defect in the iteration pattern, not a regression from a recent release.

Testing

• Built PresentationFramework for x86 and x64 (0 warnings/errors).
• Reproduced the original crash with a minimal BindingGroup + commit repro app on the locally-built runtime, then confirmed the same app no longer throws after overlaying the fixed PresentationFramework.dll.
• Manually exercised the edit/commit/cancel paths.

Risk

Low. The change is confined to BindingGroup.cs and only alters how four existing loops enumerate the binding-expression collection (snapshot + membership check) without changing the per-expression work or its ordering. Expressions added mid-loop retain the pre-existing behavior of being deferred to the next pass, and all side-effecting calls continue to run for every surviving member.

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Pull request overview

This PR hardens BindingGroup against re-entrant mutation of its internal binding-expression collection during edit/commit/validation flows, preventing ArgumentOutOfRangeException crashes when user code (converters/setters/validation rules) changes group membership mid-iteration.

Changes:

• Replaces index-based downward loops over _bindingExpressions with snapshot-based iteration to prevent stale indices under re-entrancy.
• Adds CopyBindingExpressions() helper and uses it in CancelEdit, ObtainConvertedProposedValues, UpdateValues, and CheckValidationRules.
• Skips snapshot entries that have since left the group (bindingExpression.BindingGroup != this) to preserve per-expression semantics for surviving members.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[h3xds1nz]

You cannot possibly allocate an array everytime during evaluation.

Sure, the re-entrancy case should be handled gracefully, but this only decreases perf for everyone else based on a degenerate edge case. And it still keeps the undefined behaviour of adding new bindings.