Skip to content

Use weak events for SwipeView and ShellContent child subscriptions - #38415

Closed
wbv-profile wants to merge 1 commit into
dotnet:mainfrom
wbv-profile:fix/weak-events-content
Closed

wbv-profile wants to merge 1 commit into
dotnet:mainfrom
wbv-profile:fix/weak-events-content

Conversation

@wbv-profile

Copy link
Copy Markdown

Description of Change

SwipeView and ShellContent both subscribe to a child's PropertyChanged in OnChildAdded and unsubscribe in OnChildRemoved, using plain CLR handlers. A child can outlive the parent that hosted it — cached by the app, or re-parented onto a different host — and the handler then keeps every parent the child was ever attached to alive, along with that parent's whole subtree.

Both now route the subscription through the existing WeakNotifyPropertyChangedProxy, tracking one proxy per child so removal still detaches deterministically, plus the finalizer the proxy contract requires.

This is the pattern the surrounding code already reaches for. SwipeView.OnChildAdded carries a comment explaining that SwipeItems children are deliberately not subscribed for exactly this reason, and OnSwipeItemsChanged documents the earlier fix for #35481 where captured closures kept shared SwipeItems from being collected. This change extends the same reasoning to ordinary content children.

Same defect class as #38397, #38403, #38405, #38408, #38410 and #38412.

Tests

src/Controls/tests/Core.UnitTests/ContentWeakEventTests.cs adds four tests: one leak test per control (a retained Label assigned as SwipeView.Content, a retained ContentPage assigned as ShellContent.Content), plus two guards pinning that attached content is still parented and that removing it still detaches.

Reverting the src/Controls/src/Core changes fails exactly the two leak tests and passes both guards. With the fix all four pass and the full Controls.Core.UnitTests suite is green (5651 passed, 0 failed).

Issues Fixed

Fixes #37217
Fixes #37218

Both types subscribe to a child's PropertyChanged in OnChildAdded and
unsubscribe in OnChildRemoved, with plain CLR handlers. A child can
outlive the parent that hosted it -- cached by the app, or re-parented
onto a different host -- so the handler kept every parent the child was
ever attached to alive, and with it that parent's whole subtree.

Both now route the subscription through WeakNotifyPropertyChangedProxy,
tracking one proxy per child so removal still detaches deterministically,
plus the finalizer the proxy contract requires.

This extends reasoning the surrounding code already applies: SwipeView
deliberately skips SwipeItems children for the same reason, and
OnSwipeItemsChanged documents the earlier dotnet#35481 fix where captured
closures kept shared SwipeItems alive.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

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

Or

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

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service dotnet-policy-service Bot added the community ✨ Community Contribution label Sep 10, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Hey there @@wbv-profile! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Hey there @wbv-profile! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@kubaflo

kubaflo commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Closing as overlapping with #37390 (ShellContent, 26233a1) and #38101 (SwipeView, dba446a), both already merged into inflight/current.

The event-retention diagnosis is correct, but those existing changes already replace these strong child subscriptions. Their single lazy proxy per control also matches the single relevant content/page lifecycle without the dictionaries of proxies introduced here. There is no remaining product fix here that justifies a parallel implementation.

If carrying tests forward, SwipeViewStillTracksContentIsEnabled should assert delivery to an attached handler: its current Parent-only assertion cannot detect a dropped IsEnabled notification. main still lacks the existing fixes; any deliberate backport should reuse the inflight changes. Thanks for identifying these retention paths.

@kubaflo kubaflo closed this Sep 10, 2026

This branch was previously deployed

1 inactive deployment
copilot-pat-pool — 88825039 Deployed Sep 10, 2026 by wbv-profile via conclusion #1850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community ✨ Community Contribution

Projects

None yet

2 participants