[leak-fix] Fix IndicatorView.ItemsSource memory leak (Fixes #35775) - #36252
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36252Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36252" |
) Replace the non-weak CollectionChanged subscription in ResetItemsSource with a WeakNotifyCollectionChangedProxy, and add a finalizer that unsubscribes, so a shared/long-lived collection no longer roots every IndicatorView it is assigned to. Adds a Controls.Core.UnitTests regression test that fails without this fix (mirrors the existing BindableLayoutController weak-proxy pattern). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
714c3f3 to
ccf385e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Expert Review — 1 findings
See inline comments for details.
| ~override Microsoft.Maui.Controls.Handlers.Items2.StructuredItemsViewController2<TItemsView>.NumberOfSections(UIKit.UICollectionView collectionView) -> nint | ||
| ~override Microsoft.Maui.Controls.RadioButton.OnPropertyChanged(string propertyName = null) -> void | ||
| override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void | ||
| override Microsoft.Maui.Controls.Handlers.Items2.StructuredItemsViewController2<TItemsView>.UpdateFlowDirection() -> void |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[moderate] Public API Surface — This adds a duplicate StructuredItemsViewController2<TItemsView>.UpdateFlowDirection() baseline entry; the same signature is already present in this file at line 6. The same accidental duplicate appears in net-maccatalyst/PublicAPI.Unshipped.txt. Please remove these unrelated duplicates and leave only the IndicatorView.~IndicatorView() entries for this fix.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@kubaflo — new AI review results are available based on this last commit:
9e5729f. 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: 9a71eb6e
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 IndicatorViewTests IndicatorViewTests |
✅ FAIL — 155s | ✅ PASS — 125s |
🔴 Without fix — 🧪 IndicatorViewTests: FAIL ✅ · 155s
Determining projects to restore...
Restored /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj (in 6.47 sec).
Restored /home/vsts/work/1/s/src/TestUtils/src/TestUtils/TestUtils.csproj (in 102 ms).
Restored /home/vsts/work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 257 ms).
Restored /home/vsts/work/1/s/src/Essentials/src/Essentials.csproj (in 255 ms).
Restored /home/vsts/work/1/s/src/Controls/src/Xaml/Controls.Xaml.csproj (in 10.29 sec).
Restored /home/vsts/work/1/s/src/Core/src/Core.csproj (in 120 ms).
Restored /home/vsts/work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 29 ms).
Restored /home/vsts/work/1/s/src/Core/maps/src/Maps.csproj (in 837 ms).
Restored /home/vsts/work/1/s/src/Controls/Maps/src/Controls.Maps.csproj (in 731 ms).
1 of 10 projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
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.90-ci+azdo.14599358
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.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.27] Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.31] Discovered: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.34] Starting: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:04.02] IndicatorViewItemsSourceDoesNotLeak [FAIL]
[xUnit.net 00:00:04.03] IndicatorView should not be alive!
[xUnit.net 00:00:04.03] Stack Trace:
[xUnit.net 00:00:04.03] /_/src/Controls/tests/Core.UnitTests/IndicatorViewLayoutTests.cs(86,0): at Microsoft.Maui.Controls.Core.UnitTests.IndicatorViewTests.IndicatorViewItemsSourceDoesNotLeak()
[xUnit.net 00:00:04.03] --- End of stack trace from previous location ---
Passed IndicatorLayout_ShouldBeRemovedWhenIndicatorTemplateIsNulled [93 ms]
Passed IndicatorStackLayoutNoItems_ResetIndicators_ShouldHaveNoChildren [9 ms]
Passed IndicatorStackLayout_ResetIndicatorCount_ShouldBindChildren(oldCount: 1, expected: 2) [15 ms]
Passed IndicatorStackLayout_ResetIndicatorCount_ShouldBindChildren(oldCount: -2, expected: 2) [1 ms]
Passed IndicatorStackLayout_ResetIndicatorCount_ShouldBindChildren(oldCount: 0, expected: 2) [< 1 ms]
[xUnit.net 00:00:04.05] Finished: Microsoft.Maui.Controls.Core.UnitTests
Failed IndicatorViewItemsSourceDoesNotLeak [1 s]
Error Message:
IndicatorView should not be alive!
Stack Trace:
at Microsoft.Maui.Controls.Core.UnitTests.IndicatorViewTests.IndicatorViewItemsSourceDoesNotLeak() in /_/src/Controls/tests/Core.UnitTests/IndicatorViewLayoutTests.cs:line 86
--- End of stack trace from previous location ---
Passed IndicatorStackLayoutWithItems_ResetIndicators_ShouldBindChildren [7 ms]
Test Run Failed.
Total tests: 7
Passed: 6
Failed: 1
Total time: 4.8283 Seconds
🟢 With fix — 🧪 IndicatorViewTests: PASS ✅ · 125s
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.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.90-ci+azdo.14599358
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14599358
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.25] Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.47] Discovered: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.49] Starting: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.77] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed IndicatorLayout_ShouldBeRemovedWhenIndicatorTemplateIsNulled [90 ms]
Passed IndicatorStackLayoutNoItems_ResetIndicators_ShouldHaveNoChildren [9 ms]
Passed IndicatorStackLayout_ResetIndicatorCount_ShouldBindChildren(oldCount: 1, expected: 2) [16 ms]
Passed IndicatorStackLayout_ResetIndicatorCount_ShouldBindChildren(oldCount: -2, expected: 2) [1 ms]
Passed IndicatorStackLayout_ResetIndicatorCount_ShouldBindChildren(oldCount: 0, expected: 2) [1 ms]
Passed IndicatorViewItemsSourceDoesNotLeak [55 ms]
Passed IndicatorStackLayoutWithItems_ResetIndicators_ShouldBindChildren [1 ms]
Test Run Successful.
Total tests: 7
Passed: 7
Total time: 3.7468 Seconds
📁 Fix files reverted (8 files)
src/Controls/src/Core/IndicatorView/IndicatorView.cssrc/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txtsrc/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txtsrc/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txtsrc/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txtsrc/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txtsrc/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txtsrc/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt
📱 UI Tests — CarouselView,IndicatorView
Detected UI test categories: CarouselView,IndicatorView
[!WARNING]
No UI test results were produced for the detected categories. The platform-pool run
returned no results — most often because the PR build failed (see the Gate section) or
the deep UI test stage was skipped. Fix the build/gate issues and comment/review rerun
to get UI test results.
📋 Pre-Flight — Context & Validation
Issue: #35775 - IndicatorView leaks when CarouselView.IndicatorView is bound to a shared ObservableCollection
PR: #36252 - [leak-fix] Fix IndicatorView.ItemsSource memory leak (Fixes #35775)
Platforms Affected: Android, iOS, macOS/Mac Catalyst; managed Controls change applies cross-platform
Files Changed: 8 implementation/API, 1 test
Key Findings
- Root cause:
IndicatorView.ItemsSourcesubscribed toINotifyCollectionChanged.CollectionChangedwith an instance handler; a rooted sharedObservableCollection<T>could strongly retain discardedIndicatorViewinstances. - PR fix replaces the strong subscription with
WeakNotifyCollectionChangedProxy, keeps the handler delegate alive in an instance field, and adds a finalizer to unsubscribe the proxy. - Regression coverage is a Controls unit memory test (
IndicatorViewItemsSourceDoesNotLeak); no UI test category is required beyond possibleIndicatorViewimpact tracking.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ℹ No code-level issues found. Confidence is capped because
gh pr checks --requiredis unavailable without authentication and public check state was pending/undetermined.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36252 | Replace direct CollectionChanged subscription with WeakNotifyCollectionChangedProxy; add finalizer cleanup and PublicAPI entries |
✅ PASSED (Gate) | IndicatorView.cs, PublicAPI files, unit test |
Original PR |
🔬 Code Review — Deep Analysis
Code Review — PR #36252
Independent Assessment
What this changes: Replaces IndicatorView.ItemsSource's strong CollectionChanged subscription with WeakNotifyCollectionChangedProxy, stores the handler delegate strongly on the IndicatorView, and adds a finalizer to unsubscribe the proxy. Adds a memory regression test and PublicAPI entries for the finalizer.
Inferred motivation: Prevent long-lived observable item sources from retaining discarded IndicatorView instances through event subscriptions.
Reconciliation with PR Narrative
Author claims: Fixes IndicatorView.ItemsSource memory leak for shared/rooted ObservableCollection, following the existing BindableLayoutController weak-proxy pattern, with regression coverage.
Agreement/disagreement: Agrees. The code matches the described root cause and mirrors the established WeakNotifyCollectionChangedProxy usage pattern in BindableLayoutController.
Prior Review Reconciliation
No prior ❌ Error findings found. Checked top-level reviews, inline review comments, and issue comments via public GitHub API. Existing MauiBot reviews reported no error-level findings; ❌ FAILS references were baseline test-verification notes, not unresolved review findings.
Blast Radius Assessment
- Runs for all instances: Only
IndicatorViewinstances whoseItemsSourceimplementsINotifyCollectionChanged. - Startup impact: No.
- Static/shared state: No new static/shared state.
- Lifecycle impact: Adds a finalizer and weak event proxy lifecycle for observable
ItemsSource.
CI Status
- Required-check result:
gh pr checks --requiredunavailable due missing auth; public commit checks show multiplemaui-prchecks stillin_progress/ pending. - Classification: undetermined / pending.
- Action taken: Invoked
azdo-build-investigator; requiredci-analysisskill was unavailable. Confidence capped low; verdict cannot be LGTM while CI is pending/undetermined.
Findings
No ❌ Error findings.
No
No 💡 Suggestion findings.
Failure-Mode Probing
- Non-observable
ItemsSource: No proxy subscription is created; reset still recomputes count safely. - Reassigning observable sources: Old observable source causes
_collectionChangedProxy.Unsubscribe()before subscribing the new source. ItemsSource = null:OnCollectionChanged(null, Reset)setsCountto 0 without null dereference.- Delegate lifetime:
_collectionChangedEventHandlerkeeps the handler alive while the view lives; proxy holds it weakly to avoid rooting the view. - Post-GC collection changes: If handler target is gone, proxy unsubscribes itself on the next event; finalizer also unsubscribes.
Verdict: NEEDS_DISCUSSION
Confidence: low, due pending/undetermined CI per skill rules.
Summary: The implementation looks correct and follows an existing MAUI weak-event pattern. I found no code-level issues, but CI has not been verified as passing, so this cannot be marked LGTM yet.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 | Lifecycle-scoped strong subscription via OnParentSet; subscribe only while parented and resync on reparent |
✅ PASS | IndicatorView.cs, PublicAPI files |
Not selected: changes unparented update semantics and still adds API entries for the protected override. |
| 2 | try-fix-2 | Private static weak listener registry keyed by collection with static event handler and weak IndicatorView references |
✅ PASS | IndicatorView.cs |
Not selected: avoids finalizer/API entries, but introduces bespoke static state, locking, and opportunistic cleanup; higher maintenance risk than existing MAUI weak proxy. |
| PR | PR #36252 | Use existing WeakNotifyCollectionChangedProxy, retain handler delegate in field, and finalizer-unsubscribe proxy |
✅ PASSED (Gate) | IndicatorView.cs, PublicAPI files, unit test |
Selected: preserves existing semantics and reuses established MAUI pattern. |
Cross-Pollination
| Model/Reviewer | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Suggested lifecycle-scoped strong subscription; implemented as try-fix-1. |
| maui-expert-reviewer | 2 | Yes | Suggested static weak listener registry; implemented as try-fix-2. |
| maui-expert-reviewer | 3 | No | Reported design space exhausted: weak proxy, lifecycle subscription, static registry, no-subscription binding, and manual cleanup/API variants were either already tested or inferior/trivial. |
Exhausted: Yes
Selected Fix: PR's fix — it is the best option among tested candidates because it preserves IndicatorView.ItemsSource collection-change behavior, avoids bespoke global/static listener state, and follows the existing WeakNotifyCollectionChangedProxy pattern already used in MAUI Controls.
Candidate Details
try-fix-1
- Approach: Subscribe strongly only while the
IndicatorViewhas a parent; unsubscribe when unparented; resync on reparent. - Test:
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~IndicatorView" --logger "console;verbosity=minimal" - Result: ✅ PASS after adding required PublicAPI entries for
override IndicatorView.OnParentSet(). - Failure/selection analysis: Not selected because it changes behavior for unparented observable
ItemsSourceinstances and still requires public API entries. - Diff:
CustomAgentLogsTmp/PRState/36252/PRAgent/try-fix-1/fix.diff - Test output:
CustomAgentLogsTmp/PRState/36252/PRAgent/try-fix-1/test-output.log
try-fix-2
- Approach: Use a private static weak listener registry (
ConditionalWeakTable) and static collection-change handler to avoid collection -> IndicatorView strong references. - Test:
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~IndicatorView" --logger "console;verbosity=minimal" - Result: ✅ PASS.
- Failure/selection analysis: Not selected because the bespoke static registry is more complex and riskier than reusing MAUI's existing weak-event proxy pattern; dead listener cleanup is opportunistic.
- Diff:
CustomAgentLogsTmp/PRState/36252/PRAgent/try-fix-2/fix.diff - Test output:
CustomAgentLogsTmp/PRState/36252/PRAgent/try-fix-2/test-output.log
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the implementation is accurate, but the description should mention the reviewer cleanup that removes unrelated duplicate iOS/MacCatalyst PublicAPI entries.
Recommended title
[Controls] IndicatorView: Fix ItemsSource memory leak (Fixes #35775)
Recommended description
> [!NOTE]
> 🤖 **AI-generated PR.** This fix and its regression test were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging.
Fixes #35775
### The leak
`IndicatorView.ItemsSource` subscribed the `IndicatorView` to the assigned collection with a plain, non-weak delegate — `collection.CollectionChanged += OnCollectionChanged` in `ResetItemsSource` (`src/Controls/src/Core/IndicatorView/IndicatorView.cs`). Because `OnCollectionChanged` is an instance method, the delegate `Target` is the `IndicatorView`, and the only `-=` ran when `ItemsSource` itself changed — never on unload/navigation. The documented usage is to bind the **same long-lived collection as a `CarouselView`**, so that shared `ObservableCollection` strongly retained every `IndicatorView` attached to it (and its visual subtree / behaviors) for the collection's whole lifetime.
### The fix
Replace the plain subscription with a `WeakNotifyCollectionChangedProxy` stored in a field, and add a finalizer that calls `Unsubscribe()`. This mirrors the **existing `BindableLayoutController` pattern** (`src/Controls/src/Core/BindableLayout/BindableLayout.cs`): the collection now references only the proxy, which holds the `IndicatorView` weakly, so it can be collected once unreferenced; the handler delegate is kept alive in a field while the `IndicatorView` lives, and the finalizer tears the proxy down from the still-alive collection.
The final candidate also removes unrelated duplicate `StructuredItemsViewController2<TItemsView>.UpdateFlowDirection()` entries from the iOS and MacCatalyst `PublicAPI.Unshipped.txt` files, leaving only the intended `IndicatorView.~IndicatorView()` API baseline additions for this fix.
### Regression test
`IndicatorViewTests.IndicatorViewItemsSourceDoesNotLeak` in `src/Controls/tests/Core.UnitTests/IndicatorViewLayoutTests.cs`: assigns a long-lived `ObservableCollection` to a new `IndicatorView`, drops the only strong reference, forces GC, and asserts the view is collected via `WeakReference.WaitForCollect()`.
**Verified locally against this PR's base (`main`), `Controls.Core.UnitTests`:**
| State | Result |
|---|---|
| Without fix (current `main`) | ❌ FAILS — `IndicatorView should not be alive!` |
| With fix | ✅ `Passed IndicatorViewItemsSourceDoesNotLeak` |
### Scope
Managed cross-platform Controls change → all platforms. The only intended public-API addition is the finalizer, recorded in the per-TFM `PublicAPI.Unshipped.txt` files.
🏁 Report — Final Recommendation
Comparative Analysis — PR #36252
Candidates evaluated
| Candidate | Approach | Regression result | Rank |
|---|---|---|---|
pr-plus-reviewer |
PR weak-proxy fix plus removal of duplicate unrelated iOS/MacCatalyst PublicAPI entries | ✅ PASS | 1 |
pr |
Replace direct CollectionChanged subscription with WeakNotifyCollectionChangedProxy, retain delegate, finalizer-unsubscribe proxy |
✅ PASS | 2 |
try-fix-2 |
Static weak listener registry keyed by collection with weak IndicatorView references |
✅ PASS | 3 |
try-fix-1 |
Subscribe strongly only while parented; unsubscribe on unparent | ✅ PASS | 4 |
Analysis
pr-plus-reviewer is the best candidate. It preserves the PR's core weak-proxy implementation, which fixes the collection-to-IndicatorView strong reference cycle without changing ItemsSource update semantics, and it applies the expert reviewer's concrete cleanup by removing duplicate unrelated PublicAPI entries from the iOS and MacCatalyst API baselines.
The raw pr candidate has the same correct leak fix and passing regression result, but it leaves accidental duplicate StructuredItemsViewController2<TItemsView>.UpdateFlowDirection() entries in two PublicAPI files. That is unrelated to the IndicatorView leak fix and should not ship as part of this PR.
try-fix-2 also passes and avoids finalizer/PublicAPI additions, but it introduces a bespoke static registry, locking, listener pruning, and opportunistic cleanup. That is a broader maintenance surface than the existing MAUI WeakNotifyCollectionChangedProxy pattern already documented and used in Controls.
try-fix-1 passes but changes behavior: an unparented IndicatorView with an observable ItemsSource stops receiving collection updates until it is parented again. It also still requires PublicAPI entries for the protected OnParentSet() override. Because all candidates passed regression tests, ranking is determined by correctness, semantic preservation, and maintenance risk.
Winner
Winner: pr-plus-reviewer
The winning fix is the PR implementation with the expert reviewer cleanup applied. It fixes the leak with the established weak-event proxy pattern, preserves existing ItemsSource behavior, avoids new global/static listener state, and removes unrelated duplicate PublicAPI baseline entries.
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
Copilot re-review (provenance): the fixer de-dup/close gates (a/b/c/d) and the hunter fixed-on-main set searched by the '[leak-fix]' title substring only, so a manually-created PR titled [leak-fix] (not workflow-owned) could false-match — blocking a real leak, inflating the attempt cap, or (worst) closing a [leak-scan] issue. Scope all six [leak-fix] PR searches to label:agentic-workflows (the label this workflow's create-pull-request output applies), so only workflow-owned fixes count as provenance. [leak-scan] issue searches are unchanged. Empirically verified: 'gh pr list --search "[leak-fix]" in:title label:agentic-workflows' returns only the 3 genuine workflow PRs, excluding non-workflow [leak-fix]-titled PRs (e.g. #36252/#35417/#22673) that the unfiltered search matched. Also (consistency, from the same review): - Step 3 intro parenthetical no longer says a green-on-unpatched-main test means 'already fixed' — aligned with Step 6 (green => no PR, but not proof, no auto-close; closure only via a merged fix PR / gate (d)). - Added provenance notes to the gate (d) and hunter fixed-on-main comments. Prompt-only + description text: gh aw v0.80.9 recompile 0 errors/0 warnings; fixer lock changes only WORKFLOW_DESCRIPTION text + body_hash (no permission/tool drift), hunter lock body_hash-only, actions-lock.json unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 767d2d21-494b-41fb-8e8b-4410f9cc1b4b
…36252) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! > [!NOTE] > 🤖 **AI-generated PR.** This fix and its regression test were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging. Fixes #35775 ### The leak `IndicatorView.ItemsSource` subscribed the `IndicatorView` to the assigned collection with a plain, non-weak delegate — `collection.CollectionChanged += OnCollectionChanged` in `ResetItemsSource` (`src/Controls/src/Core/IndicatorView/IndicatorView.cs`). Because `OnCollectionChanged` is an instance method, the delegate `Target` is the `IndicatorView`, and the only `-=` ran when `ItemsSource` itself changed — never on unload/navigation. The documented usage is to bind the **same long-lived collection as a `CarouselView`**, so that shared `ObservableCollection` strongly retained every `IndicatorView` attached to it (and its visual subtree / behaviors) for the collection's whole lifetime. ### The fix Replace the plain subscription with a `WeakNotifyCollectionChangedProxy` stored in a field, and add a finalizer that calls `Unsubscribe()`. This mirrors the **existing `BindableLayoutController` pattern** (`src/Controls/src/Core/BindableLayout/BindableLayout.cs`): the collection now references only the proxy, which holds the `IndicatorView` weakly, so it can be collected once unreferenced; the handler delegate is kept alive in a field while the `IndicatorView` lives, and the finalizer tears the proxy down from the still-alive collection. ### Regression test `IndicatorViewTests.IndicatorViewItemsSourceDoesNotLeak` in `src/Controls/tests/Core.UnitTests/IndicatorViewLayoutTests.cs`: assigns a long-lived `ObservableCollection` to a new `IndicatorView`, drops the only strong reference, forces GC, and asserts the view is collected via `WeakReference.WaitForCollect()`. **Verified locally against this PR's base (`main`), `Controls.Core.UnitTests`:** | State | Result | |---|---| | Without fix (current `main`) | ❌ FAILS — `IndicatorView should not be alive!` | | With fix | ✅ `Passed IndicatorViewItemsSourceDoesNotLeak` | ### Scope Managed cross-platform change → all platforms. The only public-API addition is the finalizer, recorded in the per-TFM `PublicAPI.Unshipped.txt` files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…36252) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! > [!NOTE] > 🤖 **AI-generated PR.** This fix and its regression test were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging. Fixes #35775 ### The leak `IndicatorView.ItemsSource` subscribed the `IndicatorView` to the assigned collection with a plain, non-weak delegate — `collection.CollectionChanged += OnCollectionChanged` in `ResetItemsSource` (`src/Controls/src/Core/IndicatorView/IndicatorView.cs`). Because `OnCollectionChanged` is an instance method, the delegate `Target` is the `IndicatorView`, and the only `-=` ran when `ItemsSource` itself changed — never on unload/navigation. The documented usage is to bind the **same long-lived collection as a `CarouselView`**, so that shared `ObservableCollection` strongly retained every `IndicatorView` attached to it (and its visual subtree / behaviors) for the collection's whole lifetime. ### The fix Replace the plain subscription with a `WeakNotifyCollectionChangedProxy` stored in a field, and add a finalizer that calls `Unsubscribe()`. This mirrors the **existing `BindableLayoutController` pattern** (`src/Controls/src/Core/BindableLayout/BindableLayout.cs`): the collection now references only the proxy, which holds the `IndicatorView` weakly, so it can be collected once unreferenced; the handler delegate is kept alive in a field while the `IndicatorView` lives, and the finalizer tears the proxy down from the still-alive collection. ### Regression test `IndicatorViewTests.IndicatorViewItemsSourceDoesNotLeak` in `src/Controls/tests/Core.UnitTests/IndicatorViewLayoutTests.cs`: assigns a long-lived `ObservableCollection` to a new `IndicatorView`, drops the only strong reference, forces GC, and asserts the view is collected via `WeakReference.WaitForCollect()`. **Verified locally against this PR's base (`main`), `Controls.Core.UnitTests`:** | State | Result | |---|---| | Without fix (current `main`) | ❌ FAILS — `IndicatorView should not be alive!` | | With fix | ✅ `Passed IndicatorViewItemsSourceDoesNotLeak` | ### Scope Managed cross-platform change → all platforms. The only public-API addition is the finalizer, recorded in the per-TFM `PublicAPI.Unshipped.txt` files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Note
🤖 AI-generated PR. This fix and its regression test were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging.
Fixes #35775
The leak
IndicatorView.ItemsSourcesubscribed theIndicatorViewto the assigned collection with a plain, non-weak delegate —collection.CollectionChanged += OnCollectionChangedinResetItemsSource(src/Controls/src/Core/IndicatorView/IndicatorView.cs). BecauseOnCollectionChangedis an instance method, the delegateTargetis theIndicatorView, and the only-=ran whenItemsSourceitself changed — never on unload/navigation. The documented usage is to bind the same long-lived collection as aCarouselView, so that sharedObservableCollectionstrongly retained everyIndicatorViewattached to it (and its visual subtree / behaviors) for the collection's whole lifetime.The fix
Replace the plain subscription with a
WeakNotifyCollectionChangedProxystored in a field, and add a finalizer that callsUnsubscribe(). This mirrors the existingBindableLayoutControllerpattern (src/Controls/src/Core/BindableLayout/BindableLayout.cs): the collection now references only the proxy, which holds theIndicatorViewweakly, so it can be collected once unreferenced; the handler delegate is kept alive in a field while theIndicatorViewlives, and the finalizer tears the proxy down from the still-alive collection.Regression test
IndicatorViewTests.IndicatorViewItemsSourceDoesNotLeakinsrc/Controls/tests/Core.UnitTests/IndicatorViewLayoutTests.cs: assigns a long-livedObservableCollectionto a newIndicatorView, drops the only strong reference, forces GC, and asserts the view is collected viaWeakReference.WaitForCollect().Verified locally against this PR's base (
main),Controls.Core.UnitTests:main)IndicatorView should not be alive!Passed IndicatorViewItemsSourceDoesNotLeakScope
Managed cross-platform change → all platforms. The only public-API addition is the finalizer, recorded in the per-TFM
PublicAPI.Unshipped.txtfiles.