Skip to content

Commit f703b12

Browse files
oysteinkrogClaude (Initial Force WPF Bot)claude
authored
fix(AdornerLayer): remove unsafe _layoutDirty gate; add re-entrancy leases (#6)
The dirty-bit gate in OnLayoutUpdated (82dc1d5) 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 dfe6bd4 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 9dce07e 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 (Initial Force WPF Bot) <wpf-bot@initialforce.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 02fe016 commit f703b12

1 file changed

Lines changed: 73 additions & 42 deletions

File tree

  • src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents

src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/AdornerLayer.cs

Lines changed: 73 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -664,17 +664,21 @@ internal void OnLayoutUpdated(object sender, EventArgs args)
664664
// calls UpdateAdorner→TransformToAncestor→InvalidateMeasure on every
665665
// pass, which schedules a new render via NeedsRecalc→PostRender,
666666
// amplifying any forever-animation by ~17× (e.g. a perpetual busy
667-
// spinner produces ~570 renders/sec instead of ~32). Clearing
668-
// _layoutDirty before exit prevents stale-flag leak when the first
669-
// adorner is later attached (oracle-panel correction, gemini 9/10).
667+
// spinner produces ~570 renders/sec instead of ~32).
670668
if (ElementMap.Count == 0)
671669
{
672-
_layoutDirty = false;
673670
return;
674671
}
675672

676-
if (!_layoutDirty) return; // existing dirty-bit guard from 5e7df8833 — keep
677-
_layoutDirty = false;
673+
// Non-empty layer: always run UpdateAdorner. A cached _layoutDirty flag
674+
// cannot comprehensively observe every coordinate-space change that
675+
// requires re-walking adorners (ancestor RenderTransform changes,
676+
// ContentPresenter re-templating that swaps the subtree between an
677+
// adorned element and the AdornerLayer parent without firing layout
678+
// on the adorned element itself, ancestor scroll, layer-parent
679+
// changes, ArrangeDirty propagation, stale-element cleanup, etc.).
680+
// The transform/size/clip-change gates inside UpdateElementAdorners
681+
// still suppress redundant Adorner.InvalidateMeasure calls.
678682
UpdateAdorner(null);
679683
}
680684

@@ -969,55 +973,82 @@ private void UpdateAdorner(UIElement element)
969973
return;
970974
}
971975

972-
// Reuse pooled list to avoid per-call ArrayList allocation.
973-
_removeList ??= new List<UIElement>(4);
974-
_removeList.Clear();
975-
List<UIElement> removeList = _removeList;
976+
// Lease the pooled removeList: null the field so a re-entrant
977+
// UpdateAdorner (triggered when a custom Adorner's InvalidateMeasure/
978+
// InvalidateVisual override calls back into AdornerLayer.Add/Remove)
979+
// allocates its own buffer instead of clobbering ours. Same pattern
980+
// as _zOrderValuesSnapshotBuffer in Measure/ArrangeOverride.
981+
List<UIElement> removeList = _removeList ?? new List<UIElement>(4);
982+
_removeList = null;
983+
removeList.Clear();
976984

977-
if (element != null)
978-
{
979-
// Make sure element is still beneath the adorner decorator
980-
if (!element.IsDescendantOf(adornerLayerParent))
981-
{
982-
removeList.Add(element);
983-
}
984-
else
985-
{
986-
UpdateElementAdorners(element);
987-
}
988-
}
989-
else
985+
// Lease the pooled keys snapshot buffer for the same reason.
986+
UIElement[] keysBuffer = null;
987+
int leasedKeysCount = 0;
988+
989+
try
990990
{
991-
ICollection keyCollection = ElementMap.Keys;
992-
int keysCount = keyCollection.Count;
993-
// Reuse a grow-only snapshot buffer; min capacity 8.
994-
if (_keysSnapshotBuffer == null || _keysSnapshotBuffer.Length < keysCount)
995-
_keysSnapshotBuffer = new UIElement[Math.Max(keysCount, 8)];
996-
keyCollection.CopyTo(_keysSnapshotBuffer, 0); // static snapshot to prevent enumerator exceptions
997-
998-
for (int i = 0; i < keysCount; i++)
991+
if (element != null)
999992
{
1000-
UIElement elTemp = _keysSnapshotBuffer[i];
1001-
1002993
// Make sure element is still beneath the adorner decorator
1003-
if (!elTemp.IsDescendantOf(adornerLayerParent))
994+
if (!element.IsDescendantOf(adornerLayerParent))
1004995
{
1005-
removeList.Add(elTemp);
996+
removeList.Add(element);
1006997
}
1007998
else
1008999
{
1009-
UpdateElementAdorners(elTemp);
1000+
UpdateElementAdorners(element);
10101001
}
10111002
}
1003+
else
1004+
{
1005+
ICollection keyCollection = ElementMap.Keys;
1006+
int keysCount = keyCollection.Count;
10121007

1013-
// Clear used slots to release UIElement refs; prevents the buffer from
1014-
// retaining strong references to elements after this call returns.
1015-
Array.Clear(_keysSnapshotBuffer, 0, keysCount);
1016-
}
1008+
keysBuffer = _keysSnapshotBuffer;
1009+
_keysSnapshotBuffer = null;
1010+
if (keysBuffer == null || keysBuffer.Length < keysCount)
1011+
keysBuffer = new UIElement[Math.Max(keysCount, 8)];
1012+
keyCollection.CopyTo(keysBuffer, 0); // static snapshot to prevent enumerator exceptions
1013+
leasedKeysCount = keysCount;
1014+
1015+
for (int i = 0; i < keysCount; i++)
1016+
{
1017+
UIElement elTemp = keysBuffer[i];
1018+
1019+
// Make sure element is still beneath the adorner decorator
1020+
if (!elTemp.IsDescendantOf(adornerLayerParent))
1021+
{
1022+
removeList.Add(elTemp);
1023+
}
1024+
else
1025+
{
1026+
UpdateElementAdorners(elTemp);
1027+
}
1028+
}
1029+
}
10171030

1018-
for (int i = 0; i < removeList.Count; i++)
1031+
for (int i = 0; i < removeList.Count; i++)
1032+
{
1033+
Clear(removeList[i]);
1034+
}
1035+
}
1036+
finally
10191037
{
1020-
Clear(removeList[i]);
1038+
// Clear used slots in the keys buffer before returning it to the
1039+
// pool so it doesn't retain UIElement references across calls.
1040+
if (keysBuffer != null)
1041+
{
1042+
Array.Clear(keysBuffer, 0, leasedKeysCount);
1043+
if (_keysSnapshotBuffer == null || _keysSnapshotBuffer.Length < keysBuffer.Length)
1044+
_keysSnapshotBuffer = keysBuffer;
1045+
}
1046+
1047+
// Return removeList to the pool only if a nested call hasn't
1048+
// already produced a (potentially larger) replacement.
1049+
removeList.Clear();
1050+
if (_removeList == null)
1051+
_removeList = removeList;
10211052
}
10221053
}
10231054

0 commit comments

Comments
 (0)