From 5a5150f553ecaed2e47fa74004141cc575b123ea Mon Sep 17 00:00:00 2001 From: "Claude (Initial Force WPF Bot)" Date: Sun, 17 May 2026 18:33:36 +0200 Subject: [PATCH] fix(AdornerLayer): remove unsafe _layoutDirty gate; add re-entrancy leases The dirty-bit gate in OnLayoutUpdated (82dc1d5a5) was an unsafe correctness hazard. The cached _layoutDirty flag cannot comprehensively observe every coordinate-space change that requires re-walking adorners: - ContentPresenter re-templating that rebuilds the intermediate subtree between an adorned element E and the AdornerLayer parent without firing layout on E itself - Ancestor RenderTransform / scroll / clip changes - Layer-parent changes - ArrangeDirty propagation - Stale-element cleanup when E exits the adorner-decorator subtree For MotionCatalyst this manifested when opening a take inside the AnalysisView: ContentPresenter swap rebuilt the intermediate visual chain around the adorned elements (video viewports, data-box hosts), their transform-to-ancestor changed, but neither their own LayoutUpdated nor any Add/Remove/Update call armed _layoutDirty. OnLayoutUpdated hit the gate and returned early; UpdateAdorner was skipped; adorners rendered at stale positions. Symptom: take state transitioned but no video frames and no data-boxes appeared. The fix removes only the gate (line 676); the empty-AdornerLayer fast path from dfe6bd478 stays intact (preserves the bigger 17x render scheduling drop). The transform/size/clip change checks inside UpdateElementAdorners still suppress redundant Adorner.InvalidateMeasure calls, so the only steady-state cost is the per-pass ElementMap walk plus TryTransformToAncestorAsMatrix calls. The element-level LayoutUpdated subscription plumbing (SubscribeToElementLayout / OnAdornedElementLayoutUpdated / _layoutDirty field / _subscribedElements set) is retained but the field is now write-only; a future cleanup can remove the unused machinery. Companion safety: now that UpdateAdorner runs on every non-empty LayoutUpdated fire, the pooled _removeList and _keysSnapshotBuffer become re-entrancy hazards. If a custom Adorner's InvalidateMeasure or InvalidateVisual override synchronously calls back into AdornerLayer.Add/Remove, the nested UpdateAdorner would clobber the outer call's lists mid-iteration. Wrap UpdateAdorner's body in try/finally and lease both pools using the same pattern as _zOrderValuesSnapshotBuffer in MeasureOverride/ArrangeOverride. Multi-model consensus: - 3x Opus agents in parallel ruled out other candidates (BooleanBoxes in DataBindEngine, SimpleTransform fast path, _zOrderMap lease, _keysSnapshotBuffer pool alone) - see investigation/2026-05-16 - gpt-5.5-pro (against stance, 7/10): "deleting line 676 is the safest minimal correction"; do not bundle 9dce07e92 revert - gemini-3.1-pro-preview (for stance, 9/10): "completely sufficient"; also recommended _removeList lease pattern (incorporated) Validated end-to-end against MotionCatalyst's regression scenario via binary-swap into BUILD/x64_Debug: take loads with all video viewports, Pressure plate, Launch Monitor, and Torque/Force charts visible after the ContentPresenter content swap. Co-Authored-By: Claude Opus 4.7 --- .../System/Windows/Documents/AdornerLayer.cs | 115 +++++++++++------- 1 file changed, 73 insertions(+), 42 deletions(-) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/AdornerLayer.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/AdornerLayer.cs index 87e379aa155..abe76c029d0 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/AdornerLayer.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/AdornerLayer.cs @@ -664,17 +664,21 @@ internal void OnLayoutUpdated(object sender, EventArgs args) // calls UpdateAdorner→TransformToAncestor→InvalidateMeasure on every // pass, which schedules a new render via NeedsRecalc→PostRender, // amplifying any forever-animation by ~17× (e.g. a perpetual busy - // spinner produces ~570 renders/sec instead of ~32). Clearing - // _layoutDirty before exit prevents stale-flag leak when the first - // adorner is later attached (oracle-panel correction, gemini 9/10). + // spinner produces ~570 renders/sec instead of ~32). if (ElementMap.Count == 0) { - _layoutDirty = false; return; } - if (!_layoutDirty) return; // existing dirty-bit guard from 5e7df8833 — keep - _layoutDirty = false; + // Non-empty layer: always run UpdateAdorner. A cached _layoutDirty flag + // cannot comprehensively observe every coordinate-space change that + // requires re-walking adorners (ancestor RenderTransform changes, + // ContentPresenter re-templating that swaps the subtree between an + // adorned element and the AdornerLayer parent without firing layout + // on the adorned element itself, ancestor scroll, layer-parent + // changes, ArrangeDirty propagation, stale-element cleanup, etc.). + // The transform/size/clip-change gates inside UpdateElementAdorners + // still suppress redundant Adorner.InvalidateMeasure calls. UpdateAdorner(null); } @@ -969,55 +973,82 @@ private void UpdateAdorner(UIElement element) return; } - // Reuse pooled list to avoid per-call ArrayList allocation. - _removeList ??= new List(4); - _removeList.Clear(); - List removeList = _removeList; + // Lease the pooled removeList: null the field so a re-entrant + // UpdateAdorner (triggered when a custom Adorner's InvalidateMeasure/ + // InvalidateVisual override calls back into AdornerLayer.Add/Remove) + // allocates its own buffer instead of clobbering ours. Same pattern + // as _zOrderValuesSnapshotBuffer in Measure/ArrangeOverride. + List removeList = _removeList ?? new List(4); + _removeList = null; + removeList.Clear(); - if (element != null) - { - // Make sure element is still beneath the adorner decorator - if (!element.IsDescendantOf(adornerLayerParent)) - { - removeList.Add(element); - } - else - { - UpdateElementAdorners(element); - } - } - else + // Lease the pooled keys snapshot buffer for the same reason. + UIElement[] keysBuffer = null; + int leasedKeysCount = 0; + + try { - ICollection keyCollection = ElementMap.Keys; - int keysCount = keyCollection.Count; - // Reuse a grow-only snapshot buffer; min capacity 8. - if (_keysSnapshotBuffer == null || _keysSnapshotBuffer.Length < keysCount) - _keysSnapshotBuffer = new UIElement[Math.Max(keysCount, 8)]; - keyCollection.CopyTo(_keysSnapshotBuffer, 0); // static snapshot to prevent enumerator exceptions - - for (int i = 0; i < keysCount; i++) + if (element != null) { - UIElement elTemp = _keysSnapshotBuffer[i]; - // Make sure element is still beneath the adorner decorator - if (!elTemp.IsDescendantOf(adornerLayerParent)) + if (!element.IsDescendantOf(adornerLayerParent)) { - removeList.Add(elTemp); + removeList.Add(element); } else { - UpdateElementAdorners(elTemp); + UpdateElementAdorners(element); } } + else + { + ICollection keyCollection = ElementMap.Keys; + int keysCount = keyCollection.Count; - // Clear used slots to release UIElement refs; prevents the buffer from - // retaining strong references to elements after this call returns. - Array.Clear(_keysSnapshotBuffer, 0, keysCount); - } + keysBuffer = _keysSnapshotBuffer; + _keysSnapshotBuffer = null; + if (keysBuffer == null || keysBuffer.Length < keysCount) + keysBuffer = new UIElement[Math.Max(keysCount, 8)]; + keyCollection.CopyTo(keysBuffer, 0); // static snapshot to prevent enumerator exceptions + leasedKeysCount = keysCount; + + for (int i = 0; i < keysCount; i++) + { + UIElement elTemp = keysBuffer[i]; + + // Make sure element is still beneath the adorner decorator + if (!elTemp.IsDescendantOf(adornerLayerParent)) + { + removeList.Add(elTemp); + } + else + { + UpdateElementAdorners(elTemp); + } + } + } - for (int i = 0; i < removeList.Count; i++) + for (int i = 0; i < removeList.Count; i++) + { + Clear(removeList[i]); + } + } + finally { - Clear(removeList[i]); + // Clear used slots in the keys buffer before returning it to the + // pool so it doesn't retain UIElement references across calls. + if (keysBuffer != null) + { + Array.Clear(keysBuffer, 0, leasedKeysCount); + if (_keysSnapshotBuffer == null || _keysSnapshotBuffer.Length < keysBuffer.Length) + _keysSnapshotBuffer = keysBuffer; + } + + // Return removeList to the pool only if a nested call hasn't + // already produced a (potentially larger) replacement. + removeList.Clear(); + if (_removeList == null) + _removeList = removeList; } }