[leak-scan] Fix: ShellContent.Content shared Page.PropertyChanged retains prior ShellContent instances

[SubhikshaSf4851]

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 whether this change resolves your issue. Thank you!

Description of Change

Memory management and event subscription improvements:

• Adds a destructor (~ShellContent) to ShellContent that unsubscribes from property change notifications, preventing memory leaks when a shared Page is assigned to multiple transient ShellContent instances (src/Controls/src/Core/Shell/ShellContent.cs).
• Refactored event subscription logic to use WeakNotifyPropertyChangedProxy and a cached handler, ensuring that event handlers do not prevent garbage collection of ShellContent instances (src/Controls/src/Core/Shell/ShellContent.cs).

Testing and regression coverage:

• Adds a unit test to verify that assigning the same long-lived Page to many transient ShellContent instances does not retain those instances in memory, addressing issue [leak-scan] ShellContent.Content — shared Page.PropertyChanged retains prior ShellContent instances #37217 (src/Controls/tests/Core.UnitTests/ShellTests.cs).

Public API updates:

• Adds destructor signature for ShellContent to the platform-specific PublicAPI.Unshipped.txt files to document the new public API surface (src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt, src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt, src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt, src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt, src/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txt, src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt, src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt). [1] [2] [3] [4]

Issues Fixed

Fixes #37217

Tested the behavior in the following platforms

• Windows
• Android
• iOS
• Mac

Before Issue Fix	After Issue Fix
BeforeFix37217.mov	AfterFix37217.mov

[github-actions]

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

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37390"

[dotnet-policy-service]

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

[vishnumenon2684]

/azp run maui-pr-uitests , maui-pr-devicetests

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[azure-pipelines]

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

[Copilot]

Pull request overview

This PR addresses a memory retention issue in .NET MAUI Shell where assigning the same long-lived Page to many transient ShellContent.Content instances could keep prior ShellContent instances alive via Page.PropertyChanged.

Changes:

• Switched ShellContent’s Page.PropertyChanged subscription to a weak proxy (WeakNotifyPropertyChangedProxy) and cached handler to avoid strong retention.
• Added a ~ShellContent finalizer that unsubscribes the proxy as a safety net to prevent proxy accumulation when ShellContent is collected.
• Added a unit test to validate that transient ShellContent instances are not retained when sharing a long-lived Page, and updated PublicAPI unshipped files to reflect the new finalizer API surface.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

File	Description
src/Controls/src/Core/Shell/ShellContent.cs	Replaces strong PropertyChanged subscription with weak proxy + cached handler; adds finalizer to ensure proxy unsubscribes.
src/Controls/tests/Core.UnitTests/ShellTests.cs	Adds regression test to ensure shared Page does not retain transient ShellContent instances after GC.
src/Controls/src/Core/PublicAPI/*/PublicAPI.Unshipped.txt	Records the newly added ShellContent finalizer in the public API baseline across TFMs.

[MauiBot]

Expert Review — 3 findings

See inline comments for details.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The delivery path for OnPagePropertyChanged changed from a direct page.PropertyChanged += to a weak proxy whose handler is only reachable through the _pagePropertyChangedHandler field, but no test in this PR (and none pre-existing — IShellContentController.IsPageVisibleChanged has no unit-test coverage anywhere in src/Controls/tests) exercises the positive path. Concrete scenario this leaves unguarded: ShellContentCollection.OnIsPageVisibleChanged (ShellContentCollection.cs:26/35) rebuilds the visible-items list from this event, so if the proxy ever stops forwarding (e.g. the handler field is not assigned before Subscribe, or a future refactor drops the field), setting page.IsVisible = false on a live ShellContent silently stops updating flyout/tab item visibility with no failing test. Add a test that sets Content = page, flips page.IsVisible, and asserts the IsPageVisibleChanged event fires.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[minor] Handler Lifecycle Symmetry — The unsubscribe is no longer keyed to the page that was removed: previously page.PropertyChanged -= OnPagePropertyChanged only detached the specific child, now any Page child removal tears down whatever source the single shared proxy currently points at. Today this is safe only because ContentCache's setter removes the old child before adding the new one (ShellContent.cs:229 before :235) and Page is the only logical child type, so the removed page is always the subscribed one. That invariant is implicit and unasserted — if a second Page child is ever added (or add/remove ordering is inverted), removing page A silently kills the live subscription for page B. Cheap hardening: if (child is Page page && _pagePropertyChangedProxy is not null && _pagePropertyChangedProxy.TryGetSource(out var s) && ReferenceEquals(s, page)) before unsubscribing.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[minor] Regression Prevention and Test Coverage — This GC assertion uses a single TestHelpers.Collect() pass, while the repo already provides TestHelpers.WaitForCollect() (TestHelpers.cs:20) which retries collection up to 40 times specifically to de-flake this class of test. ShellContent now has a finalizer, so each instance needs finalization to complete before it is fully reclaimed; under CI (server GC, tiered JIT, parallel xunit collections) a single forced pass is a weaker guarantee than the established helper. Prefer looping await reference.WaitForCollect() per reference, and on failure report the index/count rather than a bare Assert.Equal(0, alive) so a regression is diagnosable from the CI log.

[MauiBot]

AI Review Summary

@SubhikshaSf4851 — new AI review results are available based on commit ca1d50f.

---

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: IOS · Base: main · Merge base: 5d86a00a

Test	Without Fix (expect FAIL)	With Fix (expect PASS)
🧪 ShellTests ShellTests	✅ FAIL — 41s	✅ PASS — 25s

🔴 Without fix — 🧪 ShellTests: FAIL ✅ · 41s

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

     at Microsoft.Maui.Controls.Core.UnitTests.ShellTests.SharedPagePropertyChangedDoesNotRetainTransientShellContents() in /_/src/Controls/tests/Core.UnitTests/ShellTests.cs:line 1926

🟢 With fix — 🧪 ShellTests: PASS ✅ · 25s

(no coded error found; showing last 1200 chars)

dReAddingSetsCurrentItem [< 1 ms]
  Passed PoppedShellPageDisconnectsHandlers [31 ms]
  Passed VisualPropagationPageLevel [< 1 ms]
  Passed ClearingShellSectionAndReAddingSetsCurrentItem [< 1 ms]
  Passed ShellFlyoutChangeableOnShellWithFlyoutItem [< 1 ms]
  Passed FlyoutNavigateToImplicitContentPage [< 1 ms]
  Passed SameRouteInDifferentParentsIsAllowed [< 1 ms]
  Passed NavigationProxyWireUpTest [< 1 ms]
  Passed HidingShellSectionSetsNewCurrentItem [4 ms]
  Passed HotReloadStaysOnActiveItem [1 ms]
  Passed OnBackbuttonPressedPageReturnsFalse [< 1 ms]
  Passed ShellFlyoutChangeableOnShellWithTabBar [< 1 ms]
  Passed SimpleGoTo [< 1 ms]
[xUnit.net 00:00:00.92]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed ShellToolbarNotVisibleWithBasicContentPage [< 1 ms]
  Passed MenuItemBindingContext [< 1 ms]
  Passed CantNavigateToNotVisibleShellItem [1 ms]
  Passed SettingCurrentItemOnShellViaContentPage [< 1 ms]
  Passed ShellToolbarTitleIgnoresModalTitle [< 1 ms]
  Passed PopToRootShellPagesDisconnectHandlers [1 ms]
  Passed ShellSectionChangedFires [< 1 ms]
  Passed ShellToolbarTitle [< 1 ms]
Test Run Successful.
Total tests: 89
     Passed: 89
 Total time: 1.1853 Seconds

📁 Fix files reverted (8 files)

• src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt
• src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt
• src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
• src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt
• src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt
• src/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txt
• src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt
• src/Controls/src/Core/Shell/ShellContent.cs

---

📋 Pre-Flight — Context & Validation

PR #37390 Pre-Flight

Scope

• PR: [leak-scan] Fix: ShellContent.Content shared Page.PropertyChanged retains prior ShellContent instances
• Issue: #37217
• Base / head: main / SubhikshaSf4851:Fix-37217
• Materialized review branch: pr-review-37390 at ee3e293478 (PR #37390 squashed for review)
• Target platform requested by the review: iOS. The defect and primary test are cross-platform managed code.
• Gate: already passed before STEP 5a (test fails without the PR fix and passes with it). Do not rerun gate verification or alter gate/content.md.

Problem

A long-lived Page assigned as ShellContent.Content becomes a logical child. ShellContent.OnChildAdded subscribes ShellContent.OnPagePropertyChanged directly to Page.PropertyChanged. If the transient ShellContent is abandoned without clearing Content, OnChildRemoved never runs, so the page's delegate invocation list strongly retains the ShellContent.

Retention path:

shared ContentPage -> PropertyChanged delegate -> ShellContent.OnPagePropertyChanged -> transient ShellContent

Existing PR approach

The PR replaces the direct event subscription with a WeakNotifyPropertyChangedProxy, caches the bound handler, unsubscribes the proxy from OnChildRemoved, and adds a ShellContent finalizer that unsubscribes the proxy. The finalizer is recorded in all seven PublicAPI.Unshipped.txt target files.

Changed committed files:

• src/Controls/src/Core/Shell/ShellContent.cs (+9/-3)
• src/Controls/tests/Core.UnitTests/ShellTests.cs (+44)
• Seven src/Controls/src/Core/PublicAPI/*/PublicAPI.Unshipped.txt files (+1 each)

The regression test creates 30 transient ShellContent instances sharing one live ContentPage, forces collection, keeps the page alive, and requires all weak references to the shell contents to be dead.

Test boundary for each candidate

Detected primary test:

dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName=Microsoft.Maui.Controls.Core.UnitTests.ShellTests.SharedPagePropertyChangedDoesNotRetainTransientShellContents"

No additional mandatory regression tests were included in the STEP 5a request. Run only this primary test; do not run a full test suite. Each candidate receives one implementation/test pass and at most one focused correction/retest.

Candidate requirements

• Candidate 1 must differ from the PR's weak-proxy/finalizer strategy.
• Candidate 2 must read candidate 1's recorded result and use a different root-cause hypothesis or strategy; it must not reopen or rerun candidate 1.
• Preserve all pre-existing unrelated working-tree changes.
• Use EstablishBrokenBaseline.ps1 and its -Restore mode as required by the try-fix skill.
• Perform the required expert self-review inline; do not launch another reviewer.

---

🔬 Code Review — Deep Analysis

PR #37390 — Expert Code Review (raw submitted PR HEAD)

PR: [leak-scan] Fix: ShellContent.Content shared Page.PropertyChanged retains prior ShellContent instances
Issue: #37217 · Base/head: main / SubhikshaSf4851:Fix-37217 · Reviewed commit: ee3e293478 (materialized pr-review-37390)
Scope: submitted PR fix only. Try-fix candidates explicitly not evaluated or compared. No GitHub posting, no worktree mutation.

---

1. Independent assessment (code read before narrative)

Changed surface (9 files, +60/−3):

File	Change
src/Controls/src/Core/Shell/ShellContent.cs	+9/−3 — weak proxy subscription + finalizer
src/Controls/tests/Core.UnitTests/ShellTests.cs	+44 — GC regression test
7 × src/Controls/src/Core/PublicAPI/*/PublicAPI.Unshipped.txt	+1 each — ShellContent.~ShellContent() -> void

What the code does

OnChildAdded previously did page.PropertyChanged += OnPagePropertyChanged. Because ShellContent.OnPagePropertyChanged is an instance method, the resulting delegate rooted the ShellContent from the Page's invocation list. When a long-lived Page is assigned to many transient ShellContent instances, every abandoned ShellContent stayed reachable: Page → PropertyChanged delegate → ShellContent.

The PR replaces this with the repo's established WeakNotifyPropertyChangedProxy pattern:

• _pagePropertyChangedHandler ??= OnPagePropertyChanged — the delegate is rooted only by the ShellContent itself.
• _pagePropertyChangedProxy ??= new() then Subscribe(page, handler) — the proxy subscribes its own OnPropertyChanged to the page and stores WeakReferences to both source and handler.
• ~ShellContent() => _pagePropertyChangedProxy?.Unsubscribe() — removes the proxy from the live page's invocation list once the ShellContent is finalized.

Retention chain after the fix: Page → proxy.OnPropertyChanged → proxy → WeakReference<ShellContent-delegate>. The ShellContent is no longer strongly reachable from the page. Verified correct against src/Controls/src/Core/Internals/WeakEventProxy.cs.

Pattern conformance: identical in shape to the shipped Label precedent (Label.cs:124–150 — cached handler fields, ??= new() proxies, finalizer calling Unsubscribe()), which is exactly what the WeakEventProxy XML docs prescribe ("Your class should have a finalizer that calls Unsubscribe()"). Also used by Border, Path, VisualElement. This is the house pattern, not an invention.

Correctness probes I ran (all clean)

Probe	Result
Add/remove ordering — could an unconditional Unsubscribe() kill a live subscription?	No. ContentCache setter removes the old child (RemoveLogicalChild, line 229) before AddLogicalChild(value) (line 235). OnContentChanged likewise sets ContentCache = null before assigning the new page. So OnChildRemoved(old) always precedes OnChildAdded(new).
Could a second Page become a logical child concurrently?	No. The only AddLogicalChild of a Page is line 235; MenuItemsCollectionChanged routes MenuItems (not Page) through OnChildAdded/Removed, and the is Page guard filters them out.
Double-subscribe on repeated OnChildAdded for the same page?	Improved. WeakNotifyPropertyChangedProxy.Subscribe detaches the previous source first, so re-entry is now idempotent — the old direct += code could double-subscribe.
Template/service re-creation cycle (EvaluateDisconnect → RemoveLogicalChild → GetOrCreateContent → AddLogicalChild)	Proxy reuse is safe: Unsubscribe then Subscribe on the same instance.
Does the proxy still deliver IsVisible notifications?	Yes — handler field keeps the delegate alive for the ShellContent's lifetime; TryGetHandler succeeds. No functional regression identified, but untested (see F1).
Off-thread -= from the finalizer thread	BindableObject.PropertyChanged (BindableObject.cs:65) is a field-like event → compiler-generated interlocked add/remove; delegate invocation lists are immutable. Same risk profile as the shipped Label finalizer. Not a finding.
Test validity: does WeakReference.IsAlive work with a finalizable type?	Yes — short weak references are cleared when the object becomes unreachable, before finalization; TestHelpers.Collect() also does two WaitForPendingFinalizers. GC.KeepAlive(sharedPage) correctly pins the page, and CreateShellContents is [MethodImpl(NoInlining)] so no stack slot keeps the last instance rooted.
Public API bookkeeping	All 7 TFM-specific PublicAPI.Unshipped.txt files updated; Shipped.txt untouched; entry shape matches the existing Label.~Label() line. Correct.

Blast radius / failure-mode probing (Shell + PropertyChanged subscriptions)

• Consumer of the affected signal: the only thing OnPagePropertyChanged does is re-raise IShellContentController.IsPageVisibleChanged for Page.IsVisible. Its sole consumer is ShellContentCollection.OnIsPageVisibleChanged (ShellContentCollection.cs:26/35/42), which maintains the visible items projection driving flyout/tab item rendering. A silent break here is user-visible (stale tab/flyout entries) and has zero unit-test coverage today — that is the main residual risk, captured as F1.
• Delayed-detach window: proxies now linger in a shared page's invocation list until the owning ShellContent is finalized. This is bounded (one small proxy per abandoned ShellContent, self-purging on the next PropertyChanged via TryGetHandler failure) and is the accepted trade-off of the house pattern. Not a regression.
• Finalizer cost: every ShellContent instance is now promoted to the finalization queue, adding one GC cycle to reclamation even when no page is attached. Real but negligible at Shell-item cardinality, and precedented by Label (far higher cardinality). Not blocking.
• Adjacent, unfixed retention paths in the same file (pre-existing, outside the diff — noted for awareness, not charged to this PR): SendPageAppearing attaches page.ParentSet += OnPresentedPageParentSet with a closure capturing this (line 172), and _createdViaService pages get value.Unloaded += OnPageUnloaded (line 239). The first is the same class of Page → ShellContent strong edge and only self-detaches if ParentSet actually fires. Fixing it is out of scope here, but it means #37217's general shape isn't fully eliminated by this PR.
• Cross-platform: change is shared managed code in Controls.Core; no platform-conditional code touched. Platform behavior is uniform by construction.
• Regression cross-reference: regression-check/risks.json → "result": "CLEAN", revert_count: 0, overlap_count: 0. No previously-added bug-fix lines are removed.

Independent verdict before reading narrative: sound, minimal, idiomatic fix; residual concerns are test-coverage and robustness-grade only.

---

2. Narrative reconciliation

pre-flight/content.md describes the retention path as shared ContentPage → PropertyChanged delegate → ShellContent.OnPagePropertyChanged → transient ShellContent and the fix as "weak proxy + cached handler + unsubscribe in OnChildRemoved + finalizer, recorded in all seven PublicAPI.Unshipped.txt files."

This matches my independent read exactly — root cause, mechanism, and file inventory all reconcile with no gap. The one thing the narrative does not mention, and which my read adds, is that OnChildRemoved is now unconditional rather than keyed to the removed page (F2), and that the changed notification path has no positive-path test (F1).

Title/description accurately describe the implementation; no scope creep, no bundled unrelated changes (a try-fix anti-pattern this PR avoids).

---

3. Prior-review reconciliation

gh is unauthenticated in this environment, so GitHub review threads could not be fetched directly. Reconciliation was done against the available pipeline artifacts:

• pre-flight/content.md — no reviewer findings recorded against the submitted PR.
• regression-check/ — CLEAN, nothing to acknowledge.
• try-fix/attempt-{1,2}/reviewer-findings.json — these belong to alternative candidates and are out of scope per the review brief; they were not read into this assessment and none of my findings are derived from them.

No duplicate-suppression was therefore possible from live PR comments; if any of F1–F3 has already been raised on the PR, it should be de-duplicated at posting time. All three findings are new relative to every artifact available offline.

---

4. Required-check status

Not retrievable in this run: gh is unauthenticated (gh auth login prompt on gh pr diff) and no CI-status artifact exists under CustomAgentLogsTmp/PRState/37390/. Required-check status is therefore UNKNOWN and this review does not certify it.

What is trusted and available:

• Gate: PASSED (pipeline-supplied) — the new test fails without the fix and passes with it; logs present under gate/verify-tests-fail/. Not rerun, per instruction.
• Regression cross-reference: CLEAN.

Merge should still be conditioned on green required checks — in particular the Controls.Core.UnitTests leg (the new GC test) and the Public API analyzer leg (7 Unshipped.txt edits must match the emitted API shape on every TFM, including the Tizen/netstandard legs that are easy to miss).

---

5. Findings

Inline findings written to CustomAgentLogsTmp/PRState/37390/PRAgent/inline-findings.json (3 entries, all on lines changed by this PR).

Blocking

None.

Should fix (non-blocking)

F1 · [moderate] Regression Prevention and Test Coverage — ShellContent.cs:196
The PR adds a leak test but no positive-path test. The delivery mechanism for OnPagePropertyChanged changed materially (direct event → weak proxy with a weakly-referenced handler), and IShellContentController.IsPageVisibleChanged has no unit-test coverage anywhere in src/Controls/tests. Its consumer ShellContentCollection.OnIsPageVisibleChanged drives visible flyout/tab item projection, so a forwarding failure is user-visible and currently silent. Add a test that assigns Content = page, toggles page.IsVisible, and asserts IsPageVisibleChanged fires. This is the classic "green tests prove the negative case, nothing covers the positive case" gap.

F2 · [minor] Handler Lifecycle Symmetry — ShellContent.cs:206
OnChildRemoved now unsubscribes whatever the single shared proxy points at, rather than the specific removed page. Validated as currently unreachable: ContentCache's remove-before-add ordering plus the single-Page-child invariant guarantee the removed page is always the subscribed one. It is a latent fragility, not a live bug — an implicit invariant with no assertion. A TryGetSource + ReferenceEquals(s, page) guard restores the precise pairing the old code had, at no cost.

F3 · [minor] Regression Prevention and Test Coverage — ShellTests.cs:1913
The GC assertion uses one TestHelpers.Collect() pass where the repo already ships TestHelpers.WaitForCollect() (retries up to 40×) precisely to de-flake this test class. ShellContent is now finalizable, and CI runs server GC / tiered JIT / parallel xunit. Prefer the retry helper, and report which/how many references survived instead of a bare Assert.Equal(0, alive) so a CI failure is diagnosable.

Noted, not charged to this PR

• Pre-existing Page → ShellContent strong edges remain at ShellContent.cs:172 (ParentSet closure capturing this) and :239 (Unloaded). #37217's general shape is narrowed, not eliminated.
• Brace indentation of the preceding test's closing } (ShellTests.cs:1900) was disturbed by the insertion. Braces balance; dotnet format owns this. Deliberately not filed as an inline finding per the no-style rule.
• ShellContent gaining a finalizer costs one extra GC cycle per instance. Precedented by Label; acceptable.

---

6. Verdict

LGTM (with two optional follow-ups)

Confidence: high — for the fix mechanism and the absence of blocking defects. Medium on merge-readiness overall, solely because required-check status could not be observed in this environment.

Rationale. The change is minimal, targets the exact root cause identified in #37217, and applies the repository's own prescribed weak-event pattern (WeakEventProxy docs; Label/Border/VisualElement precedent) rather than an ad-hoc scheme. The retention chain is provably broken: the only strong root for the handler delegate is the ShellContent itself, and the finalizer purges the residual proxy from live pages. Add/remove ordering, template re-creation cycles, multi-child scenarios, off-thread unsubscribe, public API bookkeeping across all 7 TFMs, and the GC-test methodology were each probed and came back clean. The regression cross-reference is CLEAN and Gate passed.

The residual items are quality-grade: the changed notification path lacks a positive-path test (F1 — the one I'd most like addressed, since its consumer is user-visible flyout/tab visibility), the unsubscribe is defensively looser than the code it replaces (F2), and the GC test skips the repo's own de-flake helper (F3). None of these justifies blocking the fix. Merge should be gated on green required checks, especially the Public API analyzer legs given the 7-file Unshipped.txt edit.

---

🛠️ Try-Fix — Analysis & Comparison

STEP 5a Try-Fix Aggregate

Pre-flight completed. Candidate results will be appended sequentially.

Candidate 1 — Attachment-Scoped Subscription

Model: claude-opus-5
Result: Pass
Detailed record: ../try-fix-1/content.md
Standard artifacts: attempt-1/

Approach

Treat the subscription lifetime, rather than its reference strength, as the defect. Keep a normal Page.PropertyChanged subscription only while the ShellContent has both a content page and a parent. OnChildAdded, OnChildRemoved, and the existing OnPropertyChanged(nameof(Parent)) path reconcile the observed page. Reattachment raises _isPageVisibleChanged to account for visibility changes while detached.

This differs from PR #37390: it uses neither WeakNotifyPropertyChangedProxy nor a finalizer and adds no public API. Teardown is deterministic on detach, and never-attached transient contents never subscribe.

Diff

diff --git a/src/Controls/src/Core/Shell/ShellContent.cs b/src/Controls/src/Core/Shell/ShellContent.cs
index 1faa996f76..bc31c4d7cf 100644
--- a/src/Controls/src/Core/Shell/ShellContent.cs
+++ b/src/Controls/src/Core/Shell/ShellContent.cs
@@ -182,12 +182,16 @@ namespace Microsoft.Maui.Controls
 			}
 		}
 
+		Page _contentPage;
+		Page _subscribedPage;
+
 		protected override void OnChildAdded(Element child)
 		{
 			base.OnChildAdded(child);
 			if (child is Page page)
 			{
-				page.PropertyChanged += OnPagePropertyChanged;
+				_contentPage = page;
+				UpdatePageVisibilityObserver();
 				_isPageVisibleChanged?.Invoke(this, EventArgs.Empty);
 			}
 		}
@@ -197,10 +201,36 @@ namespace Microsoft.Maui.Controls
 			base.OnChildRemoved(child, oldLogicalIndex);
 			if (child is Page page)
 			{
-				page.PropertyChanged -= OnPagePropertyChanged;
+				if (ReferenceEquals(_contentPage, page))
+					_contentPage = null;
+
+				UpdatePageVisibilityObserver();
 			}
 		}
 
+		bool UpdatePageVisibilityObserver()
+		{
+			var target = Parent is not null ? _contentPage : null;
+
+			if (ReferenceEquals(target, _subscribedPage))
+				return false;
+
+			if (_subscribedPage is not null)
+				_subscribedPage.PropertyChanged -= OnPagePropertyChanged;
+
+			_subscribedPage = target;
+
+			if (_subscribedPage is null)
+				return false;
+
+			_subscribedPage.PropertyChanged += OnPagePropertyChanged;
+			return true;
+		}
+
 		void OnPagePropertyChanged(object sender, PropertyChangedEventArgs e)
 		{
@@ -290,6 +320,13 @@ namespace Microsoft.Maui.Controls
 
 				EvaluateDisconnect();
 			}
+			else if (propertyName == nameof(Parent))
+			{
+				if (UpdatePageVisibilityObserver())
+					_isPageVisibleChanged?.Invoke(this, EventArgs.Empty);
+			}
 		}

Tests and analysis

Only the permitted primary test was run. The first run failed with RS0016 because the initial implementation introduced protected override OnParentSet(). The one allowed focused correction moved the hook into the existing OnPropertyChanged override. The second run passed: 0 failed, 1 passed, 1 total.

Inline expert self-review recorded two final findings (0 critical, 0 major, 1 moderate, 1 minor). Residual risk: detached ShellContent instances stop observing page visibility; this matches the current event consumer's attached lifetime but is a broader behavioral change than the PR's lifetime-neutral weak proxy.

Candidate 2 — Direct Logical-Parent Visibility Notification

Model: gpt-5.6-sol
Result: Blocked
Detailed record: ../try-fix-2/content.md
Standard artifacts: attempt-2/

Planned approach

Remove ShellContent's Page.PropertyChanged subscription entirely. Route page/visual-element visibility changes directly to the page's current logical ShellContent parent, which would raise IShellContentController.IsPageVisibleChanged. This removes the retaining event edge instead of weakening it (the PR) or limiting its lifetime to attachment (candidate 1).

Diff

No source or test diff was produced. attempt-2/fix.diff is empty.

Blocker and analysis

The mandatory EstablishBrokenBaseline.ps1 precondition rejected the existing unrelated tracked changes under .github and eng. Preserving those changes was also mandatory, so candidate 2 could not safely establish the broken baseline, implement the approach, or run the test. The required restore command ran and reported that no baseline state existed.

The primary test was not run. Inline expert self-review recorded 0 findings because no candidate diff existed. The planned design remains unvalidated and would need careful treatment of shared-page logical-parent ownership.

Aggregate outcome

Two bounded attempts were completed in order:

Candidate	Model	Strategy	Result
1	claude-opus-5	Scope a normal page subscription to the attached lifetime	Pass after one focused correction
2	gpt-5.6-sol	Eliminate the subscription and notify through logical parentage	Blocked before implementation

Candidate 1 is the only empirically validated alternative. Candidate 2 is recorded as a distinct but unvalidated direction, not as a passing fix.

---

🏁 Report — Final Recommendation

⚠️ Final Recommendation: REQUEST CHANGES

Winning candidate: pr-plus-reviewer

pr-plus-reviewer preserves the submitted PR's sound, repository-standard weak-event fix while closing all three quality gaps identified by the single expert review. Its one focused validation passed both the original leak regression and the new positive notification-path regression.

Comparative ranking

Rank	Candidate	Validation	Assessment
1	pr-plus-reviewer	Pass: 2/2 focused tests	Best balance of correctness and regression protection. It retains the raw PR's lifetime-neutral behavior, precisely pairs removal with the subscribed page, proves weak-proxy forwarding still works, and uses the established retrying GC assertion.
2	pr	Pass: trusted Gate fails without the fix and passes with it	The expert reviewer found the core weak-proxy/finalizer implementation sound and idiomatic, with no blocking defects. It ranks below the winner because the submitted test only covers collection, the notification forwarding path is untested, unsubscribe pairing relies on an implicit invariant, and the GC assertion omits the repository retry helper.
3	try-fix-1	Pass: 1/1 primary regression after its allowed correction	Deterministic attachment-scoped subscription avoids a finalizer and public API bookkeeping, but changes behavior by suppressing page visibility observation while detached. Its reattachment mitigation and current-consumer assumption remain less directly covered and broader than the raw PR's lifetime-neutral strategy.
4	try-fix-2	Blocked: no implementation or test run	The direct logical-parent notification idea is distinct but produced no diff and has unresolved shared-page/reparenting semantics, so it cannot outrank validated candidates.

Expert-review disposition

The raw PR received an expert LGTM for its core fix, with three non-blocking actionable findings. pr-plus-reviewer addresses all three in one consolidated patch and passed its single targeted validation command. Required-check status for the live PR remained unobservable in the expert environment, so this recommendation does not claim full CI is green.

Required submitted-PR change

Apply PRAgent/pr-plus-reviewer/reviewer.patch to the PR branch. Because the winning refinement is not present in the submitted PR HEAD, the result must remain REQUEST CHANGES even though the original fix itself is technically sound.

---

📱 UI Tests — Shell

Detected UI test categories: Shell

✅ Deep UI tests — 307 passed, 0 failed, 16 skipped across 1 category on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category	Tests	Snapshot diffs
Shell	307/323 (16 skipped) ✓	—
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

---

🧭 Next Steps — reviewer patch required (pr-plus-reviewer)

The reviewer-enhanced candidate won, so the submitted PR still needs those changes.

Why: The reviewer-enhanced candidate preserves the PR's sound weak-event strategy while addressing all three expert findings. Its focused validation passed both the original leak regression and new positive notification-path coverage, making it stronger than the raw PR and the STEP 5a alternatives.

Apply PRAgent/pr-plus-reviewer/reviewer.patch from the CopilotLogs
artifact (or follow the report's Required submitted-PR change), push the update, and run
the review again.