Repository navigation
Use weak events for SwipeView and ShellContent child subscriptions - #38415
wbv-profile wants to merge 1 commit into
Conversation
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>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 38415Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 38415" |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
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. |
|
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. |
|
Closing as overlapping with #37390 (ShellContent, 26233a1) and #38101 (SwipeView, dba446a), both already merged into 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, |
Description of Change
SwipeViewandShellContentboth subscribe to a child'sPropertyChangedinOnChildAddedand unsubscribe inOnChildRemoved, 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.OnChildAddedcarries a comment explaining thatSwipeItemschildren are deliberately not subscribed for exactly this reason, andOnSwipeItemsChangeddocuments the earlier fix for #35481 where captured closures kept sharedSwipeItemsfrom 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.csadds four tests: one leak test per control (a retainedLabelassigned asSwipeView.Content, a retainedContentPageassigned asShellContent.Content), plus two guards pinning that attached content is still parented and that removing it still detaches.Reverting the
src/Controls/src/Corechanges fails exactly the two leak tests and passes both guards. With the fix all four pass and the fullControls.Core.UnitTestssuite is green (5651 passed, 0 failed).Issues Fixed
Fixes #37217
Fixes #37218