[leak-fix] Fix Picker.ItemsSource memory leak (Fixes #36272) - #36274
[leak-fix] Fix Picker.ItemsSource memory leak (Fixes #36272)#36274kubaflo wants to merge 146 commits into
Conversation
Route the ItemsSource CollectionChanged subscription through a WeakNotifyCollectionChangedProxy, torn down via a ~Picker finalizer, so a shared/long-lived collection no longer roots every Picker it is assigned to. Adds a Controls.Core.UnitTests regression test that fails without this fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36274Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36274" |
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
s/agent-review-in-progress
MauiBot
added
s/agent-gate-passed
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
removed
the
s/agent-review-in-progress
There was a problem hiding this comment.
Pull request overview
Fixes a managed memory-leak in Microsoft.Maui.Controls.Picker where a long-lived ItemsSource (e.g., a VM-owned ObservableCollection) could keep Picker instances alive via a strong CollectionChanged subscription.
Changes:
- Route
Picker.ItemsSourcecollection notifications throughWeakNotifyCollectionChangedProxyinstead of a direct event subscription. - Add a
~Picker()finalizer to ensure the weak proxy unsubscribes, preventing the proxy itself from being leaked by a long-lived collection. - Add a regression unit test
PickerItemsSourceDoesNotLeakunder the memory test category.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/src/Core/Picker/Picker.cs | Replaces strong CollectionChanged subscription with WeakNotifyCollectionChangedProxy and adds ~Picker() to unsubscribe on finalization. |
| src/Controls/tests/Core.UnitTests/PickerTests.cs | Adds a memory regression test ensuring a Picker is collectible even when its shared ItemsSource remains alive. |
| src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt | Records the new Picker finalizer in the netstandard API surface. |
| src/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txt | Records the new Picker finalizer in the net API surface. |
| src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt | Records the new Picker finalizer in the Android API surface. |
| src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt | Records the new Picker finalizer in the iOS API surface. |
| src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt | Records the new Picker finalizer in the MacCatalyst API surface. |
| src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt | Records the new Picker finalizer in the Windows API surface. |
| src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt | Records the new Picker finalizer in the Tizen API surface. |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo) @MauiBot Review No code change or no-op commit is warranted. The |
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
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.
Reconcile the force-rebased target while preserving only the validated Picker leak fix and regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bbcdaa9d-d75a-4e3b-b375-cf827a689644
|
🔍 AI-generated update (GitHub Copilot CLI, on behalf of @kubaflo): Resolved the conflicts against the current |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo) @MauiBot Review |
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🔍 AI-generated review follow-up (GitHub Copilot CLI, on behalf of @kubaflo) MauiBot review #36274 (review) was submitted against the current head The |
This comment has been minimized.
This comment has been minimized.
|
🔍 AI-generated merge-readiness analysis (GitHub Copilot CLI, on behalf of @kubaflo) The PR code is good for merge; CI is blocked by the broken The PR builds 1511588, 1511590, and 1511589 have the same failure set as independent builds of exact base commit The matching base failures are solution-wide No Picker code change is required. Green CI is impossible until |
Tests Failure Analysis
Test Failure Review: Not ready - click to expandOverall verdict: Not ready. Exactly one failure is a deterministic regression vs the base branch (
Coverage: 72 checks · 22 passing · 50 failing · 0 pending · 0 inaccessible · 1 unmapped · 73 unexplained build legs · 0 unaccounted failing checks · 0 aborted failing checks · 0 canceled-build checks · 3 device-test unverified · 172 unattributed · 1 regressed-vs-base. Deterministic ceiling: Not ready — 1 leg is a deterministic regression vs base ( Builds (this PR): maui-pr 1511588, maui-pr-devicetests 1511590, maui-pr-uitests 1511589. Base sampling (inflight/current, 5 recent builds per definition): 1511352, 1511355, 1511354. Recommended actionInvestigate the |
|
🔍 AI-generated follow-up analysis (GitHub Copilot CLI, on behalf of @kubaflo) The automated test-analysis comment’s
Five recent base passes do not turn a previously documented intermittent Appium lookup failure into a deterministic regression. This failure is unrelated to #36274; the earlier merge-readiness verdict remains unchanged. |
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
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:
91554e0. 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: inflight/current · Merge base: 431890bd
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 PickerTests PickerTests |
✅ FAIL — 160s | ✅ PASS — 127s |
🔴 Without fix — 🧪 PickerTests: FAIL ✅ · 160s
Error-relevant lines (filtered from the build log):
at InvokeStub_PickerTests.TestItemsSourceCollectionChangedRemoveAtEndSelected(Object, Span`1)
at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerItemsSourceReassignmentIgnoresOldCollectionChanges() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1322
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerRetainsSelectionWhenItemsAreInsertedBeforeAndAfterSelection() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1492
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerRetainsSelectionWhenSelectedItemIsMoved() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1546
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerReconcilesRemovedSelectionWhenHandlerReattaches() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1176
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.GetItemsSourceSubscriptionObjects(Picker picker) in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1390
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerItemsSourceClearReusesCollectionChangedSubscription() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1253
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerPreservesSelectionWhenItemInsertedBeforeItWhileHandlerDetached() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1200
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.GetItemsSourceSubscriptionReferences(Picker picker) in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1382
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.CreatePickerReferences(ObservableCollection`1 itemsSource) in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1352
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerItemsSourceDoesNotLeak() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1056
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerPreservesSelectedItemAfterInsertingItemBeforeSelection() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1046
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.TestItemsSourceCollectionChangedAppend() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 477
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.TestSelectedItemAssignedItemsSourceCollectionChangedRemove() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 827
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.SelectedItemSetAfterPendingSelectedIndexClearsPendingIndex() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 275
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerItemsSourceCollectionChangedHandlerIsLazy() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1333
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.TestSelectedIndexAssignedItemsSourceCollectionChangedAppend() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 696
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.CreateReusedPickerReferences(ObservableCollection`1 itemsSource) in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1366
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.ReusedPickerItemsSourceSubscriptionDoesNotLeak() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1278
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.TestItemsSourceCollectionChangedInsert() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 519
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.PickerItemsSourceChangesStillApplyAfterGc() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 1074
at Microsoft.Maui.Controls.Core.UnitTests.PickerTests.TestItemsSourceCollectionChangedRemove() in /_/src/Controls/tests/Core.UnitTests/PickerTests.cs:line 569
🟢 With fix — 🧪 PickerTests: PASS ✅ · 127s
(no coded error found; showing last 1200 chars)
ceClampsAfterItemsSource [< 1 ms]
Passed SelectedItemSetAfterPendingSelectedIndexClearsPendingIndex [< 1 ms]
Passed TestItemsSourceEnums [4 ms]
Passed PickerItemsSourceCollectionChangedHandlerIsLazy [< 1 ms]
Passed TestEmptyCollectionResetItems [< 1 ms]
Passed PickerResyncsNonObservableItemsSourceOnFirstHandlerAttach [< 1 ms]
Passed PickerRetainsSelectionWhenDuplicateSelectedItemIsRemoved [< 1 ms]
Passed SelectedIndexSetBeforeInlineItemsSurvivesHandlerAttachmentUntilIndexBecomesValid [114 ms]
Passed TestSetSelectedIndexOnNullRows [< 1 ms]
Passed TestSelectedIndexAssignedItemsSourceCollectionChangedAppend [< 1 ms]
Passed TestSelectedItemDefault [< 1 ms]
Passed ReusedPickerItemsSourceSubscriptionDoesNotLeak [34 ms]
Passed TestItemsSourceCollectionChangedInsert [1 ms]
Passed PickerItemsSourceChangesStillApplyAfterGc [28 ms]
Passed PendingSelectedIndexCanBeClearedBeforeItemsLoad [< 1 ms]
Passed TestUnsubscribeINotifyCollectionChanged [< 1 ms]
[xUnit.net 00:00:02.96] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed TestItemsSourceCollectionChangedRemove [< 1 ms]
Test Run Successful.
Total tests: 73
Passed: 73
Total time: 3.7711 Seconds
📁 Fix files reverted (1 files)
src/Controls/src/Core/Picker/Picker.cs
📱 UI Tests — Picker
Detected UI test categories: Picker
Picker) started (the HostApp built and dotnet test launched) but produced no results — the app failed to launch or the test host crashed before a .trx was written (an app-launch/Appium failure or an emulator flake). These categories exited quickly, well under their time budget, so raising the budget will not help — re-run the review to try again. If it recurs, check the build-output.log + logcat in the drop-deep-uitests artifact for an app-startup crash.
📋 Pre-Flight — Context & Validation
Issue: #36272 - Picker.ItemsSource: a shared/long-lived collection leaks every Picker via a non-weak CollectionChanged subscription (no teardown on unload)
PR: #36274 - [leak-fix] Fix Picker.ItemsSource memory leak (Fixes #36272)
Platforms Affected: all platforms (managed cross-platform Controls code); requested test platform: android, but regression coverage is Controls.Core.UnitTests
Files Changed: 1 implementation, 1 test
Key Findings
Picker.ItemsSourcepreviously subscribed directly toINotifyCollectionChanged.CollectionChanged, so a long-lived/shared observable collection could strongly retain each assignedPickeruntil reassignment or collection disposal.- The PR's current fix routes the observable subscription through a private weak-proxy helper and adds memory/lifecycle unit tests for leak, reassignment, handler detach/reattach, and selection reconciliation.
- GitHub CLI is unauthenticated in this environment, so PR metadata came from the public GitHub API and required-check classification remains undetermined.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ℹ No actionable current code defects were found by the code-review pass.
- ℹ Prior error-level findings seen in PR discussion appear addressed in the current two-file diff: no PublicAPI changes remain, handler reattach reconciles selection, and pending
SelectedIndexis mapped after item rebuild. - ℹ CI/required checks could not be classified because
ghis unauthenticated; confidence is capped low.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36274 | Private lazily-created ItemsSourceCollectionChangedSubscription owns a WeakNotifyCollectionChangedProxy, unsubscribes deterministically on source changes/detach, and finalizes stale subscriptions so the source does not root Picker. |
✅ PASSED (Gate) | src/Controls/src/Core/Picker/Picker.cs, src/Controls/tests/Core.UnitTests/PickerTests.cs |
Original PR; gate supplied by caller. |
🔬 Code Review — Deep Analysis
Code Review — PR #36274
Independent Assessment
What this changes: Picker.ItemsSource now uses a private weak CollectionChanged subscription helper so long-lived observable sources do not keep Picker instances alive. It also adjusts handler attach/detach resync behavior and adds Picker unit/memory tests.
Inferred motivation: Fix a memory leak while preserving item/selection synchronization across handler lifecycle transitions.
Reconciliation with PR Narrative
Author claims: Fixes #36272 via a lazy weak-proxy helper, no public API change, with regression coverage for leak, reassignment, detach/reattach, and selection reconciliation.
Agreement/disagreement: The claims match the current two-file diff.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
| Duplicate PublicAPI entries / public finalizer mismatch | Copilot inline | ✅ Fixed | Current diff only changes Picker.cs and PickerTests.cs; no PublicAPI files. |
| Detached handler reattach could select stale item | MauiBot [major] |
✅ Fixed | ResyncItemsAndReconcileSelection() is used on reattach; tests cover removed selected item at PickerTests.cs:1156-1178. |
Pending SelectedIndex not mapped after Items update |
MauiBot [major] |
✅ Fixed/covered | PickerAppliesPendingSelectedIndexWhenHandlerReattaches asserts mapper order at PickerTests.cs:1204-1244; stub mapper records updates at PickerTests.cs:1403-1423. |
Blast Radius Assessment
- Runs for all instances: Only
Pickerinstances withItemsSource; weak helper allocated only for observable sources. - Startup impact: No.
- Static/shared state: No.
CI Status
- Required-check result: unavailable/undetermined —
gh pr checks --requiredfailed because GitHub CLI is unauthenticated. - Public check-run result: failing snapshot via public API; failures not attributable from available public metadata.
- Classification: undetermined.
- Action taken: public API fallback; confidence capped low; no GitHub comments posted.
Findings
No high-confidence ❌ Error,
Failure-Mode Probing
- Shared collection outlives Picker: weak proxy/helper avoids rooting; memory tests cover this.
- Handler detaches then source mutates: subscription pauses, reattach resyncs and reconciles selection.
- ItemsSource reassignment: old source unsubscribed/collectible; replacement remains active.
- Non-observable source changes before first attach: first attach resyncs.
- Null/default source: subscribe path no-ops; clear path safely unlocks and clears.
Verdict: NEEDS_DISCUSSION
Confidence: low
Summary: Code review found no current actionable code defects, but CI is red/undetermined and required-check status is unavailable without gh auth. Skill rules prohibit LGTM until CI is green or failures are classified PR-unrelated.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | maui-expert-reviewer | Reuse existing ListProxy as the weak subscription owner for Picker.ItemsSource. |
❌ Fail | 1 implementation file | 9 passed / 3 failed. Failed because current PR regression tests intentionally assert the private helper/proxy shape. |
| 2 | maui-expert-reviewer | Use a static per-source weak broker keyed by INotifyCollectionChanged, with weak Picker subscribers and behavioral test rewrites. |
✅ Pass | 1 implementation file, 1 test file | 12 passed / 0 failed, but more complex/global and not demonstrably better than PR. |
| 3 | maui-expert-reviewer | Use direct strong CollectionChanged subscription scoped only to handler lifetime, with reattach resync. |
❌ Fail | 1 implementation file | 5 passed / 7 failed. Handlerless observable changes stale and lifecycle boundary is too weak. |
| 4 | maui-expert-reviewer | Store weak proxy directly on Picker and make Picker finalizable for cleanup. |
❌ Fail | 1 implementation file | Build failed with Public API analyzer RS0016 for Picker.~Picker(). |
| PR | PR #36274 | Private lazily-created ItemsSourceCollectionChangedSubscription owns WeakNotifyCollectionChangedProxy, finalizes cleanup, pauses on handler detach, and resyncs/reconciles on reattach. |
✅ PASSED (Gate) | 1 implementation file, 1 test file | Original PR; gate result supplied by caller. |
Cross-Pollination
| Model/Reviewer | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Suggested ListProxy ownership to reuse existing weak collection proxy infrastructure. |
| maui-expert-reviewer | 2 | Yes | After ListProxy failed, suggested per-source weak broker with weak Picker references. |
| maui-expert-reviewer | 3 | Yes | After broker passed but was not better, suggested handler-lifetime direct subscription as a simpler lifecycle-scoped alternative. |
| maui-expert-reviewer | 4 | Yes | Suggested direct Picker-owned weak-proxy finalization; testing confirmed PublicAPI/analyzer downside. |
| maui-expert-reviewer | 5 | No | Remaining meaningful axes were exhausted: proxy owner (ListProxy, broker, Picker, private helper) and lifetime boundary (weak event vs handler lifetime). |
Exhausted: Yes
Selected Fix: PR #36274 — It is the best candidate. It passes the supplied gate, keeps the fix localized to Picker, avoids static/global broker bookkeeping, preserves handlerless and detach/reattach synchronization semantics, and avoids adding a public Picker finalizer/API surface.
Candidate Notes
try-fix-1/content.mdrecords the ListProxy attempt and failure analysis.try-fix-2/content.mdrecords the broker attempt; it passed focused tests but required test rewrites and was not better.try-fix-3/content.mdrecords the handler-lifetime attempt and behavioral failures.try-fix-4/content.mdrecords the Picker-finalizer attempt and PublicAPI analyzer failure.
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the current description is accurate and thorough, but the title uses a workflow/noise prefix instead of the repo's [Scope] Component: What changed style.
Recommended title
[Controls] Picker: Fix ItemsSource memory leak
Recommended description
> [!NOTE]
> 🔍 **AI-generated PR.** This fix and its regression tests were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging.
Fixes #36272
### The leak
`Picker.ItemsSource` subscribes the `Picker` to the assigned collection `CollectionChanged` event with a plain, non-weak delegate (`src/Controls/src/Core/Picker/Picker.cs`), detaching only when `ItemsSource` is reassigned. A shared or long-lived collection, such as a view-model `ObservableCollection`, therefore strongly roots every `Picker` bound to it and, through it, the parent visual tree or page.
### The fix
Route the `ItemsSource` `CollectionChanged` subscription through a `WeakNotifyCollectionChangedProxy` owned by a private, lazily-created `ItemsSourceCollectionChangedSubscription` helper. The helper finalizer unsubscribes the proxy when the owner is collected, so a shared collection no longer roots the `Picker` without adding a public `Picker` finalizer or changing the public API surface. The same helper and proxy are reused across clear, reassignment, and handler detach/reattach cycles; deterministic unsubscribe suppresses finalization while inactive, and the next subscription re-registers it.
Collection updates remain active before the first handler and pause only after explicit handler detachment. The first handler attach avoids a redundant rebuild for observable sources, while non-observable sources are still refreshed in case they changed in place. Reattachment rebuilds display items through selected-item-aware reconciliation, preserving a selected item that still exists and clearing selection when the selected item was removed while detached instead of selecting the item now occupying its stale index. Reattachment also applies a pending `SelectedIndex` before normal reconciliation when an empty source is populated while detached; mapper-order coverage verifies the automatic bindable-property update sends `SelectedIndex` after `Items`.
### Regression tests
- `PickerItemsSourceDoesNotLeak` verifies the `Picker`, subscription helper, and weak proxy are collected while the shared collection remains alive.
- `PickerItemsSourceChangesStillApplyAfterGc` verifies a live `Picker` continues processing observable collection changes after forced GC.
- `PickerItemsSourceClearReusesCollectionChangedSubscription` verifies clearing and replacing `ItemsSource` intentionally reuses the same detached helper and proxy, exercising finalizer suppression/re-registration while ignoring the old source.
- `ReusedPickerItemsSourceSubscriptionDoesNotLeak` verifies a helper re-registered after reuse still finalizes and detaches when its `Picker` is collected.
- `PickerItemsSourceChangesPauseWhileHandlerIsDetached` verifies updates pause after explicit handler detachment and resynchronize on reattach.
- `PickerDoesNotResyncObservableItemsSourceOnFirstHandlerAttach` verifies the first attach does not redundantly rebuild an already synchronized observable source.
- `PickerResyncsNonObservableItemsSourceOnFirstHandlerAttach` preserves refresh behavior for mutable non-observable sources.
- `PickerItemsSourceReplacementWhileHandlerIsDetachedUsesReplacementOnReattach` verifies replacing the source while detached ignores the old source, catches up with changes to the replacement on reattach, and continues observing only the replacement.
- `PickerReconcilesRemovedSelectionWhenHandlerReattaches` verifies removing the selected item while detached clears selection on reattach rather than selecting the stale index.
- `PickerPreservesSelectionWhenItemInsertedBeforeItWhileHandlerDetached` verifies inserting an item before the selected item while detached preserves `SelectedItem` and shifts `SelectedIndex` on reattach.
- `PickerAppliesPendingSelectedIndexWhenHandlerReattaches` verifies a pending index is applied after an empty source receives items while detached and that the handler maps `Items` before the resolved `SelectedIndex`.
- `PickerItemsSourceReassignmentMovesCollectionChangedSubscription` verifies reassignment releases the old source while the replacement remains active.
- `PickerItemsSourceReassignmentIgnoresOldCollectionChanges` verifies old source mutations are ignored and replacement source mutations still update `Picker.Items`.
- `PickerItemsSourceCollectionChangedHandlerIsLazy` verifies the cached handler is allocated only for observable sources.
All GC-sensitive helper methods use `[MethodImpl(MethodImplOptions.NoInlining)]` to keep lifetime assertions deterministic.
**Verified locally against this PR base (`inflight/current`), `Controls.Core.UnitTests`:**
| State | Result |
|---|---|
| Without fix (current `inflight/current`) | ❌ FAIL — `Picker should not be alive!` |
| With fix | ✅ all 73 `PickerTests` passed |
### Scope
Managed cross-platform change for all platforms. No public API additions.
🏁 Report — Final Recommendation
Comparative Fix Report — PR #36274
Candidate ranking
| Rank | Candidate | Regression result | Assessment |
|---|---|---|---|
| 1 | pr |
✅ Gate passed | Best overall. Localized private helper/proxy design fixes the leak without public API changes, static brokers, or handler-lifetime assumptions, and preserves the strongest regression coverage. |
| 2 | pr-plus-reviewer |
Not rerun; test-only sandbox edits | Applies safe reviewer feedback, but only changes tests. Removing the identity assertions improves refactor tolerance while weakening intentional coverage of the suppress/re-register lifecycle invariant. No production benefit over pr. |
| 3 | try-fix-2 |
✅ Passed focused tests | Valid independent broker approach, but more complex and introduces static per-source bookkeeping. Self-review noted residual broker/weak-reference slots can remain for long-lived sources until explicit unsubscribe or pruning. |
| 4 | try-fix-1 |
❌ Failed focused tests | Uses ListProxy as weak subscription owner. Failed 3 regression tests and did not satisfy the current helper/proxy lifetime coverage. |
| 5 | try-fix-3 |
❌ Failed focused tests | Handler-lifetime strong subscription failed 7 regression tests and leaves handlerless observable updates stale. Handler lifetime is too weak a boundary for the managed ItemsSource mirror. |
| 6 | try-fix-4 |
❌ Build failed | Picker-owned weak proxy/finalizer failed Public API analyzer RS0016 for Picker.~Picker() and broadens finalization impact onto the public control type. |
Comparative analysis
The raw PR fix is the best candidate. It addresses the root cause directly: a shared or long-lived observable collection no longer strongly roots each Picker through a direct CollectionChanged subscription. The solution stays private to Picker, uses the existing WeakNotifyCollectionChangedProxy infrastructure, keeps the weak proxy delegate alive while the Picker is live, and avoids adding a public Picker finalizer or global/static subscription broker.
pr-plus-reviewer is useful as review feedback but not as the winning candidate. The expert findings are test-maintainability concerns, not production correctness defects. The applied sandbox changes add issue-link comments and remove one pair of private identity assertions, but the removed assertions were deliberately covering finalizer suppression/re-registration reuse. That tradeoff makes the candidate less protective for this specific leak fix.
try-fix-2 is the only non-PR candidate that passed focused tests, but it is less attractive than the PR: it introduces static shared broker state keyed by source collections and retains small weak-reference bookkeeping for long-lived sources until cleanup/pruning. The PR's per-picker private helper has a smaller blast radius and simpler ownership.
The remaining try-fix candidates failed regression tests or build validation and must rank below passing candidates. try-fix-1 did not satisfy the regression suite, try-fix-3 regressed handlerless update behavior, and try-fix-4 hit the public API analyzer with a public-control finalizer.
Winner
pr wins. It passed the supplied gate, has no high-confidence production-code findings from expert review, and is simpler and more localized than the passing alternative.
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
|
|
||
| // Implementation/lifecycle invariant: reuse exercises finalizer suppression and | ||
| // re-registration on the same helper instead of masking lifecycle bugs with a new proxy. | ||
| Assert.Same(originalReferences.Subscription, reusedReferences.Subscription); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[moderate] Regression Prevention and Test Coverage — Assert.Same(originalReferences.Subscription, reusedReferences.Subscription) / Assert.Same(originalReferences.Proxy, reusedReferences.Proxy) assert on the identity of private implementation objects (ItemsSourceCollectionChangedSubscription, WeakNotifyCollectionChangedProxy) rather than on observable behavior. The comment above (lines 1263-1264) acknowledges this is deliberate, but it still means a functionally-equivalent refactor (e.g., allocating a fresh subscription/proxy per ItemsSource reassignment instead of reusing one) would fail this test even though nothing user-visible changed. The reuse invariant this is guarding (exercising the GC.SuppressFinalize/GC.ReRegisterForFinalize cycle) is valuable, but consider expressing it as a behavioral assertion (e.g., verifying no additional allocation/leak across many reassign cycles) instead of Assert.Same on private fields, so the test survives internal refactors that preserve behavior.
| static (object Subscription, object Proxy) GetItemsSourceSubscriptionObjects(Picker picker) | ||
| { | ||
| const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Instance; | ||
| var subscriptionField = typeof(Picker).GetField("_itemsSourceCollectionChangedSubscription", flags); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[moderate] Regression Prevention and Test Coverage — This helper (and the standalone reflection lookup of _collectionChangedEventHandler in PickerItemsSourceCollectionChangedHandlerIsLazy, ~line 1332) uses BindingFlags.NonPublic to reach into Picker._itemsSourceCollectionChangedSubscription, Picker._collectionChangedEventHandler, and the nested subscription's private _proxy field. Four tests (PickerItemsSourceClearReusesCollectionChangedSubscription, ReusedPickerItemsSourceSubscriptionDoesNotLeak, PickerItemsSourceReassignmentMovesCollectionChangedSubscription, PickerItemsSourceCollectionChangedHandlerIsLazy) are coupled to these exact private field names via string literals. A future internal rename (e.g., renaming _proxy or inlining the subscription wrapper) with no behavioral change will break these tests only through the Assert.NotNull(...Field) guard rather than a meaningful behavioral assertion. Consider verifying the 'lazy allocation' and 'no leak' invariants purely through observable behavior (collection updates propagating to picker.Items, and WeakReference-based collection of the Picker/ItemsSource, which the memory tests already do) instead of reflecting into implementation-private fields.
| } | ||
|
|
||
| [Fact, Category(TestCategory.Memory)] | ||
| public async Task PickerItemsSourceDoesNotLeak() |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[minor] Regression Prevention and Test Coverage — None of the new regression tests added for this fix (this method through ~line 1340) reference the originating issue in a // https://github.com/dotnet/maui/issues/NNNNN comment, unlike the established convention elsewhere in this same file (see lines 1004, 1427, 1450, 1473, 1503, 1527) where fix-specific tests are annotated with the issue link. Adding that cross-reference to at least the primary leak-repro tests (PickerItemsSourceDoesNotLeak, PickerItemsSourceChangesStillApplyAfterGc) would help future contributors locate the original repro and avoid silently reintroducing the leak.
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 tests were produced by an AI-driven memory-leak workflow and verified locally. Please review carefully before merging.
Fixes #36272
The leak
Picker.ItemsSourcesubscribes thePickerto the assigned collectionCollectionChangedevent with a plain, non-weak delegate (src/Controls/src/Core/Picker/Picker.cs), detaching only whenItemsSourceis reassigned. A shared or long-lived collection, such as a view-modelObservableCollection, therefore strongly roots everyPickerbound to it and, through it, the parent visual tree or page.The fix
Route the
ItemsSourceCollectionChangedsubscription through aWeakNotifyCollectionChangedProxyowned by a private, lazily-createdItemsSourceCollectionChangedSubscriptionhelper. The helper finalizer unsubscribes the proxy when the owner is collected, so a shared collection no longer roots thePickerwithout adding a publicPickerfinalizer or changing the public API surface. The same helper and proxy are reused across clear, reassignment, and handler detach/reattach cycles; deterministic unsubscribe suppresses finalization while inactive, and the next subscription re-registers it.Collection updates remain active before the first handler and pause only after explicit handler detachment. The first handler attach avoids a redundant rebuild for observable sources, while non-observable sources are still refreshed in case they changed in place. Reattachment rebuilds display items through selected-item-aware reconciliation, preserving a selected item that still exists and clearing selection when the selected item was removed while detached instead of selecting the item now occupying its stale index. Reattachment also applies a pending
SelectedIndexbefore normal reconciliation when an empty source is populated while detached; mapper-order coverage verifies the automatic bindable-property update sendsSelectedIndexafterItems.Regression tests
PickerItemsSourceDoesNotLeakverifies thePicker, subscription helper, and weak proxy are collected while the shared collection remains alive.PickerItemsSourceChangesStillApplyAfterGcverifies a livePickercontinues processing observable collection changes after forced GC.PickerItemsSourceClearReusesCollectionChangedSubscriptionverifies clearing and replacingItemsSourceintentionally reuses the same detached helper and proxy, exercising finalizer suppression/re-registration while ignoring the old source.ReusedPickerItemsSourceSubscriptionDoesNotLeakverifies a helper re-registered after reuse still finalizes and detaches when itsPickeris collected.PickerItemsSourceChangesPauseWhileHandlerIsDetachedverifies updates pause after explicit handler detachment and resynchronize on reattach.PickerDoesNotResyncObservableItemsSourceOnFirstHandlerAttachverifies the first attach does not redundantly rebuild an already synchronized observable source.PickerResyncsNonObservableItemsSourceOnFirstHandlerAttachpreserves refresh behavior for mutable non-observable sources.PickerItemsSourceReplacementWhileHandlerIsDetachedUsesReplacementOnReattachverifies replacing the source while detached ignores the old source, catches up with changes to the replacement on reattach, and continues observing only the replacement.PickerReconcilesRemovedSelectionWhenHandlerReattachesverifies removing the selected item while detached clears selection on reattach rather than selecting the stale index.PickerPreservesSelectionWhenItemInsertedBeforeItWhileHandlerDetachedverifies inserting an item before the selected item while detached preservesSelectedItemand shiftsSelectedIndexon reattach.PickerAppliesPendingSelectedIndexWhenHandlerReattachesverifies a pending index is applied after an empty source receives items while detached and that the handler mapsItemsbefore the resolvedSelectedIndex.PickerItemsSourceReassignmentMovesCollectionChangedSubscriptionverifies reassignment releases the old source while the replacement remains active.PickerItemsSourceReassignmentIgnoresOldCollectionChangesverifies old source mutations are ignored and replacement source mutations still updatePicker.Items.PickerItemsSourceCollectionChangedHandlerIsLazyverifies the cached handler is allocated only for observable sources.All GC-sensitive helper methods use
[MethodImpl(MethodImplOptions.NoInlining)]to keep lifetime assertions deterministic.Verified locally against this PR base (
inflight/current),Controls.Core.UnitTests:inflight/current)Picker should not be alive!PickerTestspassedScope
Managed cross-platform change for all platforms. No public API additions.