microsoft / microsoft/microsoft-ui-reactor
[Bug] Pooled Grid never releases BorderThickness/BorderBrush — ElementPool.CleanElement has no border-box clear for concrete panels
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
## Summary
`Grid` is in `ElementPool.PoolableTypes`, and `Grid` exposes `BorderThickness` / `BorderBrush` dependency properties. `ElementPool.CleanElement` never clears either one, so a pooled `Grid` that was given a **local** value for those DPs hands that value to its next renter. A local value outranks every `Style` setter in WinUI's DP precedence order, so the next renter cannot style its way out of it.
This is the same defect class as #985 (which covered `Padding` / `CornerRadius` / `BorderThickness` / `BorderBrush` / `Background` / `IsEnabled` on `Control` / `Border` / `Panel` / `Grid` / `StackPanel` receivers), but it sits outside that fix's invariant — see *Why #985 does not cover it* below.
## Evidence
`Grid` is poolable:
```csharp
// src/Reactor/Core/ElementPool.cs:45-64 — PoolableTypes
typeof(WinUI.Grid),
```
`CleanElement`'s `Panel` arm clears `Panel.Background` plus the Grid/StackPanel `Padding` and `CornerRadius`, but nothing border-box:
```csharp
// src/Reactor/Core/ElementPool.cs:311-329
else if (fe is WinUI.Panel resetPanel)
{
resetPanel.ClearValue(WinUI.Panel.BackgroundProperty);
if (resetPanel is WinUI.Grid resetGrid)
{
resetGrid.ClearValue(WinUI.Grid.PaddingProperty);
resetGrid.ClearValue(WinUI.Grid.CornerRadiusProperty);
// no BorderThickness / BorderBrush clear
}
else if (resetPanel is WinUI.StackPanel resetStack) { ... }
}
```
Measured — `Grid.BorderThicknessProperty` and `Grid.BorderBrushProperty` each appear **0 times** in `ElementPool.cs`.
There is a **real in-repo site** that writes them:
```csharp
// samples/apps/regedit/Components/ValueList.cs:38-44
.Set(g =>
{
// BorderThickness/BorderBrush remain imperative until their modifier gates
// grow concrete Grid support.
g.BorderThickness = new Thickness(0, 0, 0, 1);
g.BorderBrush = (Microsoft.UI.Xaml.Media.Brush)Application.Current.Resources["DividerStrokeColorDefaultBrush"];
});
```
That `Grid` is a column header inside a list component — precisely the mount/unmount churn pooling exists to serve.
The path is also **unflagged**: `ModifierTable` gates both properties to `Control | Border`, so `REACTOR_POOL_001` does not fire on a `Grid` receiver, and there is a test that pins that as intentional (`tests/Reactor.Tests/AnalyzerTests/ModifierAvailableAnalyzerTests.cs:216-227`). So the leak is silent at author time *and* at build time.
## Why #985 does not cover it
#985 establishes and enforces the invariant *"every property `ApplyModifiers` writes to receiver R must be cleared by `CleanElement` for receiver R."* `Grid.BorderThickness` / `Grid.BorderBrush` fall outside it because `ApplyModifiers` writes those two only to `Control` and `Border`:
```csharp
// src/Reactor/Core/Reconciler.cs:3882-3892
// BorderBrush / BorderThickness (on Control and Border)
if (fe is WinUI.Control bbCtrl) bbCtrl.BorderBrush = m.BorderBrush;
else if (fe is WinUI.Border bbBdr) bbBdr.BorderBrush = m.BorderBrush;
```
Measured as unchanged by #985/#1015: `Grid.BorderThicknessProperty` clears = **0 on `origin/main` and 0 on the PR head**; the PR touches `samples/` in **0 lines**; and its only edit to those two `ModifierTable` rows adds `poolReset: true` while leaving `controlGate: ControlBorder` on both sides. The leak is byte-identical before and after.
## Suggested fix — widen the gate, don't just add a clear
Adding a bare `resetGrid.ClearValue(WinUI.Grid.BorderThicknessProperty)` would stop this specific leak but leave `CleanElement` clearing a property the modifier table says is unreachable on that type, and would leave the sample stuck on `.Set`.
The layer the sample's own comment asks for is the gate:
1. Widen `BorderThickness` / `BorderBrush` `controlGate` in `src/Reactor.Analyzers/ModifierTable.cs` to include the concrete panels that actually carry the DPs (`Grid`, and check `StackPanel` / `RelativePanel`), mirroring what #1003 did for `Padding` / `CornerRadius`.
2. Widen the corresponding `ApplyModifiers` receiver chains in `Reconciler.cs`.
3. The `CleanElement` clears then follow from the existing invariant, and `PoolResetSetConsistencyTests` + the pin list in `ModifierUnsetClearValueTests.cs` enforce them.
4. Rewrite `ValueList.cs:38-44` to use the fluent `.BorderThickness(...)` / `.BorderBrush(...)` modifiers and drop the imperative `.Set` block and its comment.
Note step 1 turns the existing `.Set` site into a `REACTOR_POOL_001` warning, and `samples/` builds Release with `TreatWarningsAsErrors`, so steps 1 and 4 must land together.
## Test the fix needs
A selftest fixture in `tests/Reactor.AppTests.Host/SelfTest/Fixtures/`, following `ModifierPoolClearValueControlPanel`: mount a `Grid` carrying border-box values, assert phase-0 `ReadLocalValue != UnsetValue` (so the cleared assertion can come out the other way), drop it to force a pool return, remount, assert `ReferenceEquals` instance reuse and phase-2 `ReadLocalValue == UnsetValue`. Mutation-check it by replacing the product `ClearValue` with `_ = 0;` — not by commenting it out, which is false-green for the source scanners.
## Provenance
Found by the multi-model cross-check dimension of the repo `pr-review` skill (GPT-5.4, a different model family from the reviewing orchestrator) while reviewing PR #1015. Seven same-family dimension agents and two rounds of Copilot review on that PR did not surface it.
Contributor guide
Research direction
Start with src/Reactor/Core/ElementPool.cs, src/Reactor.Analyzers/ModifierTable.cs, and the matching receiver logic in src/Reactor/Core/Reconciler.cs. Read the pool consistency and modifier-clear tests, especially ModifierPoolClearValueControlPanel, PoolResetSetConsistencyTests, and ModifierUnsetClearValueTests.cs. Done means the Grid border properties are gated, applied, cleared and tested across pool reuse, and samples/apps/regedit/Components/ValueList.cs uses the fluent modifiers.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- desktop, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100