[leak-fix] Fix Border.StrokeDashArray memory leak (Fixes #35492)

[kubaflo]

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!

Note

🤖 AI-generated PR. This fix and its regression test were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging.

Fixes #35492

The leak

When multiple Border controls use the same DoubleCollection (e.g. from Application.Resources) for StrokeDashArray, the StrokeDashPattern getter subscribes to that collection with a plain, non-weak delegate — collection.CollectionChanged += OnStrokeDashArrayChanged (src/Controls/src/Core/Border/Border.cs) — and only detaches when StrokeDashArray is reassigned, never on unload. Because the app-level DoubleCollection is long-lived, it retains every realized dashed Border (and, through normal page-level handlers, can retain the page, its CollectionView, and view models). This is the ~180 MB-over-20-navigations scenario described in #35492.

The fix

Subscribe to the StrokeDashArray collection through a WeakNotifyCollectionChangedProxy, torn down in the existing ~Border() finalizer alongside the already-present _strokeProxy / _strokeShapeProxy — i.e. this fix simply extends Border's own established weak-proxy pattern to the third collection it listens to. A shared DoubleCollection no longer strongly roots the Border.

Regression test

BorderUnitTests.SharedStrokeDashArrayDoesNotLeakBorder in src/Controls/tests/Core.UnitTests/BorderUnitTests.cs: assigns one shared DoubleCollection to a Border, drops the border, forces GC, asserts it is collected while the shared collection stays alive.

Verified locally against this PR's base (main), Controls.Core.UnitTests:

State	Result
Without fix (current main)	❌ FAILS — Border should not be alive!
With fix	✅ Passed SharedStrokeDashArrayDoesNotLeakBorder

Scope

Managed cross-platform change → all platforms. No public-API change (the ~Border() finalizer already existed).

[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 -- 36254

Or

• Run remotely in PowerShell:

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

[MauiBot]

Expert Review — 1 findings

See inline comments for details.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Performance-Critical Path Optimization -- Avoid re-subscribing the weak collection proxy on every StrokeDashPattern read when the dash collection has not changed. WeakNotifyCollectionChangedProxy.Subscribe always detaches/re-attaches the event and replaces its source/handler WeakReferences, so each platform mapper read (including the read triggered by OnStrokeDashArrayChanged after every collection mutation) allocates additional weak references and churns event subscriptions. Guard with the current proxy source (for example, only call Subscribe when it is not already subscribed to newCollection) while still unsubscribing when the collection changes or becomes null.

[Copilot]

Pull request overview

Fixes a managed memory leak in Border.StrokeDashArray by switching the collection subscription used by StrokeDashPattern to a weak-event proxy, preventing shared/long-lived DoubleCollection instances (e.g., from Application.Resources) from strongly rooting Border instances.

Changes:

• Use WeakNotifyCollectionChangedProxy to subscribe to StrokeDashArray.CollectionChanged from StrokeDashPattern, and unsubscribe via the existing ~Border() finalizer.
• Add a unit test that verifies a Border can be collected even when its StrokeDashArray references a shared, long-lived DoubleCollection.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Controls/src/Core/Border/Border.cs	Replaces the strong CollectionChanged subscription with WeakNotifyCollectionChangedProxy and ensures teardown in ~Border().
src/Controls/tests/Core.UnitTests/BorderUnitTests.cs	Adds a regression test ensuring shared StrokeDashArray resources don’t keep Border alive.

[MauiBot]

AI Review Summary

@kubaflo — new AI review results are available based on this last commit: b53d3e2. To request a fresh review after new comments or commits, comment /review rerun.

---

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ SKIPPED

Gate skipped (SkipUITests fast test mode). No UI/device tests were run for this pipeline invocation.

---

📋 Pre-Flight — Context & Validation

Issue: #35492 - Border.StrokeDashArray leaks dashed Borders when using a shared Application resource
PR: #36254 - [leak-fix] Fix Border.StrokeDashArray memory leak (Fixes #35492)
Platforms Affected: Android, iOS, macOS/Mac Catalyst; managed Controls-layer fix likely affects all platforms that render Border.StrokeDashArray.
Files Changed: 1 implementation, 1 test

Key Findings

• The issue reports that a long-lived/shared DoubleCollection from application resources retains dashed Border instances because StrokeDashPattern subscribes to CollectionChanged with a strong instance-method delegate and does not detach when the Border is discarded.
• The PR changes Border.StrokeDashPattern to subscribe through WeakNotifyCollectionChangedProxy and adds a unit test proving a Border can be collected while the shared dash array remains alive.
• Prior inline review noted a non-blocking performance concern: the current PR calls WeakNotifyCollectionChangedProxy.Subscribe on every StrokeDashPattern getter read, causing avoidable detach/attach and weak-reference churn when the collection has not changed.
• GitHub CLI is unauthenticated in this environment, so PR/issue context was gathered from local git state and the public GitHub API; required-check status is undetermined.

Code Review Summary

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

Key code review findings:

• ℹ src/Controls/src/Core/Border/Border.cs:321-325 — Avoid repeated same-source weak-proxy resubscribe churn; this is not a correctness blocker but may be improved by guarding unchanged source collections.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #36254	Replace strong CollectionChanged subscription with WeakNotifyCollectionChangedProxy, cleaned up by ~Border().	⚠️ SKIPPED (Gate previously skipped — no tests detected by gate script)	src/Controls/src/Core/Border/Border.cs, src/Controls/tests/Core.UnitTests/BorderUnitTests.cs	Original PR; unit test exists in PR but gate result supplied to this run says no tests detected.

---

🔬 Code Review — Deep Analysis

Code Review — PR #36254

Independent Assessment

What this changes: Border.StrokeDashPattern now subscribes to StrokeDashArray.CollectionChanged through WeakNotifyCollectionChangedProxy, and unsubscribes in ~Border(). A unit test verifies a shared DoubleCollection no longer keeps a Border alive.
Inferred motivation: Prevent long-lived/shared dash arrays from strongly retaining discarded dashed Border instances.

Reconciliation with PR Narrative

Author claims: Fixes #35492 memory leak for shared Application.Resources DoubleCollection; no public API change; adds regression test.
Agreement/disagreement: Agrees with code and issue. The weak proxy breaks the strong event chain from shared collection → border instance.

Prior Review Reconciliation

No prior ❌ Error findings found.

Prior non-error finding considered:

• MauiBot inline [moderate] at src/Controls/src/Core/Border/Border.cs:325: repeated same-source weak-proxy resubscription may cause avoidable event/WeakReference churn. Still applicable, but not a correctness blocker.

Blast Radius Assessment

• Runs for all instances: No — only Border instances whose dash pattern is read/rendered.
• Startup impact: No.
• Static/shared state: No new static state.
• Handler/platform impact: Shared Controls-layer behavior consumed by all platform border handlers.

CI Status

• Required-check result: undetermined
• Classification: tool-unavailable — gh pr checks --required failed because gh is unauthenticated.
• Action taken: Public API used for PR/review context where feasible; CI confidence capped low.

Findings

💡 Suggestion — Avoid repeated same-source resubscribe churn

src/Controls/src/Core/Border/Border.cs:321-325

Every StrokeDashPattern getter call invokes WeakNotifyCollectionChangedProxy.Subscribe, which detaches/re-attaches even when the collection is unchanged and allocates fresh weak references. This preserves correctness and is similar in spirit to previous getter-time subscription behavior, but a same-source guard could reduce churn in redraw/update paths.

Failure-Mode Probing

• Shared long-lived dash array: Source event holds the proxy, not the Border; proxy only weakly references the handler, so the border can collect.
• Dash array replaced/null: Next mapper read of StrokeDashPattern re-subscribes to the new collection or unsubscribes when null.
• Collection mutates while border alive: Proxy forwards to OnStrokeDashArrayChanged, updating IBorderStroke.StrokeDashPattern.
• Border collected before collection mutates: Finalizer unsubscribes; if not yet finalized, proxy drops itself on next event when weak handler is gone.

Verdict: NEEDS_DISCUSSION

Confidence: low
Summary: Code approach looks sound and no blocking code issue was found, including by the expert reviewer. However, required CI status could not be determined because gh is unauthenticated, so per the workflow this cannot be marked LGTM.

---

🛠️ Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	try-fix-1	Remove StrokeDashArray.CollectionChanged subscription entirely and make StrokeDashPattern a pure conversion.	❌ Fail	2 files	Leak probe passed, but collection mutation no longer updated handler dash pattern.
2	try-fix-2	Move weak subscription ownership to StrokeDashArrayProperty changing/changed callbacks; keep getter side-effect free.	❌ Fail	2 files	Explicit collection mutation worked, but default StrokeDashArray mutations were missed because defaultValueCreator does not trigger the property-changed subscription path.
3	try-fix-3	Keep PR lazy weak subscription, but make WeakNotifyCollectionChangedProxy.Subscribe idempotent for same source/same handler.	✅ Pass	4 files	Preserves leak fix and mutation behavior while addressing repeated getter-time resubscribe churn.
PR	PR #36254	Use WeakNotifyCollectionChangedProxy from Border.StrokeDashPattern, cleaned up by ~Border().	⚠️ SKIPPED (Gate)	2 files	Original PR; gate result supplied to this run said no tests detected.

Cross-Pollination

Model	Round	New Ideas?	Details
gpt-5.5 / code-review + MAUI expert loop	1	Yes	Remove subscription entirely to eliminate retention edge; failed mutation semantics.
gpt-5.5 / code-review + MAUI expert loop	2	Yes	Move subscription to bindable-property lifecycle; failed default collection semantics.
gpt-5.5 / MAUI expert review	3	Yes	Make the weak proxy itself idempotent for repeated same-source/same-handler subscriptions; passed focused tests and expert review.

Exhausted: No — stopped after candidate 3 because it passed all focused regression probes and is demonstrably better than the PR's current fix by addressing the prior resubscribe-churn concern while preserving the PR's leak fix.
Selected Fix: Candidate #3 — Keep the PR fix and add idempotency to WeakNotifyCollectionChangedProxy.Subscribe.

---

📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current description accurately covers the raw PR leak fix, but the winning try-fix-3 candidate also changes WeakNotifyCollectionChangedProxy.Subscribe and adds mutation/idempotency regression coverage.

Recommended title

[All] Border: Fix StrokeDashArray memory leak

Recommended description

> [!NOTE]
> 🤖 **AI-generated PR.** This fix and its regression test were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging.

Fixes #35492

### The leak

When multiple `Border` controls use the same `DoubleCollection` (e.g. from `Application.Resources`) for `StrokeDashArray`, the `StrokeDashPattern` getter subscribed to that collection with a plain, non-weak delegate — `collection.CollectionChanged += OnStrokeDashArrayChanged` (`src/Controls/src/Core/Border/Border.cs`) — and only detached when `StrokeDashArray` was read again/reassigned, never on unload. Because the app-level `DoubleCollection` is long-lived, it retains every realized dashed `Border` (and, through normal page-level handlers, can retain the page, its `CollectionView`, and view models). This is the ~180 MB-over-20-navigations scenario described in #35492.

### The fix

Subscribe to the `StrokeDashArray` collection through a `WeakNotifyCollectionChangedProxy`, torn down in the existing `~Border()` finalizer alongside the already-present `_strokeProxy` / `_strokeShapeProxy` — i.e. this fix extends Border's own established weak-proxy pattern to the third collection it listens to. A shared `DoubleCollection` no longer strongly roots the `Border`.

The final fix also makes `WeakNotifyCollectionChangedProxy.Subscribe` idempotent for repeated subscriptions to the same source and handler. This preserves the lazy `StrokeDashPattern` subscription behavior needed for both explicit and default `StrokeDashArray` collections, while avoiding repeated unsubscribe/subscribe churn when platform render paths read `StrokeDashPattern` multiple times.

### Regression tests

- `BorderUnitTests.SharedStrokeDashArrayDoesNotLeakBorder`: assigns one shared `DoubleCollection` to a `Border`, drops the border, forces GC, and asserts it is collected while the shared collection stays alive.
- `BorderUnitTests.StrokeDashArrayMutationUpdatesHandlerStrokeDashPattern`: verifies mutating an explicitly assigned dash array still updates the handler dash pattern.
- `BorderUnitTests.DefaultStrokeDashArrayMutationUpdatesHandlerStrokeDashPattern`: verifies mutating the default `StrokeDashArray` created by `defaultValueCreator` still updates the handler dash pattern.
- `WeakEventProxyTests.SubscribeSameSourceAndHandlerIsIdempotent`: verifies repeated same-source/same-handler weak-proxy subscriptions do not detach/re-attach.

**Verified locally against this PR's base (`main`), `Controls.Core.UnitTests`:**

| State | Result |
|---|---|
| Without fix (current `main`) | ❌ FAILS — `Border should not be alive!` |
| With fix | ✅ `Passed SharedStrokeDashArrayDoesNotLeakBorder` |

### Scope

Managed cross-platform Controls change → all platforms. No public-API change.

---

🏁 Report — Final Recommendation

Comparative Report — PR #36254

Candidate ranking

Rank	Candidate	Regression result	Assessment
1	try-fix-3	✅ Pass	Best overall. It keeps the PR's weak-subscription leak fix, preserves collection-mutation update behavior including the default StrokeDashArray, and eliminates repeated same-source/same-handler weak-proxy resubscribe churn.
2	pr	⚠️ Gate skipped	Correctly fixes the strong-reference leak and includes a targeted leak regression test, but leaves avoidable repeated unsubscribe/subscribe churn on every StrokeDashPattern read.
3	pr-plus-reviewer	⚠️ Gate skipped	Equivalent to pr because the expert reviewer produced no actionable inline findings to apply.
4	try-fix-2	❌ Fail	Moving subscription ownership to bindable-property callbacks is cleaner in principle, but it misses mutations to the default StrokeDashArray created by defaultValueCreator, so live borders can render stale dash patterns.
5	try-fix-1	❌ Fail	Removing the collection subscription avoids the leak, but it regresses existing behavior because mutating an existing dash collection no longer updates the handler.

Winning candidate

Winner: try-fix-3

try-fix-3 is the strongest candidate because it passes the focused regression probes while also improving on the PR's remaining inefficiency. The raw PR fix is sound for the memory leak, but the winning candidate is more complete: it preserves the lazy subscription needed for default collection semantics and makes repeated reads of StrokeDashPattern idempotent at the weak-proxy layer. The failed regression candidates are ranked below both PR-based candidates as required.

Notes for integration

try-fix-3 is not the raw PR fix; it should be applied as an incremental improvement over PR #36254. Its required diff includes the PR's Border weak proxy change and leak test, plus an idempotency guard in WeakNotifyCollectionChangedProxy.Subscribe and focused tests for explicit/default dash-array mutation and proxy idempotency.

---

🧭 Next Steps — alternative fix proposed (try-fix-3)

Automated review — alternative fix proposed

The expert-reviewer evaluation compared the PR fix against automatically generated candidates and selected try-fix-3 as the strongest fix.

Why: try-fix-3 preserves the PR's weak-subscription leak fix, keeps collection mutation updates working for explicit and default StrokeDashArray values, and removes repeated same-source/same-handler weak-proxy resubscribe churn. The other try-fix candidates failed regression probes, while the raw PR/pr-plus-reviewer candidates did not address the churn concern.

Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.

Candidate diff (try-fix-3)

diff --git a/src/Controls/src/Core/Border/Border.cs b/src/Controls/src/Core/Border/Border.cs
index eef7d2615a..1da68b4b23 100644
--- a/src/Controls/src/Core/Border/Border.cs
+++ b/src/Controls/src/Core/Border/Border.cs
@@ -25,11 +25,14 @@ namespace Microsoft.Maui.Controls
 		PropertyChangedEventHandler? _strokeShapeChanged;
 		WeakNotifyPropertyChangedProxy? _strokeProxy = null;
 		PropertyChangedEventHandler? _strokeChanged;
+		WeakNotifyCollectionChangedProxy? _strokeDashArrayProxy = null;
+		NotifyCollectionChangedEventHandler? _strokeDashArrayChanged;
 
 		~Border()
 		{
 			_strokeShapeProxy?.Unsubscribe();
 			_strokeProxy?.Unsubscribe();
+			_strokeDashArrayProxy?.Unsubscribe();
 		}
 
 		/// <summary>Bindable property for <see cref="Content"/>.</summary>
@@ -310,13 +313,21 @@ namespace Microsoft.Maui.Controls
 		{
 			get
 			{
-				if (StrokeDashArray is INotifyCollectionChanged oldCollection)
-					oldCollection.CollectionChanged -= OnStrokeDashArrayChanged;
-
 				_strokeDashPattern = StrokeDashArray?.ToFloatArray();
 
+				// Subscribe weakly so a shared/long-lived DoubleCollection does not strongly
+				// root this Border for the collection's lifetime. The proxy is torn down in
+				// ~Border(), mirroring the Stroke/StrokeShape weak proxies above.
 				if (StrokeDashArray is INotifyCollectionChanged newCollection)
-					newCollection.CollectionChanged += OnStrokeDashArrayChanged;
+				{
+					_strokeDashArrayChanged ??= OnStrokeDashArrayChanged;
+					_strokeDashArrayProxy ??= new();
+					_strokeDashArrayProxy.Subscribe(newCollection, _strokeDashArrayChanged);
+				}
+				else
+				{
+					_strokeDashArrayProxy?.Unsubscribe();
+				}
 
 				return _strokeDashPattern;
 			}
diff --git a/src/Controls/src/Core/Internals/WeakEventProxy.cs b/src/Controls/src/Core/Internals/WeakEventProxy.cs
index 6f0f1b0af8..9cc0612376 100644
--- a/src/Controls/src/Core/Internals/WeakEventProxy.cs
+++ b/src/Controls/src/Core/Internals/WeakEventProxy.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Collections.Specialized;
 using System.ComponentModel;
 
@@ -87,6 +88,13 @@ namespace Microsoft.Maui.Controls
 		{
 			if (TryGetSource(out var s))
 			{
+				if (ReferenceEquals(s, source) &&
+					TryGetHandler(out var existingHandler) &&
+					EqualityComparer<NotifyCollectionChangedEventHandler>.Default.Equals(existingHandler, handler))
+				{
+					return;
+				}
+
 				s.CollectionChanged -= OnCollectionChanged;
 			}
 
diff --git a/src/Controls/tests/Core.UnitTests/BorderUnitTests.cs b/src/Controls/tests/Core.UnitTests/BorderUnitTests.cs
index e72381752a..bc15bc5741 100644
--- a/src/Controls/tests/Core.UnitTests/BorderUnitTests.cs
+++ b/src/Controls/tests/Core.UnitTests/BorderUnitTests.cs
@@ -1,4 +1,8 @@
 #nullable enable
+using System;
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Microsoft.Maui.Handlers;
 using Xunit;
 
 namespace Microsoft.Maui.Controls.Core.UnitTests
@@ -44,5 +48,81 @@ namespace Microsoft.Maui.Controls.Core.UnitTests
 			border.Content = null;
 			Assert.Null(label.Parent);
 		}
+
+		[Fact]
+		public async Task SharedStrokeDashArrayDoesNotLeakBorder()
+		{
+			// A shared / long-lived DoubleCollection, exactly as the issue describes
+			// (e.g. a static field or a value reused from a ResourceDictionary).
+			var sharedDashArray = new DoubleCollection { 2, 2 };
+
+			WeakReference weakBorder;
+			{
+				var border = new Border { StrokeDashArray = sharedDashArray };
+
+				// Reading StrokeDashPattern installs the CollectionChanged subscription,
+				// exactly as the platform border handler does when rendering the dash.
+				_ = border.StrokeDashPattern;
+
+				weakBorder = new WeakReference(border);
+				// Drop the only strong reference to `border`; `sharedDashArray` stays alive.
+			}
+
+			Assert.False(await weakBorder.WaitForCollect(), "Border should not be alive!");
+			GC.KeepAlive(sharedDashArray);
+		}
+
+		[Fact]
+		public void StrokeDashArrayMutationUpdatesHandlerStrokeDashPattern()
+		{
+			var border = new Border { StrokeDashArray = new DoubleCollection { 2, 2 } };
+			var handler = new BorderHandlerStub();
+			border.Handler = handler;
+
+			_ = border.StrokeDashPattern;
+
+			var strokeDashArray = border.StrokeDashArray;
+			Assert.NotNull(strokeDashArray);
+			strokeDashArray.Add(4);
+
+			Assert.Contains("StrokeDashPattern", handler.UpdatedProperties);
+		}
+
+		[Fact]
+		public void DefaultStrokeDashArrayMutationUpdatesHandlerStrokeDashPattern()
+		{
+			var border = new Border();
+			var handler = new BorderHandlerStub();
+			border.Handler = handler;
+
+			_ = border.StrokeDashPattern;
+
+			var strokeDashArray = border.StrokeDashArray;
+			Assert.NotNull(strokeDashArray);
+			strokeDashArray.Add(4);
+
+			Assert.Contains("StrokeDashPattern", handler.UpdatedProperties);
+		}
+
+		class BorderHandlerStub : ViewHandler<Border, object>
+		{
+			public List<string> UpdatedProperties { get; } = new();
+
+			public BorderHandlerStub()
+				: base(new PropertyMapper<IView>())
+			{
+			}
+
+			public override void UpdateValue(string property)
+			{
+				UpdatedProperties.Add(property);
+				base.UpdateValue(property);
+			}
+
+			protected override object CreatePlatformView()
+			{
+				return new object();
+			}
+		}
 	}
 }
\ No newline at end of file
diff --git a/src/Controls/tests/Core.UnitTests/WeakEventProxyTests.cs b/src/Controls/tests/Core.UnitTests/WeakEventProxyTests.cs
index c14e846af7..ab0a7bf911 100644
--- a/src/Controls/tests/Core.UnitTests/WeakEventProxyTests.cs
+++ b/src/Controls/tests/Core.UnitTests/WeakEventProxyTests.cs
@@ -44,9 +44,46 @@ namespace Microsoft.Maui.Controls.Core.UnitTests
 			Assert.True(fired);
 		}
 
+		[Fact]
+		public void SubscribeSameSourceAndHandlerIsIdempotent()
+		{
+			var source = new CollectionChangedSource();
+			var proxy = new WeakNotifyCollectionChangedProxy();
+			NotifyCollectionChangedEventHandler handler = (_, _) => { };
+
+			proxy.Subscribe(source, handler);
+			proxy.Subscribe(source, handler);
+
+			Assert.Equal(1, source.AddCount);
+			Assert.Equal(0, source.RemoveCount);
+			GC.KeepAlive(handler);
+		}
+
 		class Subscriber
 		{
 			public void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) { }
 		}
+
+		class CollectionChangedSource : INotifyCollectionChanged
+		{
+			NotifyCollectionChangedEventHandler _collectionChanged;
+
+			public int AddCount { get; private set; }
+			public int RemoveCount { get; private set; }
+
+			public event NotifyCollectionChangedEventHandler CollectionChanged
+			{
+				add
+				{
+					AddCount++;
+					_collectionChanged += value;
+				}
+				remove
+				{
+					RemoveCount++;
+					_collectionChanged -= value;
+				}
+			}
+		}
 	}
 }