[leak-fix] Fix GeometryGroup.Children memory leak (Fixes #36365) - #36526
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@MauiBot The current-head review reports zero errors, warnings, suggestions, inline findings, or code correctness issues. Its low-confidence verdict is only unavailable authenticated required-CI metadata, so no no-op code change is warranted. Ready for another review. |
This comment has been minimized.
This comment has been minimized.
|
🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo) @MauiBot Reviewed the current-head result for No code change is needed. Ready for re-review. |
This comment has been minimized.
This comment has been minimized.
|
🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo) @MauiBot Reviewed the latest current-head result for No code change is needed. Ready for re-review. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@github-actions[bot] — new AI review results are available based on this last commit:
4d59222. 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: ✅ PASSED
Platform: ANDROID · Base: main · Merge base: 0395a53b
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 GeometryGroupMemoryTests GeometryGroupMemoryTests |
✅ FAIL — 114s | ✅ PASS — 84s |
🔴 Without fix — 🧪 GeometryGroupMemoryTests: FAIL ✅ · 114s
(truncated to last 15,000 chars)
cts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
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.16] Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.39] Discovered: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.41] Starting: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.50] AssigningChildrenWithNullEntrySubscribesValidChildren [FAIL]
[xUnit.net 00:00:01.50] System.NullReferenceException : Object reference not set to an instance of an object.
[xUnit.net 00:00:01.50] Stack Trace:
[xUnit.net 00:00:01.50] /_/src/Controls/src/Core/Shapes/GeometryGroup.cs(75,0): at Microsoft.Maui.Controls.Shapes.GeometryGroup.UpdateChildren(GeometryCollection oldCollection, GeometryCollection newCollection)
[xUnit.net 00:00:01.50] /_/src/Controls/src/Core/Shapes/GeometryGroup.cs(21,0): at Microsoft.Maui.Controls.Shapes.GeometryGroup.OnChildrenChanged(BindableObject bindable, Object oldValue, Object newValue)
[xUnit.net 00:00:01.50] /_/src/Controls/src/Core/BindableObject.cs(701,0): at Microsoft.Maui.Controls.BindableObject.OnBindablePropertySet(BindableProperty property, Object original, Object value, Boolean didChange, Boolean willFirePropertyChanged)
[xUnit.net 00:00:01.50] /_/src/Controls/src/Core/BindableObject.cs(688,0): at Microsoft.Maui.Controls.BindableObject.SetValueActual(BindableProperty property, BindablePropertyContext context, Object value, Boolean currentlyApplying, SetValueFlags attributes, SetterSpecificity specificity, Boolean silent)
[xUnit.net 00:00:01.50] /_/src/Controls/src/Core/BindableObject.cs(616,0): at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes, SetValuePrivateFlags privateAttributes, SetterSpecificity specificity)
[xUnit.net 00:00:01.50] /_/src/Controls/src/Core/BindableObject.cs(521,0): at Microsoft.Maui.Controls.BindableObject.SetValue(BindableProperty property, Object value)
[xUnit.net 00:00:01.50] /_/src/Controls/src/Core/Shapes/GeometryGroup.cs(41,0): at Microsoft.Maui.Controls.Shapes.GeometryGroup.set_Children(GeometryCollection value)
[xUnit.net 00:00:01.50] /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs(77,0): at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.AssigningChildrenWithNullEntrySubscribesValidChildren()
[xUnit.net 00:00:01.50] at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
[xUnit.net 00:00:01.50] at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:00:01.52] AppendPathSkipsNullChildren [FAIL]
[xUnit.net 00:00:01.52] System.NullReferenceException : Object reference not set to an instance of an object.
[xUnit.net 00:00:01.52] Stack Trace:
[xUnit.net 00:00:01.52] /_/src/Controls/src/Core/Shapes/GeometryGroup.cs(75,0): at Microsoft.Maui.Controls.Shapes.GeometryGroup.UpdateChildren(GeometryCollection oldCollection, GeometryCollection newCollection)
[xUnit.net 00:00:01.52] /_/src/Controls/src/Core/Shapes/GeometryGroup.cs(21,0): at Microsoft.Maui.Controls.Shapes.GeometryGroup.OnChildrenChanged(BindableObject bindable, Object oldValue, Object newValue)
[xUnit.net 00:00:01.52] /_/src/Controls/src/Core/BindableObject.cs(701,0): at Microsoft.Maui.Controls.BindableObject.OnBindablePropertySet(BindableProperty property, Object original, Object value, Boolean didChange, Boolean willFirePropertyChanged)
[xUnit.net 00:00:01.52] /_/src/Controls/src/Core/BindableObject.cs(688,0): at Microsoft.Maui.Controls.BindableObject.SetValueActual(BindableProperty property, BindablePropertyContext context, Object value, Boolean currentlyApplying, SetValueFlags attributes, SetterSpecificity specificity, Boolean silent)
[xUnit.net 00:00:01.52] /_/src/Controls/src/Core/BindableObject.cs(616,0): at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes, SetValuePrivateFlags privateAttributes, SetterSpecificity specificity)
[xUnit.net 00:00:01.52] /_/src/Controls/src/Core/BindableObject.cs(521,0): at Microsoft.Maui.Controls.BindableObject.SetValue(BindableProperty property, Object value)
[xUnit.net 00:00:01.52] /_/src/Controls/src/Core/Shapes/GeometryGroup.cs(41,0): at Microsoft.Maui.Controls.Shapes.GeometryGroup.set_Children(GeometryCollection value)
[xUnit.net 00:00:01.52] /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs(92,0): at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.AppendPathSkipsNullChildren()
[xUnit.net 00:00:01.52] at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
[xUnit.net 00:00:01.52] at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:00:01.53] AppendPathAllowsNullChildrenCollection [FAIL]
[xUnit.net 00:00:01.53] System.NullReferenceException : Object reference not set to an instance of an object.
[xUnit.net 00:00:01.53] Stack Trace:
[xUnit.net 00:00:01.53] /_/src/Controls/src/Core/Shapes/GeometryGroup.cs(118,0): at Microsoft.Maui.Controls.Shapes.GeometryGroup.AppendPath(PathF path)
[xUnit.net 00:00:01.53] /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs(113,0): at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.AppendPathAllowsNullChildrenCollection()
[xUnit.net 00:00:01.53] at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
[xUnit.net 00:00:01.53] at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:00:02.61] GeometryGroupDoesNotLeakWhenSharingChild [FAIL]
[xUnit.net 00:00:02.61] GeometryGroup should not be alive!
[xUnit.net 00:00:02.61] Stack Trace:
[xUnit.net 00:00:02.61] /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs(50,0): at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.GeometryGroupDoesNotLeakWhenSharingChild()
[xUnit.net 00:00:02.61] --- End of stack trace from previous location ---
Passed MovingChildrenPreservesSubscriptions [15 ms]
Failed AssigningChildrenWithNullEntrySubscribesValidChildren [2 ms]
Error Message:
System.NullReferenceException : Object reference not set to an instance of an object.
Stack Trace:
at Microsoft.Maui.Controls.Shapes.GeometryGroup.UpdateChildren(GeometryCollection oldCollection, GeometryCollection newCollection) in /_/src/Controls/src/Core/Shapes/GeometryGroup.cs:line 75
at Microsoft.Maui.Controls.Shapes.GeometryGroup.OnChildrenChanged(BindableObject bindable, Object oldValue, Object newValue) in /_/src/Controls/src/Core/Shapes/GeometryGroup.cs:line 21
at Microsoft.Maui.Controls.BindableObject.OnBindablePropertySet(BindableProperty property, Object original, Object value, Boolean didChange, Boolean willFirePropertyChanged) in /_/src/Controls/src/Core/BindableObject.cs:line 701
at Microsoft.Maui.Controls.BindableObject.SetValueActual(BindableProperty property, BindablePropertyContext context, Object value, Boolean currentlyApplying, SetValueFlags attributes, SetterSpecificity specificity, Boolean silent) in /_/src/Controls/src/Core/BindableObject.cs:line 688
at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes, SetValuePrivateFlags privateAttributes, SetterSpecificity specificity) in /_/src/Controls/src/Core/BindableObject.cs:line 616
at Microsoft.Maui.Controls.BindableObject.SetValue(BindableProperty property, Object value) in /_/src/Controls/src/Core/BindableObject.cs:line 521
at Microsoft.Maui.Controls.Shapes.GeometryGroup.set_Children(GeometryCollection value) in /_/src/Controls/src/Core/Shapes/GeometryGroup.cs:line 41
at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.AssigningChildrenWithNullEntrySubscribesValidChildren() in /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs:line 77
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Passed ReplacingChildrenCollectionMovesSubscriptions [< 1 ms]
Failed AppendPathSkipsNullChildren [< 1 ms]
Error Message:
System.NullReferenceException : Object reference not set to an instance of an object.
Stack Trace:
at Microsoft.Maui.Controls.Shapes.GeometryGroup.UpdateChildren(GeometryCollection oldCollection, GeometryCollection newCollection) in /_/src/Controls/src/Core/Shapes/GeometryGroup.cs:line 75
at Microsoft.Maui.Controls.Shapes.GeometryGroup.OnChildrenChanged(BindableObject bindable, Object oldValue, Object newValue) in /_/src/Controls/src/Core/Shapes/GeometryGroup.cs:line 21
at Microsoft.Maui.Controls.BindableObject.OnBindablePropertySet(BindableProperty property, Object original, Object value, Boolean didChange, Boolean willFirePropertyChanged) in /_/src/Controls/src/Core/BindableObject.cs:line 701
at Microsoft.Maui.Controls.BindableObject.SetValueActual(BindableProperty property, BindablePropertyContext context, Object value, Boolean currentlyApplying, SetValueFlags attributes, SetterSpecificity specificity, Boolean silent) in /_/src/Controls/src/Core/BindableObject.cs:line 688
at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes, SetValuePrivateFlags privateAttributes, SetterSpecificity specificity) in /_/src/Controls/src/Core/BindableObject.cs:line 616
at Microsoft.Maui.Controls.BindableObject.SetValue(BindableProperty property, Object value) in /_/src/Controls/src/Core/BindableObject.cs:line 521
at Microsoft.Maui.Controls.Shapes.GeometryGroup.set_Children(GeometryCollection value) in /_/src/Controls/src/Core/Shapes/GeometryGroup.cs:line 41
at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.AppendPathSkipsNullChildren() in /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs:line 92
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Failed AppendPathAllowsNullChildrenCollection [1 ms]
Error Message:
System.NullReferenceException : Object reference not set to an instance of an object.
Stack Trace:
at Microsoft.Maui.Controls.Shapes.GeometryGroup.AppendPath(PathF path) in /_/src/Controls/src/Core/Shapes/GeometryGroup.cs:line 118
at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.AppendPathAllowsNullChildrenCollection() in /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs:line 113
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Passed RemovingAndReplacingChildrenMovesSubscriptions [1 ms]
Failed GeometryGroupDoesNotLeakWhenSharingChild [1 s]
Error Message:
GeometryGroup should not be alive!
Stack Trace:
at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.GeometryGroupDoesNotLeakWhenSharingChild() in /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs:line 50
--- End of stack trace from previous location ---
Passed DuplicateChildrenPreserveOccurrenceSubscriptions [< 1 ms]
Passed SharedChildrenInvalidateEachLiveGroupAfterGc [24 ms]
Passed ChildGeometryChangesStillInvalidateAfterGc [25 ms]
[xUnit.net 00:00:03.70] GeometryGroupDoesNotLeakWhenSharingChildren [FAIL]
[xUnit.net 00:00:03.70] GeometryGroup should not be alive!
[xUnit.net 00:00:03.70] Stack Trace:
[xUnit.net 00:00:03.70] /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs(40,0): at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.GeometryGroupDoesNotLeakWhenSharingChildren()
[xUnit.net 00:00:03.70] --- End of stack trace from previous location ---
[xUnit.net 00:00:03.70] ResetChildrenResubscribesLaterAdditions [FAIL]
[xUnit.net 00:00:03.70] Assert.False() Failure
[xUnit.net 00:00:03.70] Expected: False
[xUnit.net 00:00:03.70] Actual: True
[xUnit.net 00:00:03.70] Stack Trace:
[xUnit.net 00:00:03.70] /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs(264,0): at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.ResetChildrenResubscribesLaterAdditions()
[xUnit.net 00:00:03.70] at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
[xUnit.net 00:00:03.70] at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:00:03.71] Finished: Microsoft.Maui.Controls.Core.UnitTests
Failed GeometryGroupDoesNotLeakWhenSharingChildren [1 s]
Error Message:
GeometryGroup should not be alive!
Stack Trace:
at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.GeometryGroupDoesNotLeakWhenSharingChildren() in /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs:line 40
--- End of stack trace from previous location ---
Failed ResetChildrenResubscribesLaterAdditions [1 ms]
Error Message:
Assert.False() Failure
Expected: False
Actual: True
Stack Trace:
at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupMemoryTests.ResetChildrenResubscribesLaterAdditions() in /_/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs:line 264
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Test Run Failed.
Total tests: 12
Passed: 6
Failed: 6
Total time: 4.2071 Seconds
🟢 With fix — 🧪 GeometryGroupMemoryTests: PASS ✅ · 84s
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
##vso[build.updatebuildnumber]10.0.100-ci+azdo.14650840
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
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.15] Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.37] Discovered: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.40] Starting: Microsoft.Maui.Controls.Core.UnitTests
Passed MovingChildrenPreservesSubscriptions [17 ms]
Passed AssigningChildrenWithNullEntrySubscribesValidChildren [< 1 ms]
Passed ReplacingChildrenCollectionMovesSubscriptions [< 1 ms]
Passed AppendPathSkipsNullChildren [4 ms]
Passed AppendPathAllowsNullChildrenCollection [< 1 ms]
Passed RemovingAndReplacingChildrenMovesSubscriptions [< 1 ms]
Passed GeometryGroupDoesNotLeakWhenSharingChild [33 ms]
Passed DuplicateChildrenPreserveOccurrenceSubscriptions [< 1 ms]
Passed SharedChildrenInvalidateEachLiveGroupAfterGc [21 ms]
[xUnit.net 00:00:01.59] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed ChildGeometryChangesStillInvalidateAfterGc [20 ms]
Passed GeometryGroupDoesNotLeakWhenSharingChildren [21 ms]
Passed ResetChildrenResubscribesLaterAdditions [1 ms]
Test Run Successful.
Total tests: 12
Passed: 12
Total time: 2.1024 Seconds
📁 Fix files reverted (1 files)
src/Controls/src/Core/Shapes/GeometryGroup.cs
📱 UI Tests — Shape
Detected UI test categories: Shape
✅ Deep UI tests — 35 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 |
|---|---|---|
Shape |
35/35 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
📋 Pre-Flight — Context & Validation
Issue: #36365 - [leak-scan] GeometryGroup.Children shared GeometryCollection strongly roots GeometryGroup via CollectionChanged
PR: #36526 - [leak-fix] Fix GeometryGroup.Children memory leak (Fixes #36365)
Platforms Affected: Shared managed Controls code; Android requested for testing, but no Android-specific code path changed.
Files Changed: 1 implementation, 1 test
Key Findings
GeometryGroupdirectly subscribed to sharedGeometryCollection.CollectionChangedand childGeometry.PropertyChanged, creating strong source-to-group retention paths.- PR replaces those strong subscriptions with existing weak event proxy helpers and adds focused memory/subscription regression tests.
- Gate was already passed before this run: tests fail without the fix and pass with the PR fix.
- Local targeted validation of the PR fix passed: 12/12
GeometryGroupMemoryTests.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 1 | Suggestions: 2
Key code review findings:
- ⚠ Null-child support is only partial: PR makes
GeometryGroup.AppendPathand subscription bookkeeping null-tolerant, while other GeometryGroup consumers may still dereference null children. - ℹ Avoid allocation in finalizer-reachable unsubscribe:
_childProxies.ToArray()could be replaced by an index loop. - ℹ Add null reassignment lifecycle test for old collection -> null -> new collection.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36526 | Per-GeometryGroup ChildrenSubscriptions holder using existing WeakNotifyCollectionChangedProxy / WeakNotifyPropertyChangedProxy; null-safe AppendPath; focused memory tests. |
✅ PASSED (Gate and local targeted test) | GeometryGroup.cs, GeometryGroupMemoryTests.cs |
Original PR |
🔬 Code Review — Deep Analysis
Code Review — PR #36526
Independent Assessment
What this changes: Replaces GeometryGroup.Children direct collection/child PropertyChanged subscriptions with weak event proxies to avoid retaining GeometryGroup through shared/long-lived geometry collections. Adds null-tolerant AppendPath behavior and memory/subscription tests.
Inferred motivation: Fix a memory leak when GeometryGroup shares GeometryCollection or child Geometry instances.
Reconciliation with PR Narrative
Author claims: PR/commit narrative claims a GeometryGroup.Children memory leak fix for #36365.
Agreement/disagreement: Agrees with the implementation: weak proxies break the strong event-reference chain while preserving invalidation behavior.
Prior Review Reconciliation
No prior ❌ Error findings could be evaluated: gh is unauthenticated, and unauthenticated GitHub REST/HTML review/comment surfaces returned rate-limit/action errors. Confidence is capped low.
Blast Radius Assessment
- Runs for all instances: Yes, all
GeometryGroupchildren subscription paths. - Startup impact: No direct startup path.
- Static/shared state: No new static state.
CI Status
- Required-check result: undetermined —
gh pr checks --requiredfailed due missing authentication. - Classification: undetermined.
- Action taken: capped confidence; local targeted test passed:
GeometryGroupMemoryTests12/12.
Findings
⚠️ Warning — Null-child support is only partial
GeometryGroup.cs:79 and GeometryGroup.cs:190 now tolerate null children/null Children for subscription and AppendPath, and tests assert this behavior. Other GeometryGroup.Children consumers still dereference children/collection directly, e.g. GeometryHelper.cs:46-48, Windows GeometryExtensions.cs:50-52, Android compatibility GeometryExtensions.cs:61-63. If null children are now supported, those paths may still throw; if not, tests should avoid implying broader support.
💡 Suggestion — Avoid allocation in finalizer-reachable unsubscribe
GeometryGroup.cs:162 copies _childProxies with ToArray() before unsubscribing. Since proxy.Unsubscribe() does not mutate _childProxies, an index loop plus Clear() would avoid finalizer-thread allocation.
💡 Suggestion — Add null reassignment lifecycle test
Current tests cover Children = null for AppendPath, but not old collection -> null -> new collection subscription behavior. A test would lock down that old children stop invalidating and new children resume invalidating.
Failure-Mode Probing
- Shared collection outlives group: weak proxies allow collection; covered by tests.
- Child changes after GC while group alive: handler delegate is kept by group field; covered by tests.
- Reset/remove/replace/move/duplicates: current tests cover expected subscription counts.
- Null children/collection across all consumers: only partially covered; see warning.
Verdict: NEEDS_DISCUSSION
Confidence: low
Summary: Core leak fix appears sound and targeted tests pass locally. I cannot LGTM because required CI and prior-review surfaces were unavailable, and null tolerance may be inconsistent across existing GeometryGroup consumers.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Static ConditionalWeakTable per-source weak subscription registry with weak fan-out to groups. |
❌ FAIL | 1 file | Failed required null-safe AppendPath tests; also more complex than PR. |
| 2 | try-fix | Per-instance weak delegate closures capturing only WeakReference<GeometryGroup> plus generation; full child-subscription rebuild. |
✅ PASS | 1 file | Avoids proxy helpers but leaves dead weak delegates on inactive long-lived sources until later source activity. Not demonstrably better. |
| 3 | try-fix | Move child PropertyChanged aggregation into GeometryCollection; GeometryGroup observes collection-level weak aggregate events. |
✅ PASS | 2 files | Potential O(children) sharing benefit, but broader blast radius: changes GeometryCollection from passive collection to active observer. Not preferable for this bug fix. |
| PR | PR #36526 | Per-group holder using existing weak proxy helpers; finalizer cleanup; focused null-safe render handling and tests. | ✅ PASSED (Gate) | 2 files | Original PR; selected as best balance of scope and reliability. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Suggested static per-source weak fan-out registry. |
| maui-expert-reviewer | 2 | Yes | After candidate 1 null failures, suggested per-instance weak delegate closures preserving null-safe AppendPath. |
| maui-expert-reviewer | 3 | Yes | Suggested GeometryCollection-owned child aggregation; tested as final distinct approach. |
| maui-expert-reviewer | 4 | No | Remaining alternatives are either trivial variations of candidate 2/3 or converge back to the PR's existing weak proxy holder pattern. |
Exhausted: Yes
Selected Fix: PR's fix — Candidate 2 and Candidate 3 pass the targeted tests, but neither is demonstrably better. Candidate 2 has stale-delegate cleanup tradeoffs; Candidate 3 has broader blast radius by changing GeometryCollection internals. The PR uses existing MAUI weak event proxy infrastructure in a localized one-file product change and remains the best candidate.
Candidate Detail Files
try-fix-1/content.mdtry-fix-2/content.mdtry-fix-3/content.md
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
|
🔍 AI-generated review response (GitHub Copilot CLI, on behalf of @kubaflo) @MauiBot Review
The review body appears to use the earlier 12-test/two-file snapshot; the current focused suite contains 15 tests. No additional product change is needed. Ready for re-review. |
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
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: Successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR addresses a confirmed managed memory leak in GeometryGroup caused by strong event subscriptions to a shared/long-lived GeometryCollection (and its child Geometry items). It updates GeometryGroup to use the existing weak-event proxy infrastructure to avoid retaining the group, and adds focused unit tests to prevent regressions.
Changes:
- Replace strong
CollectionChanged/PropertyChangedsubscriptions inGeometryGroupwithWeakNotifyCollectionChangedProxy/WeakNotifyPropertyChangedProxy, including subscription lifecycle management for add/remove/reset. - Make geometry flattening/rendering/conversion paths treat
Children == nullandnullchild entries as empty (skip safely) across supported platforms. - Add a comprehensive unit test suite validating both “no leak” behavior and correct invalidation/subscription behavior (including duplicates, move, reset, replace).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs | Adds regression tests covering the leak scenario and subscription lifecycle correctness. |
| src/Controls/src/Core/Shapes/GeometryHelper.cs | Updates flattening to skip null Children and null child entries. |
| src/Controls/src/Core/Shapes/GeometryGroup.cs | Implements weak subscription management and null-safe AppendPath behavior. |
| src/Controls/src/Core/Platform/Windows/Extensions/GeometryExtensions.cs | Makes Windows geometry conversion skip null Children / null children. |
| src/Controls/src/Core/Compatibility/Android/Extensions/GeometryExtensions.cs | Makes Android geometry conversion skip null Children / null children. |
| src/Compatibility/Core/src/Tizen/Extensions/GeometryExtensions.cs | Makes Tizen geometry conversion skip null Children / null children. |
| src/Compatibility/Core/src/iOS/Extensions/GeometryExtensions.cs | Makes iOS/macOS geometry conversion skip null Children / null children. |
|
🔍 AI-generated merge-readiness analysis (GitHub Copilot CLI, on behalf of @kubaflo): good to merge. All three current-head pipelines completed for
The exact base commit’s UI build ( |
This comment has been minimized.
This comment has been minimized.
Tests Failure Analysis
Test Failure Review: Not ready - click to expandOverall verdict: Not ready. One build leg is a deterministic regression versus
Coverage: 162 checks · 155 passing · 7 failing · 0 pending · 0 inaccessible · 1 unmapped · 15 unexplained build legs · 0 unaccounted failing checks · 1 aborted failing check · 0 canceled-build checks · 0 device-test unverified · 12 unattributed · 1 regressed-vs-base. Deterministic ceiling: Not ready — 1 leg is a deterministic regression vs base and 12 failures are unattributed, plus an aborted check and 1 check (Build Analysis) with no inspectable AzDO evidence. Builds (this PR): maui-pr-uitests 1510859, maui-pr-devicetests 1510860. Base sampling (main, 5 recent builds for maui-pr-uitests): 1503618, 1503332, 1503036, 1499834, 1499259. Recommended actionA human should inspect the regressed |
|
🔍 AI-generated follow-up analysis (GitHub Copilot CLI, on behalf of @kubaflo) The In UI build 1510859, the failing Mono API-30 job never installed the app or ran tests: ADB reported The PR changes only GeometryGroup subscription cleanup and tests, with no emulator/XHarness/provisioning overlap. No code change is required. |
… on RS0016 The gate recompiles the MAUI product (Controls.Core, ...) from source via the unit/XAML test project's P2P references, re-running the PublicAPI analyzer under the repo-wide TreatWarningsAsErrors=true. A leak-fix PR that adds a finalizer (e.g. #36605 ~SwipeView()) surfaces RS0016/RS0017 as a build-breaking ERROR during the revert -> build -> restore -> build cycle, so the with-fix build fails to compile and the gate reports a false FAILED — even though the PR's own maui-pr build (a REQUIRED check that separately enforces PublicAPI bookkeeping) is green. Many in-flight PRs are leak fixes that add finalizers (#36575, #36566, #36547, #36531, #36526, #36521, #36513, ...), so ALL of them hit this false-FAILED. Fix: pass -p:TreatWarningsAsErrors=false to the gate's unit/XAML dotnet test and clean-rebuild-retry invocations, matching the deep stage's existing mitigation in Build-AndDeploy.ps1. The gate verifies TEST BEHAVIOR, not API bookkeeping; genuine CS-level compile ERRORS still fail the build. The UITest/device path already routes through Build-AndDeploy.ps1 and was already covered. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
Rebased onto inflight/current, which already contains #36232's shared-children fix using strong CollectionChanged/PropertyChanged subscriptions. Those strong subscriptions are exactly what #36365 reports: a shared or long-lived GeometryCollection roots the GeometryGroup (and any Path using it). Replace the strong subscriptions with WeakNotifyCollectionChangedProxy / WeakNotifyPropertyChangedProxy so the collection no longer roots the group, while preserving the Clear()/Reset teardown behavior guarded by GeometryGroupTests.ClearUnsubscribesPreviousChildrenFromPropertyChanged. Also adds null-safe iteration in GeometryHelper and the platform GeometryExtensions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e27685d0-fe80-460a-aa05-83d2ab9bf032
4d59222 to
33c5302
Compare
|
🔍 AI-generated (GitHub Copilot CLI, on behalf of @kubaflo) Rebased onto Verified locally on the net TFM: all 16 GeometryGroup tests pass (base guard test + the new |
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
This pull request was generated automatically by the Memory Leak Fixer agentic workflow. It contains regression tests, including one proven to fail on unpatched
main, and the managed product fix that makes them pass.Fixes #36365
Refs: #36365
Target branch: main
Attempt: 1/3
The leak
GeometryGroupsubscribed to itsChildren(GeometryCollection) via plainCollectionChanged += OnChildrenCollectionChangedand to each childGeometryviaPropertyChanged += OnChildrenPropertyChanged. When theGeometryCollectionis shared/long-lived, those strong delegate references from the collection (and children) back to theGeometryGroupkeep the group — and everything it roots — alive after it should have been collected.Retention path: shared
GeometryCollection→CollectionChangeddelegate →GeometryGroup(never released while the collection lives).The fix
Replace the strong subscriptions in
src/Controls/src/Core/Shapes/GeometryGroup.cswith the weak-subscription helpers already used across the codebase (WeakNotifyCollectionChangedProxy/WeakNotifyPropertyChangedProxyfromInternals/WeakEventProxy.cs), managed by a privateChildrenSubscriptionsholder with a finalizer that unsubscribes. Collection add/remove/reset events keep childPropertyChangedsubscriptions in sync. Rendering and flattening treat a nullChildrencollection and null entries within it as empty, and Android, iOS/macOS, Tizen, and Windows geometry conversion paths apply the same behavior. Finalizer cleanup unsubscribes child proxies in place to avoid allocating while releasing the group. No public API change (private nested class + fields).Regression tests
Added
src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs:GeometryGroupDoesNotLeakWhenSharingChildren— verifies a shared collection does not retain the group.GeometryGroupDoesNotLeakWhenSharingChild— verifies a shared child does not retain the group.ChildGeometryChangesStillInvalidateAfterGc— preserves live child invalidation after GC.AssigningChildrenWithNullEntrySubscribesValidChildren— covers direct assignment with a null entry while preserving valid-child invalidation.AppendPathSkipsNullChildren— verifies rendering skips null entries while still appending valid geometry.AppendPathAllowsNullChildrenCollection— verifies rendering treats a nullChildrencollection as empty.FlattenGeometrySkipsNullChildren/FlattenGeometryAllowsNullChildrenCollection— verify flattening follows the same null-as-empty contract.SharedChildrenInvalidateEachLiveGroupAfterGc— preserves invalidation for multiple groups sharing one collection.RemovingAndReplacingChildrenMovesSubscriptions— moves subscriptions for item removal and replacement.ReplacingChildrenCollectionMovesSubscriptions— detaches the old collection and children and attaches the replacement collection and children.ReplacingChildrenThroughNullMovesSubscriptions— verifies old → null → new transitions detach stale sources and resume invalidation on the replacement.MovingChildrenPreservesSubscriptions— reordering children preserves exactly one invalidation subscription per child.DuplicateChildrenPreserveOccurrenceSubscriptions— tracks duplicate occurrences independently.ResetChildrenResubscribesLaterAdditions— detaches cleared children and subscribes later additions.GeometryGroupMemoryTestspassScope
Changes are limited to
GeometryGroupsubscription cleanup, null-safe geometry conversion and flattening paths on Android, iOS/macOS, Tizen, and Windows, plus focused unit tests. No public API changes.