dotnet / dotnet/winforms

Control.Dispose leaves ScaledControlFont referencing a disposed Font, making Control.Font throw after disposal

Open
#14,839 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
4.9k
Forks
1.1k
Avg merge
1d 13m
Merged PRs (30d)
85

Description

### Summary

`Control.Dispose` disposes every `Font` cached in `_dpiFonts` but does not clear `_scaledControlFont`, which aliases those exact instances. After disposal, `Control.Font` therefore returns a **disposed** `Font`, and every member that touches the native GDI+ handle throws.

This has been present since #9112 (May 2023), i.e. it ships in .NET 8 and later. It only manifests in PerMonitorV2 applications where font scaling has produced a cached DPI font.

### Observed exception

```
System.ArgumentException: Parameter is not valid.
at System.Drawing.Font.GetHeight()
at System.Windows.Forms.ButtonInternal.RadioButtonModernAdapter.PaintCore(PaintEventArgs e)
at System.Windows.Forms.ButtonBase.OnPaint(PaintEventArgs pevent)
at System.Windows.Forms.RadioButton.OnPaint(PaintEventArgs pevent)
at System.Windows.Forms.Control.PaintWithErrorHandling(PaintEventArgs e, Int16 layer)
at System.Windows.Forms.Control.WmPaint(Message& m)
at System.Windows.Forms.Control.WndProc(Message& m)
at System.Windows.Forms.NativeWindow.Callback(HWND hWnd, UInt32 msg, WPARAM wparam, LPARAM lparam)
```

`GdipGetFontHeight` returns `InvalidParameter` for a freed native font handle, which surfaces as `ArgumentException("Parameter is not valid.")`.

### Root cause

In `Control`:

1. `GetScaledFont` caches owned `Font` instances in `_dpiFonts` and **returns the dictionary values by reference**.
2. DPI handling assigns one of those same instances to `ScaledControlFont` (`_scaledControlFont`).
3. `GetCurrentFontAndDpi` — which backs the public `Control.Font` getter — returns `ScaledControlFont` first, before any explicit font or parent walk.
4. `ClearDpiFonts()` disposes every value in `_dpiFonts` and clears the dictionary, but leaves `_scaledControlFont` pointing at a now-disposed instance.
5. `Dispose(bool)` calls `ClearDpiFonts()`.

The intended invariant is visible in the `Font` **setter**, which does the right thing:

```csharp
if (ScaleHelper.IsThreadPerMonitorV2Aware)
{
ScaledControlFont = null; // clear the alias FIRST
ClearDpiFonts();
}
```

The dispose path omits the first line.

Two related loose ends:

- `ClearDpiFonts` does not reset the `s_fontHeightProperty` cache, producing an asymmetry where `Control.FontHeight` still returns a value while `Control.Font.Height` throws.
- `SetScaledFont` can also alias the same cached instance into `s_fontProperty`, so clearing `ScaledControlFont` alone would not fully close the hole.

### Impact

After `control.Dispose()`, on a PerMonitorV2 app where font scaling has occurred:

- `Control.Font` returns a disposed instance (non-null).
- Managed members still return correct values (`Name`, `Size`, `Style`, `Unit`, `FontFamily`) because those are cached managed fields.
- Native-touching members throw: `Height`, `GetHeight()`, `ToHfont()`, `ToLogFont()`, `SizeInPoints` (when `Unit != Point`).

Anything that paints or measures using `Control.Font` after disposal has begun — including a `WM_PAINT` that arrives while a control tree is being torn down — can fault. Because `Control.PaintWithErrorHandling` rethrows after setting `States.ExceptionWhilePainting`, this surfaces as an unhandled-exception dialog in a Release build with no debugger attached.

### Proposed fix

Two options were considered. **Option B is recommended** on backward-compatibility grounds.

**Option A — clear `ScaledControlFont` before disposing `_dpiFonts`** (mirrors the `Font` setter).

After the fix, `Control.Font` on a disposed control falls through the parent walk (the parent is already detached) and returns `DefaultFont`. That is a **silent behaviour change**: code that reads `control.Font.Name` or `.Size` in a `Disposed` handler — for example persisting UI state during teardown — currently receives the real scaled-font values and would afterwards receive `DefaultFont` values, with no exception to signal it. A silent wrong-value regression is arguably worse than the current throw.

**Option B (recommended) — do not dispose instances that are still aliased.**

In `ClearDpiFonts`, skip disposing the instance referenced by `_scaledControlFont` (and any instance aliased into `s_fontProperty` by `SetScaledFont`); dispose the remaining `_dpiFonts` entries exactly as today.

- `Control.Font` keeps returning the **same instance with the same values** as today.
- Members that previously threw now succeed and return the correct value.
- This is strictly a **throw → success** transition with **no observable value change anywhere**.
- Cost: at most one `Font` per disposed control is released by its finalizer rather than eagerly. `Font` has a finalizer that frees the native handle, so this is deferred cleanup, not a leak.

Known workaround shapes all survive Option B:

| Workaround | Survives Option B |
|---|---|
| `control.Font = null` before disposing (already takes the safe setter path) | Yes, untouched |
| `catch (ArgumentException)` around paint / font access | Yes — the catch simply stops being hit |
| Overriding `OnPaint` to skip while `Disposing` | Yes, untouched |
| Disposing the font manually | Yes — `Font.Dispose` is idempotent |

No `AppContext` switch is required for Option B. Option A would need one.

Additionally: reset the `s_fontHeightProperty` cache in the same place so `Control.FontHeight` and `Control.Font.Height` cannot disagree.

### Suggested tests

PerMonitorV2 coverage for, after `Dispose()`:

- explicitly set font
- inherited font
- font scaled to a non-96 DPI and then scaled back to 96 DPI
- `Control.Font.Height` does not throw, and `Control.FontHeight` agrees with it

### Notes

Found while investigating an animation stress test that closes a Form with many animated `ButtonBase` controls. The animation work is unrelated to this defect and is tracked separately; this bug is reachable without any of it, on .NET 8+.

Contributor guide

Open the contributing guide

Research direction

Start by reading Control.Dispose(bool), ClearDpiFonts, SetScaledFont, the Font setter, and GetCurrentFontAndDpi to trace the cached-font aliases. Add PerMonitorV2 coverage for the listed font scenarios after Dispose(); done means Control.Font.Height does not throw and Control.FontHeight agrees with it.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
desktop
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.