Control.Dispose leaves ScaledControlFont referencing a disposed Font, making Control.Font throw after disposal
- 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
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