Fix GradientBrush.GradientStops.Clear() leaks brushes when stops are shared

[devanathan-vaithiyanathan]

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Issue Details

GradientBrush subscribes to each GradientStop's PropertyChanged event, but GradientStops.Clear() raises a collection Reset with e.OldItems == null. Because GradientBrush.OnGradientStopCollectionChanged only unsubscribes stops from e.OldItems, cleared stops remain subscribed.

Description of Change

Added a dictionary to track GradientStop subscriptions and manage event subscriptions correctly. Updated the collection change handling to support all collection actions, including Add, Remove, Replace, Move, and Reset. Fixed the Reset scenario by properly unsubscribing from existing GradientStop instances before re-subscribing to the current collection items. Also added reference counting to correctly handle duplicate GradientStop instances and refactored the collection attach/detach logic into helper methods to improve code maintainability.

Issues Fixed

Fixes #36743

Tested the behavior in the following platforms.

• Android
• Windows
• iOS
• Mac

Before	After
Android Before.mov	Android After.mov

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36746

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36746"

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Fixes a memory leak in GradientBrush where GradientStops.Clear() (Reset notifications with OldItems == null) could leave GradientStop.PropertyChanged handlers subscribed, keeping brushes alive when stops are shared.

Changes:

• Refactors GradientBrush’s collection attach/detach logic and expands CollectionChanged handling to cover all actions, including Reset.
• Introduces subscription tracking with reference counting to correctly subscribe/unsubscribe stops (including duplicate instances in the collection).
• Adds a unit test ensuring Clear() unsubscribes previous gradient stops and prevents spurious invalidations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Controls/src/Core/GradientBrush.cs	Adds ref-counted subscription tracking and robust NotifyCollectionChangedAction handling, including Reset resubscription.
src/Controls/tests/Core.UnitTests/LinearGradientBrushTests.cs	Adds a regression test validating Clear() unsubscribes old stops from PropertyChanged.

[MauiBot]

Expert Review — 2 findings

See inline comments for details.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/Controls/src/Core/GradientBrush.cs:13

• Controls.Core is multi-targeted to netstandard2.0/2.1 (see Controls.Core.csproj). System.Collections.Generic.ReferenceEqualityComparer isn't part of .NET Standard, so this may fail to compile for the netstandard TFMs. Consider using a small local reference comparer (ReferenceEquals + RuntimeHelpers.GetHashCode) instead of ReferenceEqualityComparer.Instance.

		readonly Dictionary<GradientStop, int> _subscriptionRefCounts = new(ReferenceEqualityComparer.Instance);

src/Controls/src/Core/GradientBrush.cs:186

• Similarly, UnsubscribeFromAllGradientStops should only null out stop.Parent when this brush is the current parent, to avoid clobbering the parent relationship when GradientStop instances are shared across brushes.

			foreach (var stop in _subscriptionRefCounts.Keys)
			{
				stop.Parent = null;
				stop.PropertyChanged -= OnGradientStopPropertyChanged;
			}

[MauiBot]

AI Review Summary

@devanathan-vaithiyanathan — new AI review results are available based on this last commit: 31e1444.

---

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: ANDROID · Base: main · Merge base: ee21c935

Test	Without Fix (expect FAIL)	With Fix (expect PASS)
🧪 LinearGradientBrushTests LinearGradientBrushTests	✅ FAIL — 153s	✅ PASS — 109s

🔴 Without fix — 🧪 LinearGradientBrushTests: FAIL ✅ · 153s

Error-relevant lines (filtered from the build log):

     at Microsoft.Maui.Controls.Core.UnitTests.LinearGradientBrushTests.ClearUnsubscribesPreviousGradientStopsFromPropertyChanged() in /_/src/Controls/tests/Core.UnitTests/LinearGradientBrushTests.cs:line 260
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

🟢 With fix — 🧪 LinearGradientBrushTests: PASS ✅ · 109s

(no coded error found; showing last 1200 chars)

es matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.52]   Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:04.30]   Discovered:  Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:04.33]   Starting:    Microsoft.Maui.Controls.Core.UnitTests
  Passed TestNullOrEmptyLinearGradientBrush [27 ms]
  Passed TestConstructorUsingGradientStopCollection [5 ms]
  Passed TestLinearGradientBrushOnlyOneGradientStop [< 1 ms]
  Passed TestHasTransparencyLinearGradientBrush [1 ms]
  Passed TestEmptyLinearGradientBrush [< 1 ms]
  Passed TestConstructor [< 1 ms]
  Passed TestLinearGradientBrushGradientStops [4 ms]
  Passed TestNullGradientStopLinearGradientPaint [5 ms]
  Passed TestNullOrEmptyLinearGradientPaintWithNullGradientStop [2 ms]
[xUnit.net 00:00:04.49]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed TestLinearGradientBrushPoints [< 1 ms]
  Passed TestNullOrEmptyLinearGradientPaintWithEmptyGradientStop [< 1 ms]
  Passed ClearUnsubscribesPreviousGradientStopsFromPropertyChanged [8 ms]

Test Run Successful.
Total tests: 12
     Passed: 12
 Total time: 5.8708 Seconds

📁 Fix files reverted (1 files)

• src/Controls/src/Core/GradientBrush.cs

---

📱 UI Tests — BoxView,Brush

Detected UI test categories: BoxView,Brush

✅ Deep UI tests — 62 passed, 0 failed across 2 categories on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category	Tests	Snapshot diffs
BoxView	20/21 ✓	—
Brush	42/42 ✓	—
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

---

📋 Pre-Flight — Context & Validation

Issue: #36743 - GradientBrush.GradientStops.Clear() leaks brushes when stops are shared
PR: #36746 - Fix GradientBrush.GradientStops.Clear() leaks brushes when stops are shared
Platforms Affected: Android, iOS, Windows, macOS (shared Controls code)
Files Changed: 1 implementation, 1 test

Key Findings

• GradientBrush.OnGradientStopCollectionChanged historically missed Reset/Clear because OldItems is null, leaving cleared shared stops subscribed to old brushes.
• PR fix refactors subscription ownership into helper methods and reference-counts duplicate stops, but current implementation uses ReferenceEqualityComparer.Instance, which is inaccessible for the target framework build and causes CI compile failure.
• Prior inline reviews also flagged subtle ownership coverage: duplicate-stop ref-count tests and shared-stop Parent handling across multiple brushes.

Code Review Summary

Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 1 | Suggestions: 0

Key code review findings:

• ✗ src/Controls/src/Core/GradientBrush.cs:13 uses ReferenceEqualityComparer.Instance; CI reports CS0122/inaccessible for Controls.Core target builds.
• ⚠ src/Controls/src/Core/GradientBrush.cs:176,184 clears stop.Parent unconditionally, which can null the parent currently owned by another brush when a shared stop is removed from one brush.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #36746	Ref-count per-stop subscriptions in GradientBrush, handling Add/Remove/Replace/Move/Reset and using reference identity for dictionary keys	✅ PASSED (Gate) / ❌ current build-blocked by ReferenceEqualityComparer	GradientBrush.cs, LinearGradientBrushTests.cs	Original PR; gate was completed separately and must not be rerun

---

🔬 Code Review — Deep Analysis

Code Review — PR #36746

Independent Assessment

What this changes: Refactors GradientBrush to track GradientStop.PropertyChanged subscriptions with ref-counting, including Clear()/Reset.
Inferred motivation: Prevent cleared/shared gradient stops from keeping brushes subscribed/alive.

Reconciliation with PR Narrative

Author claims: Fixes GradientStops.Clear() leak and handles Add/Remove/Replace/Move/Reset.
Agreement/disagreement: The approach matches the claim, but the current implementation does not build.

Prior Review Reconciliation

Prior ❌ Error Finding	Source	Status	Evidence
_subscriptionRefCounts must use reference equality, not GradientStop.Equals.	MauiBot	✅ Fixed / regressed differently	Line 13 now uses ReferenceEqualityComparer.Instance, but that API is inaccessible for current targets.

Blast Radius Assessment

• Runs for all instances: Yes — all GradientBrush subscription management.
• Startup impact: No direct startup path.
• Static/shared state: No.

CI Status

• Required-check result: gh pr checks --required unavailable due unauthenticated gh; public check-runs show maui-pr failed.
• Classification: PR-caused failure ❌.
• Action taken: Public AzDO timeline for build 1524591 reports GradientBrush.cs(13,71): error CS0122.

Findings

❌ Error — ReferenceEqualityComparer breaks Controls.Core builds

src/Controls/src/Core/GradientBrush.cs:13

Controls.Core.csproj targets netstandard2.1;netstandard2.0;..., and CI reports:

GradientBrush.cs(13,71): error CS0122: 'ReferenceEqualityComparer' is inaccessible due to its protection level

Use a local comparer compatible with all target frameworks, e.g. ReferenceEquals plus RuntimeHelpers.GetHashCode.

Failure-Mode Probing

• netstandard build: fails at compile time on line 13.
• Clear()/Reset: intended leak fix appears sound once the comparer compiles.
• Duplicate shared stops: ref-counting requires reference identity; value equality would corrupt bookkeeping.
• Shared stop across brushes: clearing Parent unconditionally can disrupt the brush that most recently set the stop's parent.

Verdict: NEEDS_CHANGES

Confidence: low per CI-red calibration, though the build-blocking finding is high-confidence.
Summary: The bug fix direction is good, but current PR cannot build. No GitHub comments were posted.

---

🛠️ Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	maui-expert-reviewer / try-fix	Per-brush reference-identity occurrence ledger using List<GradientStop> and ReferenceEquals; unsubscribe distinct references on Reset and only clear Parent when this brush is current parent	✅ PASS	2 files	Avoids current PR's netstandard ReferenceEqualityComparer build blocker; adds duplicate/reference/shared-stop regression coverage
PR	PR #36746	Ref-count dictionary keyed by GradientStop using ReferenceEqualityComparer.Instance; handles Add/Remove/Replace/Move/Reset	✅ PASSED (Gate) / ❌ build-blocked in current review	2 files	Gate was completed separately; independent code review found CS0122 on ReferenceEqualityComparer

Cross-Pollination

Model	Round	New Ideas?	Details
maui-expert-reviewer	1	Yes	Candidate 1: per-brush identity occurrence ledger
maui-expert-reviewer	1	Yes	Candidate 2: move the Clear old-items gap into GradientStopCollection.ClearItems() via an internal clearing snapshot
maui-expert-reviewer	1	Yes	Candidate 3: weak subscription proxy plus reference reconciliation

Exhausted: No — stopped because Candidate #1 passed targeted regression tests and the netstandard build check, and is demonstrably better than the PR's current fix by removing the build blocker.
Selected Fix: Candidate #1 — Same bug coverage as the PR direction, no unavailable comparer dependency, explicit reference-identity semantics, and stronger regression coverage.

---

📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current description is accurate for the raw PR, but the winning fix should no longer describe the implementation as a dictionary/reference-count solution.

Recommended title

[Controls] GradientBrush: Fix Clear() leaks when stops are shared

Recommended description

### Issue Details
GradientBrush subscribes to each GradientStop's PropertyChanged event, but GradientStops.Clear() raises a collection Reset with e.OldItems == null. Because GradientBrush.OnGradientStopCollectionChanged only unsubscribed stops from e.OldItems, cleared stops remained subscribed and could keep old brushes alive.

### Description of Change

Updated GradientBrush subscription ownership so collection changes are handled consistently for Add, Remove, Replace, Move, and Reset.

The fix tracks GradientStop subscriptions per brush using reference identity and one ledger entry per collection occurrence. This keeps duplicate GradientStop instances subscribed until their last occurrence is removed, treats equal-but-distinct GradientStop objects independently, and avoids the unavailable ReferenceEqualityComparer.Instance dependency in netstandard Controls.Core builds.

For Reset/Clear, the brush now unsubscribes all previously tracked GradientStop instances before re-subscribing the collection's current contents. When detaching stops, it only clears stop.Parent if that stop is still parented to the current brush, so clearing one brush does not corrupt Parent state for a shared GradientStop currently owned by another brush.

Added regression tests covering:
- Clear() unsubscribing old stops from PropertyChanged.
- Duplicate GradientStop references remaining subscribed until the last occurrence is removed.
- Equal-but-distinct GradientStop instances being tracked by reference, not value equality.
- Shared GradientStop Parent state remaining intact when one brush is cleared.

### Issues Fixed

Fixes #36743

**Tested the behavior in the following platforms.**
- [x] Android
- [x] Windows
- [x] iOS
- [x] Mac

| Before  | After  |
|---------|--------|
| **Android**<br> <video src="https://github.com/user-attachments/assets/6c924225-c68a-44e6-8062-e705eaeb8402" width="600" height="300"> | **Android**<br> <video src="https://github.com/user-attachments/assets/28315b3b-05a6-4935-8f86-0316058986a4" width="600" height="300"> |

---

🏁 Report — Final Recommendation

Comparative Report — PR #36746

Candidates compared

Rank	Candidate	Result	Assessment
1	pr-plus-reviewer	✅ Passed targeted regression/build evidence via the same sandbox implementation as try-fix-1	Best choice. It preserves the PR's intended ownership model and incorporates the expert review fixes: no unavailable ReferenceEqualityComparer.Instance, explicit reference-identity tracking, duplicate-stop handling, and guarded Parent clearing for shared stops.
2	try-fix-1	✅ PASS	Technically equivalent to pr-plus-reviewer: a per-brush reference-identity occurrence ledger using ReferenceEquals, plus stronger duplicate/equal/shared-stop regression tests. Ranked below pr-plus-reviewer only because the requested PR path is to apply reviewer feedback to the PR fix rather than replace it with a separately named alternative.
3	pr	❌ Build-blocked despite gate passing	The raw PR fix addresses the Clear/Reset leak conceptually, but it uses ReferenceEqualityComparer.Instance, which fails Controls.Core netstandard builds with CS0122. It also clears shared stops' Parent unconditionally.

Regression-test rule

No STEP 5a candidate with failing regression tests ranks above a passing candidate. try-fix-1 passed its targeted LinearGradientBrushTests run and the netstandard Controls.Core build. The raw PR gate passed for the targeted behavior, but the raw PR is still ranked lower because it has a build-blocking compile failure.

Winning candidate

Winner: pr-plus-reviewer

pr-plus-reviewer wins because it keeps the PR's correct high-level approach while applying the expert review fixes proven by the STEP 5a sandbox candidate. It fixes the original GradientStops.Clear()/Reset leak, avoids the netstandard build blocker, preserves reference-identity semantics for duplicate/equal stops, and avoids corrupting Parent state when a GradientStop is shared by multiple brushes.

---

🧭 Next Steps — review latest findings

No alternative fix was selected for this run. Review the session findings and CI results before merging.

[MauiBot]

Expert Review — 2 findings

See inline comments for details.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[major] Build & MSBuild — ReferenceEqualityComparer is not public for the netstandard2.0/netstandard2.1 targets that Controls.Core.csproj builds. A targeted dotnet build src/Controls/src/Core/Controls.Core.csproj -f netstandard2.0 --no-restore fails here with CS0122, so the package cannot build until this uses a netstandard-compatible reference comparer or explicit ReferenceEquals/identity-hash bookkeeping.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — Clear()/Reset now nulls every tracked stop's Parent even if another brush currently owns the same shared GradientStop. Repro: add one stop to brush1, then brush2, then call brush1.GradientStops.Clear(); this Reset path clears stop.Parent from brush2 to null, breaking inherited context/parent state for brush2. Only clear the parent when ReferenceEquals(stop.Parent, this).