fix(AdornerLayer): remove unsafe _layoutDirty gate + re-entrancy leases#6
Merged
Merged
Conversation
…eases 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 Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a correctness regression introduced by
82dc1d5a5(perf: dirty-bit guard around AdornerLayer.UpdateAdorner walk). The_layoutDirtygate inOnLayoutUpdatedwas an unsafe coarse correctness gate for coordinate-space changes it cannot comprehensively observe. Reverting only that gate (one line) while keeping the bigger empty-AdornerLayer fast path fromdfe6bd478(17x render-scheduling drop when no user adorners are attached).Also adds re-entrancy leases on
_removeListand_keysSnapshotBuffersinceUpdateAdornernow runs on every non-emptyLayoutUpdatedfire — same lease pattern as_zOrderValuesSnapshotBufferinMeasureOverride/ArrangeOverride.The bug
_layoutDirtywas gated by:Add()/Remove()/Update()/SetAdornerZOrder()setting it trueLayoutUpdatedhandlerOnAdornedElementLayoutUpdatedWhen a
ContentPresenterswaps content (DataContext rebind triggers template application), the intermediate visual subtree between an adorned elementEand theAdornerLayerparent is rebuilt.E'sTransformToAncestor(adornerLayerParent)will return a different matrix. ButEitself may not be re-measured/arranged (only its ancestors are), soE.LayoutUpdateddoes not fire. Meanwhile the layer's ownLayoutUpdateddoes fire. Result:ElementMap.Count > 0(adorner still registered for E)_layoutDirty == false(no per-element handler fired)UpdateAdornerskippedFor MotionCatalyst this manifested as: opening a take inside the AnalysisView transitioned UI state correctly, but no video frames or data-boxes appeared (stale transforms mapped them off-screen / to wrong position).
What changes
Single file:
AdornerLayer.cs.internal void OnLayoutUpdated(object sender, EventArgs args) { if (ElementMap.Count == 0) { - _layoutDirty = false; return; } - - if (!_layoutDirty) return; // ← THE BUG - _layoutDirty = false; + // Always run on non-empty layer. Element-level LayoutUpdated subscriptions + // miss the case where an ancestor (ContentPresenter content swap) changes + // the element's transform relative to the AdornerLayer without firing + // layout on the adorned element itself. UpdateAdorner(null); }UpdateAdorneris wrapped in try/finally with_removeListand_keysSnapshotBufferleased per-call.The
_layoutDirtyfield,OnAdornedElementLayoutUpdated,SubscribeToElementLayout,UnsubscribeFromElementLayout, and_subscribedElementsare retained but the field is now write-only. A follow-up cleanup PR can remove the dead infrastructure. Keeping it here keeps the diff minimal and reversible.Perf impact
dfe6bd478) is the dominant win for default windows. 17x render-scheduling drop when no user adorners — unchanged.UpdateElementAdornersstill suppress redundantAdorner.InvalidateMeasurecalls._layoutDirtyshort-circuit when a non-empty layer has no apparent reason to re-walk. Steady-state cost =ElementMapwalk +TryTransformToAncestorAsMatrixcalls per pass, bounded by the typically-small number of adorners.Multi-model consensus
9dce07e92revert_removeListlease pattern (incorporated)3 parallel Opus agents ruled out other candidates from this perf branch:
BooleanBoxes.FalseBox/TrueBoxinDataBindEngine.RequestRun—DispatcherOperation._argsis readonly, no identity-based dedup in dispatch path_zOrderMaplease (PR fix(AdornerLayer): re-entrancy-safe lease for zOrderMap snapshot pool #5) — provably correctSimpleTransform/HasSimpleTransformfast path — everyTransformread audited; semantically equivalent_keysSnapshotBufferpool alone — latent but requires custom adorner re-entrancy9dce07e92(UncommonField always-write FalseBox) flagged medium; kept out of this PRFull investigation notes:
investigation/2026-05-16/.Validation
BUILD/x64_DebugContentPresentercontent swapbuild.ymlon merge →InitialForce.WPF 10.0.0-if.81on nuget.org🤖 Generated with Claude Code