dotnet / dotnet/winforms

.NET 11 animation stack: renderers keep running and can be resurrected after control disposal

Open
#14,840 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
4.9k
Forks
1.1k
Avg merge
20h 23m
Merged PRs (30d)
103

Description

### Summary

The animation infrastructure added in #14809 (`HighPrecisionTimer`, `AnimationManager`, `AnimatedControlRenderer` and the modern button adapters) has no disposal gating. Renderers keep being driven after their owning control is disposed, can be **resurrected** by a late paint, and the modern paint path touches `Control.Font` before any renderer is consulted.

This was found while stress-testing ~156 `ButtonBase` controls with ~40 animating concurrently and then closing the Form. All of it is new in .NET 11 preview and has never shipped stable, so there is no downlevel compatibility constraint on the fixes.

### Defects

#### 1. `AnimatedControlRenderer` has no disposed gate

`StartAnimation`, `RestartAnimation`, `AnimationProc`, `Invalidate` and `CompleteAnimation` all remain fully functional after `Dispose()`. There is no `_disposed` check and no check of `Control.IsDisposed` / `Control.Disposing` / `GetAnyDisposingInHierarchy()`.

#### 2. `Dispose` does not stop the animation

`AnimatedControlRenderer.Dispose(bool)` unsubscribes `SystemVisualSettingsChanged` and unregisters from `AnimationManager`, but never sets `IsRunning = false`. `AnimationManager.OnFrameTickAsync` iterates a snapshot, so a tick already in flight still reaches `ProcessRenderer`, which gates only on `item.Renderer.IsRunning` — and that is still `true`. The disposed renderer is driven for at least one more frame, calling `AnimationProc` and `Invalidate` on a disposed control.

#### 3. Renderer resurrection via lazy properties

Every renderer is exposed through a lazy `??= new(...)` property:

- `RadioButton.RadioGlyphRenderer`, `RadioButton.ToggleSwitchRenderer`
- `CheckBox.CheckGlyphRenderer`, `CheckBox.ToggleSwitchRenderer`
- `TextBoxBase.FocusIndicatorRenderer`
- `UpDownBase.FocusIndicatorRenderer`

`RadioButton.Dispose` correctly disposes and nulls `_radioGlyphRenderer`, but `RadioButtonModernAdapter.PaintCore` unconditionally reads `Control.RadioGlyphRenderer`. A `WM_PAINT` arriving after disposal therefore **constructs a brand new renderer on a disposed control**, re-subscribes `SystemVisualSettingsChanged`, and can re-register it with `AnimationManager` (`NotifyCheckedChanged` → `RestartAnimation`). That is a resurrection plus a rooted-object leak for the lifetime of the manager.

#### 4. Modern adapters touch `Control.Font` before any renderer gate can help

`RadioButtonModernAdapter.Layout` and `CheckBoxModernAdapter.Layout` compute `CheckSize` from `Control.Font.Height`:

```csharp
layout.CheckSize = Math.Max(
Control.LogicalToDeviceUnits(13),
(int)(Control.Font.Height * 0.9f));
```

This is new behaviour — every classic adapter uses a constant (`StandardCheckSize = 13`, `RadioButtonFlatAdapter.FlatCheckSize = 12`, `CheckBoxBaseAdapter.FlatCheckSize = 11`) or `RadioButtonRenderer.GetGlyphSize`. Two consequences:

- `Control.Font.Height` is uncached: it acquires a screen DC, creates a `Graphics`, and calls `GdipGetFontHeight` on **every paint**.
- It runs in `Layout(e)`, i.e. **before** the renderer is reached, so gating the renderer alone cannot prevent a fault here. This is the frame that actually threw in the reported stack.

#### 5. `HighPrecisionTimer`: `InFlight` can stick at 1 forever

`DispatchCallbacks` sets `registration.InFlight` on the timer thread before `SynchronizationContext.Post`. The flag is cleared only when the posted callback actually executes. If the marshalling control's handle is destroyed after the callback was queued, `Control.DestroyHandle` drains the callback list **without invoking the delegate**, so `InFlight` stays `1`. That registration then coalesces every subsequent frame forever and emits a `DroppedFrames` event per tick without ever recovering.

If `Post` itself throws, the existing `catch` + `ConsecutiveFaultLimit` handles it correctly — it is only the "queued, then dropped" path that is unrecoverable.

#### 6. Synchronously-completed `ValueTask` is never consumed

`DispatchCallback` inspects `callbackTask.IsCompletedSuccessfully` and returns without consuming the `ValueTask`. That violates the consumption contract for pooled `IValueTaskSource` implementations. It should call `GetAwaiter().GetResult()` on that path.

#### 7. Bare `catch (Exception)` in the callback paths

`DispatchCallback` and `AwaitCallbackAsync` catch bare `Exception`, converting critical exceptions (e.g. `StackOverflowException`-adjacent, `OutOfMemoryException`, `ThreadAbortException`) into ordinary callback faults. Every neighbouring catch in the same file uses `when (!ex.IsCriticalException())`.

#### 8. `DroppedFrames` diagnostics missing an `IsEnabled()` guard

`Drift` and `ResidualSpin` are guarded with `HighPrecisionTimerEventSource.s_log.IsEnabled(...)`; `DroppedFrames` is not. It sits on the hot coalescing path, and defect 5 makes it fire every single frame once a registration is stuck.

### Proposed approach

Apply the gate as **defense in depth**, because no single layer covers every path:

```
control disposal begins
→ AnimatedControlRenderer: set _disposed FIRST, set IsRunning = false,
unregister from AnimationManager immediately
→ renderer accessors do not allocate while disposing/disposed;
paint adapters use the backing field, not the lazy property
→ AnimationManager.ProcessRenderer re-checks before virtual dispatch
and drops disposed renderers
→ modern paint adapters bail out when GetAnyDisposingInHierarchy() is true
(required, because Layout(e) reads Control.Font before any renderer is reached)
```

Two guidelines that fell out of the analysis:

- **Do not gate primarily on `HandleDestroyed`** — it also fires during legitimate handle recreation.
- **Do not gate only on `Control.Disposing`** — the renderer-owning subclass (e.g. `RadioButton.Dispose`) begins disposing its renderers *before* `Control.Dispose` sets that flag.

One open design decision: when the modern adapter bails out during teardown, should it return completely early, or still paint the parent background? Returning early is simplest and invisible during window destruction, but a slow `Dispose` on a large form could briefly show an unpainted control.

### Related performance follow-ups

Found in the same investigation, listed roughly by value-to-effort. Not blocking, but several are one-liners:

1. **`AnimationManager` never releases its `HighPrecisionTimer` registration.** It registers once per UI thread in its constructor and holds it until `Application.ThreadExit`, so a 60 Hz pacer, a `BeginInvoke` marshal and a dictionary walk run forever even when nothing is animating. Reference-count it with a short hysteresis (~200-500 ms) to avoid first-frame jank, and reset `_hasTickTimestamp` on restart so a new animation cannot inherit a stale `TargetTimestamp`. Largest idle CPU/power win.
2. **`RestartAnimation` churns the registration.** It goes `StopAnimation` → `AnimationManager.Suspend` (removes from the dictionary) → `StartAnimation` (allocates a new `AnimationRendererItem`). Under load this fires twice per control per state change. `RegisterOrUpdateAnimationRenderer` already has an in-place update path that this defeats. `OnAnimationStarted()` must still run so each renderer re-captures its start values and resets `AnimationProgress`; all five `OnAnimationStopped()` overrides are empty, so skipping the stop notification loses nothing. Largest active-load win.
3. `AnimationManager.OnFrameTickAsync` enumerates `_renderer.Values`, which snapshots a `ConcurrentDictionary` into a new collection **every frame**. Enumerate the dictionary directly.
4. Use a cached control font height in the modern adapters instead of `Control.Font.Height` (see defect 4). Note `Control.FontHeight` is `protected` and the adapters are not `Control` subclasses, so this needs a small `internal` accessor, and it must be validated across DPI transitions.
5. `AnimatedRadioGlyphRenderer.DrawGlyph` uses `graphics.Save()`/`Restore()` — a `GraphicsState` allocation plus a full GDI+ container save — when only `SmoothingMode` changes. `AnimatedToggleSwitchRenderer` already does the cheaper local save.
6. Same method allocates `new Pen(borderColor, borderThickness)` per paint instead of `GetCachedPenScope(thickness)` as the rest of the repo does. Tempered expectation: `GdiPlusCache` only caches width-1 pens, so this still allocates above 100% DPI.

Deliberately **not** proposed: moving rendering off the UI thread, or a Direct2D/Composition-backed path. `Graphics` is bound to a UI-thread-owned DC and `WM_PAINT` must be serviced there; the effort is architectural and far out of proportion to the measured win.

Worth profiling before committing to anything: `ColorOptions.Calculate` (two allocations and ~10 `FindNearestColor` calls per paint), `ParentBackgroundRenderer.Paint` (two GDI+ `Region`s plus a `graphics.Clip` read plus a parent repaint, per control per frame — likely the largest active cost, but transparency correctness is subtle), and the per-paint `LayoutOptions`/`LayoutData` allocations in every button adapter.

### Notes

The same investigation surfaced a **pre-existing** defect that produces an identical-looking `ArgumentException` from `Control.Font` and is reachable on .NET 8+ without any of this animation code: dotnet/winforms#14839. The gating fixes proposed here will *mask* that bug in the animated-button path, so the two should be fixed independently.

The application used to reproduce this also had a genuine lifetime bug of its own (it disposed a shared `Font` that live controls still referenced, and it forced a synchronous `Update()` across 156 controls from `OnHandleDestroyed`). That app bug is being fixed separately — it is what made the framework gaps above fire reliably, but every defect listed here is independently reachable.

### Suggested tests

- After `Dispose()`: `StartAnimation`, `RestartAnimation`, `AnimationProc` and `Invalidate` are no-ops, `IsRunning` is `false`, and the renderer is unregistered.
- A tick delivered after renderer disposal does not invoke `AnimationProc`; disposed renderers are dropped from the manager.
- Touching `RadioButton.RadioGlyphRenderer` (and the `CheckBox` / `TextBoxBase` / `UpDownBase` equivalents) after `Dispose()` neither allocates a renderer nor re-registers one.
- Disposing a Form containing modern `RadioButton`s / `CheckBox`es while animations are running and a synchronous `Update()` is forced must not throw.
- `HighPrecisionTimer`: `InFlight` recovers when a posted callback is dropped, and the registration continues receiving frames afterwards.

Contributor guide

Open the contributing guide

Research direction

Start with AnimatedControlRenderer, AnimationManager, HighPrecisionTimer, and the modern RadioButton/CheckBox adapters named in the issue. Run the suggested disposal, resurrection, teardown, and timer tests first, then trace each reported lifetime path. Done means disposed renderers are inert and unregistered, adapters do not resurrect them or throw during teardown, and dropped callbacks recover.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
desktop
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.