Skip to content

[leak-fix] Fix TransformGroup.Children memory leak (Fixes #36367) - #36531

Closed
github-actions[bot] wants to merge 1 commit into
inflight/currentfrom
leak-fix/issue-36367-26fc76508186d186
Closed

[leak-fix] Fix TransformGroup.Children memory leak (Fixes #36367)#36531
github-actions[bot] wants to merge 1 commit into
inflight/currentfrom
leak-fix/issue-36367-26fc76508186d186

Conversation

@github-actions

@github-actions github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

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!

🤖 This pull request was generated automatically by the Memory Leak Fixer agentic workflow. It contains an empirically-validated regression test (red→green) plus a managed product fix.

Fixes #36367
Refs: #36367
Target branch: main
Attempt: 1/3

The leak

TransformGroup subscribed to its Children (TransformCollection) via a plain
CollectionChanged += instanceMethod handler, and to each child Transform via a plain
PropertyChanged += instanceMethod handler. Because these are strong delegates, a shared or
long-lived TransformCollection
keeps the TransformGroup alive: the collection's event holds
a strong reference back to the group, so the group can never be collected while the collection
lives. The teardown (-=) only happened when Children was reassigned, never when the group
itself was dropped.

The fix

src/Controls/src/Core/Shapes/TransformGroup.cs now routes both subscriptions through the
existing weak-event helpers used elsewhere in the codebase:

  • WeakNotifyCollectionChangedProxy for the collection's CollectionChanged
  • WeakNotifyPropertyChangedProxy for each child's PropertyChanged

These are managed by a small nested ChildrenSubscriptions class that also has a finalizer
(~ChildrenSubscriptions() => UnsubscribeAll()), mirroring the pattern used by other
WeakEventProxy owners (e.g. the sibling GeometryGroup hardening in #36526). The group no
longer roots itself through a shared collection, and child add/remove/replace/reset are still
tracked so transform invalidation continues to work. Move operations reuse existing child proxies. Matrix recomputation also treats a null Children collection and null collection entries as empty.

The change is fully managed and cross-platform (src/Controls/src).

Regression test

Added src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs with ten tests:

  • TransformGroupDoesNotLeakWhenSharingChildren — assigns a shared TransformCollection to a
    TransformGroup, drops the group, and asserts it is collected. This is the genuine leak check.
  • ChildTransformChangesStillInvalidateAfterGc — verifies that after a GC, changing a child
    transform still invalidates the group's Value (guards against the weak subscriptions being
    collected too eagerly).
  • ExistingChildTransformChangesStillInvalidateAfterGc — assigns a pre-populated
    TransformCollection and verifies changing an existing child still updates the group after GC.
  • ReassigningChildrenMovesChangeSubscriptions — replaces the full collection, verifies old collection/child mutations no longer invalidate the group, and confirms replacement mutations remain active.
  • RemovingChildTransformReleasesSubscription — removes one child while keeping it alive, then
    verifies its weak proxy is collected and the retained child still invalidates the group.
  • ReplacingChildTransformReleasesOldSubscription — replaces a child while keeping the old child
    alive, verifies the old weak proxy is collected, and confirms the replacement still invalidates the group.
  • MovingChildTransformsReusesSubscriptions — moves existing children, verifies the matrix changes
    for the new order, and confirms the existing weak proxies continue to invalidate after GC.
  • ClearingChildrenReleasesSubscriptionsAndAllowsReuse — clears all children, verifies both weak
    proxies are collected, and confirms a subsequently added child still invalidates after GC.
  • NullChildrenUseIdentityMatrix — treats a null Children collection as empty and preserves the identity matrix.
  • NullChildIsIgnoredWhenUpdatingMatrix — skips null entries while composing the remaining transforms.

The cleanup-specific tests intentionally inspect private proxy instances because they verify eager detachment and collection of dead subscription objects; public matrix behavior alone cannot distinguish that from a stale weak proxy retained by a quiet source.

Initial red → green evidence (net TFM, from-source build):

Without the fix (unpatched main product source):

Failed TransformGroupDoesNotLeakWhenSharingChildren [1 s]
  Error Message:
   TransformGroup should not be alive!
Passed ChildTransformChangesStillInvalidateAfterGc
Total tests: 2  Passed: 1  Failed: 1

With the fix:

Passed ChildTransformChangesStillInvalidateAfterGc [59 ms]
Passed TransformGroupDoesNotLeakWhenSharingChildren [54 ms]
Test Run Successful.
Total tests: 2  Passed: 2

Review follow-up red → green evidence:

Before subscribing transforms already present in an assigned collection:

Failed ExistingChildTransformChangesStillInvalidateAfterGc
  Assert.NotEqual() Failure: Values are equal

After subscribing the collection contents during attachment:

Passed TransformGroupMemoryTests
Total tests: 3  Passed: 3  Failed: 0

Latest focused result: TransformGroupMemoryTests passes all 10 tests on net10.0.

Scope

  • Product change limited to managed code: src/Controls/src/Core/Shapes/TransformGroup.cs.
  • No public API surface change (the ChildrenSubscriptions helper is a private nested class).
  • No existing tests were muted, skipped, or weakened.

Generated by Memory Leak Fixer · 439.3 AIC · ⌖ 22.2 AIC · ⊞ 19.6K ·

@github-actions github-actions Bot added agentic-workflows perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) labels Jul 12, 2026
@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 12, 2026

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 1 findings

See inline comments for details.

Comment thread src/Controls/src/Core/Shapes/TransformGroup.cs Outdated
@MauiBot MauiBot added s/agent-gate-passed AI verified tests catch the bug (fail without fix, pass with fix) s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels Jul 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor Author

🚀 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 -- 36531

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36531"

@kubaflo

kubaflo commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot The existing-child subscription gap is fixed at head 788f049935: pre-populated collections now subscribe their current transforms, the regression test fails before the fix and passes after it, the unused test import was removed, and the PR description is current. The inline thread is resolved and the PR is ready for re-review.

@kubaflo

This comment has been minimized.

MauiBot

This comment was marked as outdated.

@kubaflo

kubaflo commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot The assigned-child suggestion is already addressed in the cited head 788f049935: ChildrenSubscriptions.Subscribe enumerates the newly assigned collection and adds a weak property-change proxy for every existing transform. ExistingChildTransformChangesStillInvalidateAfterGc specifically covers that path. The subscription-owner warning explicitly accepts the established finalizable weak-proxy pattern, so no no-op code change is warranted. Ready for another review.

@kubaflo

This comment has been minimized.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jul 12, 2026
@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 12, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jul 12, 2026
@kubaflo

kubaflo commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot The current-head review reports zero errors, warnings, suggestions, inline findings, or actionable code issues and confirms the prior major finding is fixed. Its discussion verdict is only unavailable or pending required CI, so no no-op code change is warranted. Ready for another review.

@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 12, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jul 12, 2026
@kubaflo

kubaflo commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated update (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot Implemented the remove/reset coverage warning in 1292f5db73:

  • RemovingChildTransformReleasesSubscription keeps the removed transform alive, proves its weak property proxy is collectible, and confirms the retained child still invalidates the group.
  • ClearingChildrenReleasesSubscriptionsAndAllowsReuse proves both cleared child proxies are collectible and confirms a newly added child still invalidates after GC.

All 5 focused TransformGroupMemoryTests pass on net10.0, and the PR description now documents the added coverage. Ready for re-review.

@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 12, 2026

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 1 findings

See inline comments for details.

Comment thread src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs Outdated
@kubaflo

kubaflo commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot Addressed the current replace-path coverage finding in f8eae8d49d. The regression now verifies both sides of NotifyCollectionChangedAction.Replace: the old child proxy is released while the replacement remains subscribed and updates TransformGroup.Value. The focused suite now contains six passing tests, and the PR description has been updated. Ready for re-review.

@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 14, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jul 15, 2026
@kubaflo

kubaflo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot Review 4700036789 passed the supplied gate, ran all 8 focused TransformGroupMemoryTests, found no error-level issue, and selected the current PR as the strongest fix.

No code change is warranted for the warning: TransformCollection is sealed and currently has an empty body, so its only Reset behavior is the inherited ObservableCollection<T>.ClearItems; treating Reset as clear is therefore the exact current invariant, not a correctness gap. Likewise, TransformGroup has no explicit disposal/lifecycle hook today, so the nested owner finalizer follows the established WeakEventProxy ownership pattern. Explicit teardown can be added if such a lifecycle surface is introduced later.

@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 15, 2026
MauiBot

This comment was marked as outdated.

@kubaflo

kubaflo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot Review 4700555657 reports 0 errors, 0 warnings, and 0 suggestions, confirms the gate passed, and selects the current PR implementation as the best fix. No code change is warranted. The [leak-fix] prefix is intentionally retained because it identifies this coordinated leak-fix PR fleet; the implementation description remains accurate for head 08cc4be332f.

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a managed memory leak in TransformGroup where a shared/long-lived TransformCollection could strongly root the owning TransformGroup via event subscriptions, and adds focused unit tests to prevent regressions in both leak behavior and invalidation behavior.

Changes:

  • Replaces strong CollectionChanged / per-child PropertyChanged subscriptions with weak-event proxies managed by a private ChildrenSubscriptions helper (with finalizer-based cleanup).
  • Updates collection-change handling to keep per-child subscriptions in sync across add/remove/replace/reset/move operations.
  • Adds TransformGroupMemoryTests covering leak prevention and ensuring invalidation still works after GC and across collection mutations.

Reviewed changes

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

File Description
src/Controls/src/Core/Shapes/TransformGroup.cs Switches to weak subscription proxies for Children and child transforms to prevent retention via shared collections.
src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs Adds regression tests validating the leak fix and subscription lifecycle behavior across collection operations and GC.

Comment thread src/Controls/src/Core/Shapes/TransformGroup.cs Outdated
@kubaflo

kubaflo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated update (GitHub Copilot CLI, on behalf of @kubaflo)

@copilot-pull-request-reviewer, the null collection/item finding is fixed in f5cb57d with two red-to-green regressions, and the PR description now documents the behavior and 10-test focused suite. Ready for re-review.

@kubaflo

This comment has been minimized.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review Summary

@github-actions[bot] — new AI review results are available based on this last commit: f5cb57d. To request a fresh review after new comments or commits, comment /review rerun.

Gate Passed Confidence Low Platform Android


🗂️ 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)
🧪 TransformGroupMemoryTests TransformGroupMemoryTests ✅ FAIL — 146s ✅ PASS — 116s
🔴 Without fix — 🧪 TransformGroupMemoryTests: FAIL ✅ · 146s

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

     at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.ExistingChildTransformChangesStillInvalidateAfterGc() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 86
     at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.ReassigningChildrenMovesChangeSubscriptions() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 127
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
     at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.GetChildProxies(TransformGroup group) in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 23
   at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.MovingChildTransformsReusesSubscriptions() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 187
     at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.TransformGroupDoesNotLeakWhenSharingChildren() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 50
   at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.GetChildProxyReference(TransformGroup group, Int32 index) in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 16
   at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.ClearingChildrenReleasesSubscriptionsAndAllowsReuse() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 219
     at Microsoft.Maui.Controls.Shapes.TransformGroup.UpdateTransformMatrix() in /_/src/Controls/src/Core/Shapes/TransformGroup.cs:line 76
   at Microsoft.Maui.Controls.Shapes.TransformGroup.OnTransformGroupChanged(BindableObject bindable, Object oldValue, Object newValue) in /_/src/Controls/src/Core/Shapes/TransformGroup.cs:line 47
   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.TransformGroup.set_Children(TransformCollection value) in /_/src/Controls/src/Core/Shapes/TransformGroup.cs:line 31
   at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.NullChildrenUseIdentityMatrix() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 244
   at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.RemovingChildTransformReleasesSubscription() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 139
     at Microsoft.Maui.Controls.Shapes.TransformGroup.UpdateTransformMatrix() in /_/src/Controls/src/Core/Shapes/TransformGroup.cs:line 77
   at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.NullChildIsIgnoredWhenUpdatingMatrix() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 256
   at Microsoft.Maui.Controls.Core.UnitTests.TransformGroupMemoryTests.ReplacingChildTransformReleasesOldSubscription() in /_/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs:line 163
🟢 With fix — 🧪 TransformGroupMemoryTests: PASS ✅ · 116s

(no coded error found; showing last 1200 chars)

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.22]   Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.91]   Discovered:  Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.94]   Starting:    Microsoft.Maui.Controls.Core.UnitTests
  Passed ExistingChildTransformChangesStillInvalidateAfterGc [96 ms]
  Passed ReassigningChildrenMovesChangeSubscriptions [14 ms]
  Passed ChildTransformChangesStillInvalidateAfterGc [51 ms]
  Passed MovingChildTransformsReusesSubscriptions [36 ms]
  Passed TransformGroupDoesNotLeakWhenSharingChildren [116 ms]
  Passed ClearingChildrenReleasesSubscriptionsAndAllowsReuse [149 ms]
  Passed NullChildrenUseIdentityMatrix [< 1 ms]
  Passed RemovingChildTransformReleasesSubscription [80 ms]
  Passed NullChildIsIgnoredWhenUpdatingMatrix [< 1 ms]
[xUnit.net 00:00:02.69]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed ReplacingChildTransformReleasesOldSubscription [62 ms]

Test Run Successful.
Total tests: 10
     Passed: 10
 Total time: 3.4414 Seconds

📁 Fix files reverted (1 files)
  • src/Controls/src/Core/Shapes/TransformGroup.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: #36367 - [leak-scan] TransformGroup.Children — shared TransformCollection strongly roots the TransformGroup via CollectionChanged
PR: #36531 - [leak-fix] Fix TransformGroup.Children memory leak (Fixes #36367)
Platforms Affected: All platforms; managed Controls code. Test platform requested for this run: android.
Files Changed: 1 implementation, 1 test

Key Findings

  • Issue #36367 reports a strong TransformCollection.CollectionChanged += group.OnChildrenCollectionChanged subscription that keeps TransformGroup alive when the collection is shared or long-lived.
  • PR #36531 changes src/Controls/src/Core/Shapes/TransformGroup.cs to route collection and child transform subscriptions through weak event proxies, and adds focused unit regression coverage in TransformGroupMemoryTests.
  • Public issue comments were empty. PR discussion showed prior MauiBot concerns around pre-populated collections, replace/move coverage, and reassignment coverage; current code/test coverage addresses those points.
  • Test type detected: Controls unit tests (src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj) focused on TransformGroupMemoryTests. No UI test category is directly impacted.

Code Review Summary

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

Key code review findings:

  • Code review found no actionable implementation findings.
  • Prior MauiBot issue "assigned pre-populated TransformCollection children were not subscribed" is fixed by subscribing current children in ChildrenSubscriptions.Subscribe.
  • Prior MauiBot requests for replace/move/reassignment coverage are covered by current regression tests.
  • CI status was undetermined because gh is unauthenticated in this environment; confidence remains low per code-review rules.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #36531 Use WeakNotifyCollectionChangedProxy for Children.CollectionChanged plus WeakNotifyPropertyChangedProxy instances for each child transform, tracked by a nested ChildrenSubscriptions helper. ✅ PASSED (Gate) TransformGroup.cs, TransformGroupMemoryTests.cs Original PR fix; gate result was supplied as already passed.

🔬 Code Review — Deep Analysis

Code Review — PR #36531

Independent Assessment

What this changes: TransformGroup now uses weak collection/property changed proxies for Children and child transforms, and handles null/empty children safely while preserving matrix invalidation.
Inferred motivation: Prevent shared/long-lived TransformCollection instances from strongly rooting TransformGroup.

Reconciliation with PR Narrative

Author claims: Fixes TransformGroup.Children memory leak from #36367 using weak subscriptions and regression tests.
Agreement/disagreement: Agrees. The implementation matches the claimed root cause and preserves child mutation behavior.

Prior Review Reconciliation

Prior Error Finding Source Status Evidence
Assigned pre-populated TransformCollection children were not subscribed MauiBot [major] inline Fixed Subscribe() now adds existing children at TransformGroup.cs:139-145; covered by TransformGroupMemoryTests.cs:71-88.
Replace/Move mutation coverage missing MauiBot [moderate] inline Fixed Replace covered at TransformGroupMemoryTests.cs:154-176; Move at 178-208.
Reassignment test did not prove old subscriptions removed MauiBot [moderate] inline Fixed Old child/collection mutations are asserted not to change value at TransformGroupMemoryTests.cs:113-119.

Blast Radius Assessment

  • Runs for all instances: yes — all TransformGroup.Children subscriptions use this path.
  • Startup impact: no — no static/startup code.
  • Static/shared state: no.

CI Status

  • Required-check result: pending/undetermined.
  • Classification: undetermined.
  • Action taken: gh pr checks --required unavailable due missing auth; public check data was not sufficient for required-check classification. Confidence capped low; no LGTM.

Findings

No Error, Warning, or Suggestion findings.

Failure-Mode Probing

  • Shared collection outlives group: weak proxy breaks strong root; finalizer unsubscribes lingering proxy.
  • Assigned collection already has children: existing children are subscribed in Subscribe().
  • Remove/replace/clear/move: subscriptions are removed/reused appropriately; tests cover each path.
  • Null Children/null child: guarded and tested; identity matrix/ignored child behavior is safe.

Verdict: NEEDS_DISCUSSION

Confidence: low
Summary: Code review found no actionable implementation issues, and prior findings appear fixed. However CI is still pending/undetermined, so the skill rules prohibit LGTM until required checks complete.


🛠️ Fix — Analysis & Comparison

Fix Candidates

# Source Approach Test Result Files Changed Notes
1 try-fix-1 Collection-owned aggregate invalidation event in TransformCollection; TransformGroup listens to one weak event. ✅ PASS 3 files Mechanically passed 10 focused unit tests, but self-review found broad global behavior risk.
2 try-fix-2 Static per-source weak observer registry in TransformGroup, with weak group references and duplicate-aware local child membership. ✅ PASS 2 files Passed 12 focused unit tests, but self-review found high complexity/global-state risk.
3 try-fix-3 Weak collection proxy plus direct child PropertyChanged subscriptions filtered by current membership. ❌ FAIL 2 files Failed the core leak test; child transform still roots the group through direct PropertyChanged.
PR PR #36531 TransformGroup owns weak collection and per-child property proxies through private ChildrenSubscriptions. ✅ PASSED (Gate) 2 files Original PR fix; gate result supplied as already passed.

Cross-Pollination

Model Round New Ideas? Details
gpt-5.5 expert reviewer 1 Yes Candidate 1: move child observation into TransformCollection.
gpt-5.5 expert reviewer 1 Yes Candidate 2: static per-source weak observer registry localized to TransformGroup.
gpt-5.5 expert reviewer 1 Yes Candidate 3: weak collection proxy only, with direct child subscriptions guarded by membership checks.
gpt-5.5 expert reviewer 2 No NO NEW IDEAS: any viable local fix reduces to weak collection subscription plus weak per-child property subscriptions; excluded alternatives are broader, more complex, or empirically failing.

Exhausted: Yes
Selected Fix: PR #36531 — It passes the supplied gate and focused regression tests while keeping the blast radius localized to TransformGroup. Passing alternatives were broader or more complex; the simpler weak-collection-only alternative failed the leak test.


📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current description is accurate and high quality, but the title uses a workflow/noise prefix instead of the required component-focused title format.

Recommended title

[All] Shapes: Fix TransformGroup.Children memory leak

Recommended description

> 🤖 This pull request was generated automatically by the **Memory Leak Fixer** agentic workflow. It contains an empirically-validated regression test (red→green) plus a managed product fix.

Fixes #36367
Refs: dotnet/maui#36367
Target branch: main
Attempt: 1/3

## The leak

`TransformGroup` subscribed to its `Children` (`TransformCollection`) via a plain
`CollectionChanged += instanceMethod` handler, and to each child `Transform` via a plain
`PropertyChanged += instanceMethod` handler. Because these are strong delegates, a **shared or
long-lived `TransformCollection`** keeps the `TransformGroup` alive: the collection's event holds
a strong reference back to the group, so the group can never be collected while the collection
lives. The teardown (`-=`) only happened when `Children` was reassigned, never when the group
itself was dropped.

## The fix

`src/Controls/src/Core/Shapes/TransformGroup.cs` now routes both subscriptions through the
existing weak-event helpers used elsewhere in the codebase:

- `WeakNotifyCollectionChangedProxy` for the collection's `CollectionChanged`
- `WeakNotifyPropertyChangedProxy` for each child's `PropertyChanged`

These are managed by a small nested `ChildrenSubscriptions` class that also has a finalizer
(`~ChildrenSubscriptions() => UnsubscribeAll()`), mirroring the pattern used by other
`WeakEventProxy` owners. The group no longer roots itself through a shared collection, and child
add/remove/replace/reset are still tracked so transform invalidation continues to work. Move
operations reuse existing child proxies. Matrix recomputation also treats a null `Children`
collection and null collection entries as empty.

The change is fully managed and cross-platform (`src/Controls/src`).

## Regression test

Added `src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs` with ten tests:

- `TransformGroupDoesNotLeakWhenSharingChildren` — assigns a shared `TransformCollection` to a
  `TransformGroup`, drops the group, and asserts it is collected. This is the genuine leak check.
- `ChildTransformChangesStillInvalidateAfterGc` — verifies that after a GC, changing a child
  transform still invalidates the group's `Value` (guards against the weak subscriptions being
  collected too eagerly).
- `ExistingChildTransformChangesStillInvalidateAfterGc` — assigns a pre-populated
  `TransformCollection` and verifies changing an existing child still updates the group after GC.
- `ReassigningChildrenMovesChangeSubscriptions` — replaces the full collection, verifies old
  collection/child mutations no longer invalidate the group, and confirms replacement mutations
  remain active.
- `RemovingChildTransformReleasesSubscription` — removes one child while keeping it alive, then
  verifies its weak proxy is collected and the retained child still invalidates the group.
- `ReplacingChildTransformReleasesOldSubscription` — replaces a child while keeping the old child
  alive, verifies the old weak proxy is collected, and confirms the replacement still invalidates
  the group.
- `MovingChildTransformsReusesSubscriptions` — moves existing children, verifies the matrix changes
  for the new order, and confirms the existing weak proxies continue to invalidate after GC.
- `ClearingChildrenReleasesSubscriptionsAndAllowsReuse` — clears all children, verifies both weak
  proxies are collected, and confirms a subsequently added child still invalidates after GC.
- `NullChildrenUseIdentityMatrix` — treats a null `Children` collection as empty and preserves the
  identity matrix.
- `NullChildIsIgnoredWhenUpdatingMatrix` — skips null entries when recomputing the transform matrix.

## Platforms

The product change is managed Controls code and applies to all platforms. The supplied regression
gate for this review passed on Android.

🏁 Report — Final Recommendation

Comparative Fix Report — PR #36531

Inputs

  • Issue: #36367, TransformGroup.Children memory leak from strong event subscriptions.
  • Supplied gate: ✅ PASSED — focused regression tests fail without the fix and pass with the fix on Android.
  • Expert PR review: no actionable inline findings; inline-findings.json is an empty JSON array.

Candidate ranking

Rank Candidate Regression result Assessment
1 pr ✅ PASSED Best candidate. It directly fixes both known retention paths with weak collection and weak per-child property subscriptions, keeps the behavior localized to TransformGroup, and has focused tests for leak prevention plus add/remove/replace/reset/move/null behavior.
2 pr-plus-reviewer ✅ PASSED Equivalent to pr. The expert reviewer found no actionable changes to apply, so this candidate has the same code and risk profile as the raw PR fix. Ranked just below pr because it adds no implementation improvement.
3 try-fix-1 ✅ PASS Functionally passed the focused tests, but moves child observation into TransformCollection via an internal aggregate event. That broadens behavior for every TransformCollection consumer and introduces global collection-owned observation that is unnecessary for this localized leak.
4 try-fix-2 ✅ PASS Functionally passed the focused tests, including duplicate/shared-observer scenarios, but relies on a static per-source weak observer registry with ConditionalWeakTable entries and weak group dispatch. It is substantially more complex than the PR fix and introduces global static observer state for a local ownership problem.
5 try-fix-3 ❌ FAIL Must rank below all passing candidates. It keeps direct child PropertyChanged subscriptions, so a shared collection can keep a child alive and the child can still strongly root the TransformGroup; the core leak test failed.

Comparison details

pr and pr-plus-reviewer solve the precise issue while preserving existing TransformGroup ownership boundaries. ChildrenSubscriptions owns the weak collection proxy and child proxies for one group instance, releases old collection/child subscriptions on reassignment and mutation, and avoids affecting unrelated TransformCollection consumers.

try-fix-1 is viable under the focused tests, but it changes the responsibility of TransformCollection itself. That means every collection now observes every child transform, even when no TransformGroup needs that aggregate invalidation. The broader blast radius is not justified when the leak can be fixed inside TransformGroup.

try-fix-2 is also viable under the focused tests, but the static weak registry adds lifecycle complexity, pruning requirements, global state, and dispatch indirection. It is harder to maintain and reason about than per-instance weak proxies.

try-fix-3 is not viable because it fails the regression suite. Its direct child subscriptions leave the child-transform retention path intact.

Winning candidate

Winner: pr

The submitted PR fix is the best balance of correctness, test coverage, and blast-radius control. It passes the supplied regression gate, the expert reviewer found no actionable defects, and the passing try-fix alternatives are broader or more complex without improving behavior.


🧭 Next Steps — review latest findings

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

@kubaflo

kubaflo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo)

@MauiBot Review 4707195586 passed the gate on f5cb57d, reports 0 errors, 0 warnings, and 0 suggestions, and ranks the submitted PR as the best candidate. The null collection/item lifecycle findings are confirmed fixed. No code change or redundant rerun is warranted; the [leak-fix] prefix remains intentional for this coordinated PR fleet.

@kubaflo

kubaflo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

@kubaflo

kubaflo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated merge-readiness analysis (GitHub Copilot CLI, on behalf of @kubaflo)

Good for merge from the automated CI perspective. The completed failures are unrelated to the TransformGroup.Children changes:

No failure implicates TransformGroup.cs or its regression tests, so no PR code change is required.

@kubaflo

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor Author

Tests Failure Analysis

@kubaflo — test-failure review results are available based on commit f5cb57d.
To request a fresh review after new comments, commits, or CI runs, comment /review tests.

Overall Not ready Failures 6 Regressed vs base 2 Baseline 0 on base

Test Failure Review: Not ready - click to expand

Overall verdict: Not ready — 2 build legs are deterministic regressions vs main (red on the PR, green across all 5 recently sampled base builds and red on none), and 4 further failures could not be attributed either way. 0 of 6 distinct failures reproduce on the base branch, so none are dismissable as pre-existing.

  • ✗ PR-related — deterministic build-break regressions (~2 legs): both are red on the PR but green on every sampled base build, e.g. DeviceTestsWindows (Windows) - build error (plus install Gradle init script (Linux/macOS) - build error).
  • i Uncertain — UI-test / build-leg failures needing a human (~4 tests + legs): flaky-on-base or unattributed UI failures such as ThumbImageSourceSizeIsCorrect and the two Picker* tests, alongside 5 unexplained build legs, 1 cancelled Material3 build check, and 7 device-test checks whose Failed==0 could not be positively confirmed.
  • ● Unrelated — pre-existing / known-issue failures (~0 tests): none — no PR failure reproduced on the base branch.

Coverage: 161 checks · 155 passing · 6 failing · 0 pending · 0 inaccessible · 1 unmapped · 5 unexplained build legs · 0 unaccounted failing checks · 1 aborted failing checks · 0 canceled-build checks · 7 device-test unverified · 4 unattributed · 2 regressed-vs-base. Deterministic ceiling: Not ready — 2 legs regressed vs base plus unexplained legs, an aborted check, unconfirmed device-test greens, and unattributed failures.

Builds (this PR): maui-pr-devicetests 1511233, maui-pr-uitests 1511231. Base sampling (main, 5 recent builds per definition): devicetests 1503431, 1503150; uitests 1503618, 1503332.

Recommended action

Have a human inspect the 2 regressed-vs-base build legs (Windows device tests and the Gradle init-script step) and confirm the 7 unverified device-test greens before merging.

@kubaflo

kubaflo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated follow-up analysis (GitHub Copilot CLI, on behalf of @kubaflo)

The /review tests two “deterministic build regressions” are wrapper-level misclassifications; no TransformGroup code change is required.

Main build 1511229 succeeded and all ten new TransformGroupMemoryTests passed. No failure overlaps the changed TransformGroup code, so the previous merge-readiness verdict remains valid.

Rebased onto inflight/current, which already contains #36150's child-subscription
fix using strong CollectionChanged/PropertyChanged subscriptions — the exact
mechanism #36367 reports (a shared/long-lived TransformCollection roots the
TransformGroup). Converts those subscriptions to WeakNotifyCollectionChangedProxy /
WeakNotifyPropertyChangedProxy, preserving #36150's Clear()/Reset teardown
(guarded by Shapes/TransformGroupTests).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e27685d0-fe80-460a-aa05-83d2ab9bf032
@kubaflo

kubaflo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🔍 AI-generated (GitHub Copilot CLI, on behalf of @kubaflo)

Rebased onto inflight/current, which already contains #36150's child-subscription fix using strong CollectionChanged/PropertyChanged subscriptions — the exact mechanism #36367 reports (a shared/long-lived TransformCollection roots the TransformGroup). This converts those to WeakNotifyCollectionChangedProxy / WeakNotifyPropertyChangedProxy, preserving #36150's Clear()/Reset teardown (guarded by Shapes/TransformGroupTests). Verified locally: 14 TransformGroup tests pass (base guard + new memory tests). Force-pushed as one clean commit.

@kubaflo

kubaflo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

@kubaflo

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentic-workflows perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-gate-passed AI verified tests catch the bug (fail without fix, pass with fix) s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

4 participants