Fix Border.StrokeDashArray leaks dashed Borders when using a shared Application resource#35544
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35544Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35544" |
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/review -b feature/regression-check -p ios |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please verify the test failures?
@kubaflo , I've addressed the test failures and modified the test. |
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
| (bindable as Border)?.NotifyStrokeDashArrayChanges(); | ||
| }); | ||
|
|
||
| void NotifyStrokeDashArrayChanges() |
There was a problem hiding this comment.
[major] Behavior Regression — Subscribing to StrokeDashArray only from the bindable property changed callback misses the default collection created by defaultValueCreator; MAUI does not raise propertyChanged for that initial default. Scenario: var border = new Border(); is connected/mapped, then border.StrokeDashArray.Add(4); mutates the default DoubleCollection. After this PR the collection was never subscribed, so OnStrokeDashArrayChanged does not call Handler.UpdateValue(nameof(IBorderStroke.StrokeDashPattern)) and Android keeps rendering the stale dash pattern. Preserve the previous getter-driven subscription (using a weak proxy) or explicitly subscribe when StrokeDashPattern is read.
| App.TapBackArrow(); | ||
| App.WaitForElement("PushPageButton"); | ||
| App.Tap("ForceGCButton"); | ||
| Thread.Sleep(3000); // Wait a moment for the GC to do its work |
There was a problem hiding this comment.
[moderate] UI Test Reliability — The fixed Thread.Sleep(3000) makes the Android leak test both slower and timing-dependent; slower CI devices can still be finalizing/collecting when the assertion runs. Prefer the existing UITest retry helper, e.g. App.RetryAssert(() => Assert.That(App.FindElement("SummaryLabel").GetText(), Is.EqualTo("Alive count: 0/1")), timeout: TimeSpan.FromSeconds(10));, so the test waits for the observable condition instead of an arbitrary delay.
| PropertyChangedEventHandler? _strokeShapeChanged; | ||
| WeakNotifyPropertyChangedProxy? _strokeProxy = null; | ||
| PropertyChangedEventHandler? _strokeChanged; | ||
| readonly WeakNotifyPropertyChangedProxy _strokeShapeProxy = new(); |
There was a problem hiding this comment.
[moderate] Performance-Critical Path — This changes Border from lazy proxy/delegate allocation to eagerly allocating property and collection proxies/delegates for every instance, even Borders that never use mutable Stroke/StrokeShape/StrokeDashArray. In template-heavy Android scenarios (the repro itself realizes many Borders in a CollectionView), that is avoidable per-item allocation. Keep the weak dash-array fix but allocate the proxies/delegates lazily as the previous code did.
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
| public static readonly BindableProperty StrokeDashArrayProperty = | ||
| BindableProperty.Create(nameof(StrokeDashArray), typeof(DoubleCollection), typeof(Border), null, | ||
| defaultValueCreator: bindable => new DoubleCollection()); | ||
| defaultValueCreator: bindable => new DoubleCollection(), |
There was a problem hiding this comment.
[major] Handler Mapper and Property Patterns — The default StrokeDashArray created by defaultValueCreator is never subscribed because propertyChanged only runs when the property is explicitly set. A border using the default collection (border.StrokeDashArray.Add(...)) will no longer call Handler.UpdateValue(nameof(IBorderStroke.StrokeDashPattern)), regressing the previous getter-time subscription behavior. Ensure the default collection is subscribed when it is created/accessed, while still using the weak proxy.
| PropertyChangedEventHandler? _strokeShapeChanged; | ||
| WeakNotifyPropertyChangedProxy? _strokeProxy = null; | ||
| PropertyChangedEventHandler? _strokeChanged; | ||
| readonly WeakNotifyPropertyChangedProxy _strokeShapeProxy = new(); |
There was a problem hiding this comment.
[moderate] Performance-Critical Path Optimization — This changes the existing lazy proxy/delegate allocation pattern so every Border now allocates stroke/stroke-shape/dash weak proxies and delegates, even when those properties are never observed. Border is commonly used in item templates, so this adds avoidable per-item overhead. Keep the existing lazy initialization pattern and add the dash-array weak proxy lazily when subscribing.
| App.TapBackArrow(); | ||
| App.WaitForElement("PushPageButton"); | ||
| App.Tap("ForceGCButton"); | ||
| Thread.Sleep(3000); // Wait a moment for the GC to do its work |
There was a problem hiding this comment.
[moderate] Regression Prevention and Test Coverage — The Android leak test relies on a fixed Thread.Sleep(3000) after tapping Force GC, which makes the result timing-dependent. Replace this with a polling/retry wait for SummaryLabel to become Alive count: 0/1 so the test waits for the UI update deterministically instead of sleeping.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
@kubaflo , Based on AI suggestion, I have modified the fix |
|
/review -b feature/refactor-copilot-yml -p ios |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| App.TapBackArrow(); | ||
| App.WaitForElement("PushPageButton"); | ||
| App.Tap("ForceGCButton"); | ||
| Thread.Sleep(3000); // Wait a moment for the GC to do its work |
There was a problem hiding this comment.
[minor] Regression Prevention — non-blocking test reliability — This fixed Thread.Sleep(3000) does not make the assertion deterministic: the app only updates SummaryLabel inside the Force GC button handler, so sleeping afterward either waits unnecessarily or still flakes if the UI command/result is delayed. Prefer waiting for the expected UI state, for example by retrying the SummaryLabel assertion until it reports Alive count: 0/1.
🤖 AI Summary
📊 Review Session —
|
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🖥️ Issue35492 Issue35492 |
✅ FAIL — 128s | ✅ PASS — 114s |
🔴 Without fix — 🖥️ Issue35492: FAIL ✅ · 128s
Determining projects to restore...
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/BindingSourceGen/Controls.BindingSourceGen.csproj (in 381 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 404 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Essentials/src/Essentials.csproj (in 404 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 444 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Core/src/Core.csproj (in 415 ms).
6 of 11 projects are up-to-date for restore.
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Graphics -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Graphics/Debug/net10.0-ios26.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Essentials -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Essentials/Debug/net10.0-ios26.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Core/Debug/net10.0-ios26.0/Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Maps.dll
Controls.BindingSourceGen -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Controls.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Core/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Controls.Foldable -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Foldable/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Foldable.dll
Microsoft.AspNetCore.Components.WebView.Maui -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Microsoft.AspNetCore.Components.WebView.Maui/Debug/net10.0-ios26.0/Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.Xaml -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Xaml.dll
Controls.Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Maps.dll
Detected signing identity:
Code Signing Key: "" (-)
Provisioning Profile: "" () - no entitlements
Bundle Id: com.microsoft.maui.uitests
App Id: com.microsoft.maui.uitests
Controls.TestCases.HostApp -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-ios/iossimulator-arm64/Controls.TestCases.HostApp.dll
Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
Optimizing assemblies for size. This process might take a while.
Build succeeded.
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
1 Warning(s)
0 Error(s)
Time Elapsed 00:01:02.14
Determining projects to restore...
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/BindingSourceGen/Controls.BindingSourceGen.csproj (in 390 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Essentials/src/Essentials.csproj (in 406 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 418 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 439 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Core/src/Core.csproj (in 455 ms).
8 of 13 projects are up-to-date for restore.
Controls.CustomAttributes -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.CustomAttributes/Debug/net10.0/Controls.CustomAttributes.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Graphics -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Essentials -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Controls.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
VisualTestUtils -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/VisualTestUtils/Debug/netstandard2.0/VisualTestUtils.dll
UITest.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.Core/Debug/net10.0/UITest.Core.dll
UITest.NUnit -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.NUnit/Debug/net10.0/UITest.NUnit.dll
VisualTestUtils.MagickNet -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/VisualTestUtils.MagickNet/Debug/netstandard2.0/VisualTestUtils.MagickNet.dll
UITest.Appium -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.Appium/Debug/net10.0/UITest.Appium.dll
UITest.Analyzers -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.Analyzers/Debug/netstandard2.0/UITest.Analyzers.dll
Controls.TestCases.iOS.Tests -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.iOS.Tests/Debug/net10.0/Controls.TestCases.iOS.Tests.dll
Test run for /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.iOS.Tests/Debug/net10.0/Controls.TestCases.iOS.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (arm64)
Starting test execution, please wait...
A total of 1 test files 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.06] Discovering: Controls.TestCases.iOS.Tests
[xUnit.net 00:00:00.18] Discovered: Controls.TestCases.iOS.Tests
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.iOS.Tests/Debug/net10.0/Controls.TestCases.iOS.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 5/26/2026 2:13:10 PM FixtureSetup for Issue35492(iOS)
>>>>> 5/26/2026 2:13:15 PM SharedDashArray_PushedPagesAreCollectedAfterForceGC Start
>>>>> 5/26/2026 2:13:20 PM SharedDashArray_PushedPagesAreCollectedAfterForceGC Stop
>>>>> 5/26/2026 2:13:20 PM Log types: syslog, crashlog, performance, safariConsole, safariNetwork, server
Failed SharedDashArray_PushedPagesAreCollectedAfterForceGC [6 s]
Error Message:
Expected all pushed pages to be collectable after forcing GC.
Assert.That(summaryText, Is.EqualTo($"Alive count: 0/1"))
String lengths are both 16. Strings differ at index 13.
Expected: "Alive count: 0/1"
But was: "Alive count: 1/1"
------------------------^
Stack Trace:
at Microsoft.Maui.TestCases.Tests.Issues.Issue35492.SharedDashArray_PushedPagesAreCollectedAfterForceGC() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35492.cs:line 25
1) at Microsoft.Maui.TestCases.Tests.Issues.Issue35492.SharedDashArray_PushedPagesAreCollectedAfterForceGC() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35492.cs:line 25
NUnit Adapter 4.5.0.0: Test execution complete
Results File: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue35492.trx
Test Run Failed.
Total tests: 1
Failed: 1
Total time: 28.4636 Seconds
>>> TRX_RESULT_FILE: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue35492.trx
🟢 With fix — 🖥️ Issue35492: PASS ✅ · 114s
Determining projects to restore...
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/BindingSourceGen/Controls.BindingSourceGen.csproj (in 333 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 401 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Essentials/src/Essentials.csproj (in 372 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 447 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Core/src/Core.csproj (in 463 ms).
6 of 11 projects are up-to-date for restore.
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Graphics -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Graphics/Debug/net10.0-ios26.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Essentials -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Essentials/Debug/net10.0-ios26.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Core/Debug/net10.0-ios26.0/Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Maps.dll
Controls.BindingSourceGen -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Controls.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Core/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Microsoft.AspNetCore.Components.WebView.Maui -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Microsoft.AspNetCore.Components.WebView.Maui/Debug/net10.0-ios26.0/Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Maps.dll
Controls.Xaml -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Xaml.dll
Controls.Foldable -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Foldable/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Foldable.dll
Detected signing identity:
Code Signing Key: "" (-)
Provisioning Profile: "" () - no entitlements
Bundle Id: com.microsoft.maui.uitests
App Id: com.microsoft.maui.uitests
Controls.TestCases.HostApp -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-ios/iossimulator-arm64/Controls.TestCases.HostApp.dll
Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
Optimizing assemblies for size. This process might take a while.
Build succeeded.
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
1 Warning(s)
0 Error(s)
Time Elapsed 00:00:52.75
Determining projects to restore...
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/BindingSourceGen/Controls.BindingSourceGen.csproj (in 3.53 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 3.54 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Essentials/src/Essentials.csproj (in 3.55 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 3.52 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Core/src/Core.csproj (in 3.58 sec).
8 of 13 projects are up-to-date for restore.
Controls.CustomAttributes -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.CustomAttributes/Debug/net10.0/Controls.CustomAttributes.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Graphics -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Essentials -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14198602
Controls.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
VisualTestUtils -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/VisualTestUtils/Debug/netstandard2.0/VisualTestUtils.dll
UITest.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.Core/Debug/net10.0/UITest.Core.dll
VisualTestUtils.MagickNet -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/VisualTestUtils.MagickNet/Debug/netstandard2.0/VisualTestUtils.MagickNet.dll
UITest.NUnit -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.NUnit/Debug/net10.0/UITest.NUnit.dll
UITest.Appium -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.Appium/Debug/net10.0/UITest.Appium.dll
UITest.Analyzers -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/UITest.Analyzers/Debug/netstandard2.0/UITest.Analyzers.dll
Controls.TestCases.iOS.Tests -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.iOS.Tests/Debug/net10.0/Controls.TestCases.iOS.Tests.dll
Test run for /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.iOS.Tests/Debug/net10.0/Controls.TestCases.iOS.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (arm64)
Starting test execution, please wait...
A total of 1 test files 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.06] Discovering: Controls.TestCases.iOS.Tests
[xUnit.net 00:00:00.17] Discovered: Controls.TestCases.iOS.Tests
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.iOS.Tests/Debug/net10.0/Controls.TestCases.iOS.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 5/26/2026 2:15:05 PM FixtureSetup for Issue35492(iOS)
>>>>> 5/26/2026 2:15:09 PM SharedDashArray_PushedPagesAreCollectedAfterForceGC Start
>>>>> 5/26/2026 2:15:15 PM SharedDashArray_PushedPagesAreCollectedAfterForceGC Stop
Passed SharedDashArray_PushedPagesAreCollectedAfterForceGC [5 s]
NUnit Adapter 4.5.0.0: Test execution complete
Results File: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue35492.trx
Test Run Successful.
Total tests: 1
Passed: 1
Total time: 26.8342 Seconds
>>> TRX_RESULT_FILE: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue35492.trx
📁 Fix files reverted (2 files)
eng/pipelines/ci-copilot.ymlsrc/Controls/src/Core/Border/Border.cs
🧪 UI Tests — Border
Detected UI test categories: Border
✅ Deep UI tests — 43 passed, 0 failed across 1 category on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Border |
43/43 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🔍 Regression Cross-Reference
🔍 Regression Cross-Reference
🟢 No regression risks detected. No labeled bug-fix PRs in the last 6 months touched the modified files.
🔍 Pre-Flight — Context & Validation
Issue: #35492 - Border.StrokeDashArray leaks dashed Borders when using a shared Application resource
PR: #35544 - Fix Border.StrokeDashArray leaks dashed Borders when using a shared Application resource
Platforms Affected: iOS, Android, macOS/MacCatalyst; PR also reports Windows tested
Files Changed: 1 implementation, 2 test
Key Findings
- Root cause: dashed
Borderinstances subscribe to a long-lived sharedDoubleCollectionused asStrokeDashArray; direct collection event subscriptions can retain discardedBorders and page graphs. - PR fix replaces direct
CollectionChangedsubscriptions withWeakNotifyCollectionChangedProxy, keeps default dash-array subscription by subscribing whenStrokeDashPatternis read, and uses weak property proxies for stroke/stroke-shape handlers. - Test type is UI test on iOS:
Issue35492.SharedDashArray_PushedPagesAreCollectedAfterForceGC, categoryBorder. Prior gate already passed: test fails without fix and passes with fix. - Existing review feedback still notes UI-test reliability risk from
Thread.Sleep(3000)after forcing GC; code-review deemed this non-blocking for product correctness.
Code Review Summary
Verdict: LGTM
Confidence: high
Errors: 0 | Warnings: 1 | Suggestions: 0
Key code review findings:
⚠️ src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35492.cs:23uses fixedThread.Sleep(3000)after GC; prefer polling/retry aroundSummaryLabelto avoid timing-dependent UI test behavior.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35544 | Use WeakNotifyCollectionChangedProxy for StrokeDashArray.CollectionChanged, unsubscribe through lifecycle cleanup, and preserve default collection subscription from StrokeDashPattern getter. |
✅ PASSED (Gate) | src/Controls/src/Core/Border/Border.cs; UI repro/test files |
Original PR |
🔬 Code Review — Deep Analysis
Code Review - PR #35544
Independent Assessment
What this changes: Replaces direct StrokeDashArray.CollectionChanged subscriptions in Border.StrokeDashPattern with a stored WeakNotifyCollectionChangedProxy, while keeping stroke and stroke-shape property notifications on weak property proxies. Adds an issue UI test that pushes/pops a page containing dashed Borders backed by a shared app resource and checks the page can be collected.
Inferred motivation: The direct event subscription from a long-lived shared DoubleCollection can retain each dashed Border; when item template gestures reference the page, that retains the whole page graph.
Reconciliation with PR Narrative
Author claims: The PR fixes #35492 by making dash-array event lifecycle explicit, unsubscribing through weak proxies, and adds coverage across affected platforms.
Agreement/disagreement: The code matches the claimed leak fix. The current revision also addresses earlier review feedback about default StrokeDashArray subscriptions by subscribing when StrokeDashPattern is read. Existing unresolved review feedback about the test's fixed sleep remains valid but is not a product-code correctness blocker.
Findings
Warning - UI test uses a fixed delay after GC
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35492.cs:23 sleeps for three seconds before reading SummaryLabel. This is already noted in existing review comments. Prefer a retry/polling assertion around the label text so the test waits for the observable condition rather than a fixed timeout. CI is currently green, so I would treat this as a test reliability improvement rather than a blocker.
Devil's Advocate
The product fix relies on WeakNotifyCollectionChangedProxy holding the collection and delegate weakly while the source event holds the proxy strongly; this is the established pattern in WeakEventProxy.cs. Repeated StrokeDashPattern reads should not accumulate subscriptions because Subscribe detaches the proxy from the previous source before re-subscribing. The main blind spot is device-GC timing in the UI test, which is why the fixed sleep is called out.
Verdict: LGTM
Confidence: high
Summary: The product code fixes the shared dash-array retention path without introducing duplicate subscriptions, and CI checks are green. I found one non-blocking UI test reliability warning that is already present in review comments and should be considered before merge.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 / expert-review-guided | Static ConditionalWeakTable<Border, DashArraySubscriptionToken> with token-held weak reference to Border; subscribe token to StrokeDashArray.CollectionChanged, update Border through weak reference, and lazily self-unsubscribe when the Border is gone. |
✅ PASS — pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "Issue35492" |
1 file | Different from PR: no WeakNotifyCollectionChangedProxy field, no Border finalizer, no stroke/stroke-shape refactor, and no propertyChanging cleanup callback. Self-review findings: 0. |
| PR | PR #35544 | Add WeakNotifyCollectionChangedProxy fields and explicit dash-array subscription lifecycle in Border, including getter-time subscription for the default collection. |
✅ PASSED (Gate) | 3 files | Original PR. Gate already verified fail-without/pass-with-fix on iOS; not re-run here. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Proposed Candidate #1: ConditionalWeakTable plus self-cleaning weak subscription token. |
Exhausted: No — stopped because Candidate #1 passed the iOS regression test and is demonstrably better on implementation scope/overhead.
Selected Fix: Candidate #1 — It passes the same iOS regression test while reducing Border instance footprint, avoiding a finalizer, and leaving unrelated stroke/stroke-shape proxy code untouched. Tradeoff: dead token cleanup is lazy until the shared collection next raises CollectionChanged, but it does not retain the Border or page graph.
📋 Report — Final Recommendation
Comparative Fix Report — PR #35544
Candidates Compared
| Rank | Candidate | Regression Result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
✅ PASS — inherits passed gate from PR; product fix unchanged | Best overall candidate. Keeps the established WeakNotifyCollectionChangedProxy fix and applies the expert reviewer's only actionable feedback by replacing a fixed UI-test sleep with retry-based assertion logic. |
| 2 | pr |
✅ PASS — Gate passed; tests fail without fix and pass with fix | Correct product fix with no blocking expert-review findings. Ranked below pr-plus-reviewer only because the raw PR keeps a timing-dependent Thread.Sleep(3000) in the regression test. |
| 3 | try-fix-1 |
✅ PASS — pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "Issue35492" |
Viable alternative using a ConditionalWeakTable<Border, DashArraySubscriptionToken> and a weak token. It is more surgical in Border.cs, but introduces a novel static token table and lazy token cleanup while the PR uses the repository's existing weak event proxy pattern. |
Analysis
All evaluated candidates passed their recorded iOS regression evidence, so no candidate was demoted for failed tests. The raw PR fix addresses the root cause by removing the strong event-handler retention path from shared DoubleCollection resources to discarded Border instances. The expert reviewer found no blocking issue in the product code.
try-fix-1 is a credible alternative and was useful for validating that a smaller product-code change can pass the same regression. However, the PR's weak-proxy approach is more aligned with existing MAUI infrastructure (WeakNotifyCollectionChangedProxy) and its cleanup model is explicit through the existing Border finalizer path. The reviewer's only actionable concern was test reliability, not the product fix.
Winner
pr-plus-reviewer wins because it preserves the PR's validated and conventional product fix while incorporating the only expert-review improvement: making the regression test wait for the observable SummaryLabel state instead of using a fixed sleep.
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 whether this change resolves your issue. Thank you!
Issue Details
Border controls share one app-level StrokeDashArray resource, Border subscribes to that collection and does not always detach cleanly. Because the shared resource is long-lived, dashed Border instances can be retained
Description of Change
Introduces a proxy and collection-changed handler fields, unsubscribes in cleanup, updates subscription handling when StrokeDashArray changes, and triggers handler updates when the collection changes. In short, it makes dash-array event lifecycle explicit so old references are released instead of accumulating.
Note: Also added code to clean up the other proxy logic as well.
Issues Fixed
Fixes #35492
Tested the behavior in the following platforms.
Before.mov
After.mov