[net11.0][Android] Fix StackNavigationManager binary break and ShellItemWrapperFragment OnDestroyView gap (follow-up #34758)

[Vignesh-SF3580]

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!

Issue Details

PR #34758 introduced the Android handler-based Shell architecture. During review, two follow-up issues were identified and tracked in issue #36108:
Concern 1 — StackNavigationManager.Connect(IView) binary compatibility
Connect(IView) was a shipped public API. PR #34758 replaced it with a 2-parameter overload using an optional parameter. Although source-compatible, it is binary-breaking because existing precompiled callers may fail with MissingMethodException at runtime.
Concern 2 — ShellItemWrapperFragment missing OnDestroyView
ShellItemWrapperFragment did not override OnDestroyView. If the view is destroyed while the fragment remains alive (back-stack scenario), _viewPager and _rootLayout can retain detached views, causing memory leaks. View recreation can also skip adapter setup, resulting in a blank screen.

Root Cause

Concern 1: Optional parameter overloads are source-compatible but not binary-compatible. Existing compiled callers referencing Connect(IView) cannot resolve the new 2-parameter signature at runtime.

Concern 2: Android fragments separate view destruction (OnDestroyView) from fragment destruction (Dispose). Without OnDestroyView, view references remain held when the fragment stays on the back stack.

Description of Changes

Fix 1 — StackNavigationManager.cs: Added the 1-arg Connect(IView) overload that delegates to Connect(IView, null), restoring the original binary-compatible API. Removed the duplicate entry from Core/PublicAPI.Unshipped.txt since it already exists in PublicAPI.Shipped.txt.

Fix 2 — ShellItemWrapperFragment.Android.cs: Added OnDestroyView() to clear _viewPager, _adapter, and _rootLayout references, preventing detached view leaks and allowing proper view recreation. Updated _adapter accessibility for fragment access and registered the API change in Controls/PublicAPI.Unshipped.txt.

Issues Fixed

Fixes #36108

[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 -- 36133

Or

• Run remotely in PowerShell:

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

[MauiBot]

Expert Review — 4 findings

See inline comments for details.

[PureWeen]

Adversarial multi-model review — 3 independent reviewers + dispute round

Methodology: Reviewer 1/2/3 (different model families) reviewed the diff independently, then a follow-up dispute round adjudicated single-reviewer findings against verified source. CI status is intentionally out of scope here.

✅ Fix 1 (StackNavigationManager binary compat) — verified correct

• Connect(IView) (1-arg) exists in PublicAPI.Shipped.txt (line 3677), so removing the *REMOVED* line from Unshipped.txt and re-adding the overload in code is correct — RS0016/RS0017 will pass.
• Git history confirms the pre-#34758 Connect(IView) body was equivalent to Connect(view, null) (container falls back to navigationView.Handler.PlatformView), so behavior is preserved.
• Overload resolution: Connect(view) binds unambiguously to the exact-arity 1-arg overload — no source break (3/3 reviewers confirmed). The redundancy of a 1-arg overload alongside a 2-arg-with-optional-default was raised once and discarded — it is intentional and harmless for binary compat.

Fix 2 (OnDestroyView) — incomplete cleanup

OnDestroyView correctly nulls _viewPager/_adapter so the adapter rebinds, but the same back-stack re-setup path (OnViewCreated re-runs while the handler outlives the view) leaves sibling handler state stale. Independent reviewers confirmed at 3/3 the issues already flagged inline in the prior automated review:

• _pageChangeCallback not reset → SetupViewPagerAdapter's if (_pageChangeCallback is null) guard skips re-registration, so the new ViewPager2 never receives the page-change callback. Toolbar/top-tab sync silently breaks on back-stack return — a functional regression, not just a leak.
• _rootLayout window-inset listener not removed before nulling — also defeats Dispose's own removal (it guards on _rootLayout is not null). Bounded by WeakReference pruning, but real.
• _tabbedViewManager/_shellItemAdapter/_bottomNavigationView recreated without teardown in SetupTabbedViewManager.

This run additionally surfaced two issues not in the prior review (see inline comments):

• ❌ Toolbar trackers + duplicate toolbar leak (3/3 after dispute)
• ❌ Appearance observer double-registration (3/3 after dispute)

💡 Optional: _backPressedCallback is recreated in OnViewCreated but not nulled in OnDestroyView (only Dispose disposes it). Immaterial — it is auto-removed via ViewLifecycleOwner — but a one-line Dispose here would be symmetric.

Recommendation

The cleanest fix is an internal helper on ShellItemHandler (e.g. OnFragmentViewDestroyed()) that mirrors the DisconnectHandler teardown for the back-stack case: unregister+null _pageChangeCallback; dispose+null _toolbarTracker/_toolbarAppearanceTracker and remove _toolbar from _appBarLayout; tear down _tabbedViewManager/_shellItemAdapter; call RemoveAppearanceObserver; then null _viewPager/_adapter/_bottomNavigationView. OnDestroyView calls that helper and removes the _rootLayout inset listener. A device test exercising push → back on a Shell with bottom tabs (verify tab switching and toolbar appearance re-sync after the round-trip) would lock this down and catch the _pageChangeCallback regression.

Verdict: COMMENT — 3 independent reviewers with adversarial consensus. Fix 1 is solid; Fix 2 needs the cleanup above before merge.

[MauiBot]

Expert Review — 3 findings

See inline comments for details.

[Copilot]

Pull request overview

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

[vishnumenon2684]

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

[azure-pipelines]

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

[Vignesh-SF3580]

Could you please check if test failures are related?

@kubaflo I checked the CI test failures. My code changes are Android-specific, while the failing tests are on iOS and macOS. Therefore, these test failures are not related to my code changes.

[MauiBot]

AI Review Summary

ℹ️ The review agent did not produce a full summary on this run (an infrastructure issue on the CI agent), but the deep UI tests completed — their results are below. Re-comment /review for a fresh full review.

✅ Deep UI tests — 118 passed, 0 failed across 1 category on platform-pool agent (replaces in-process counts above). The deep UI run for 1 category (Shell) hit the per-category time budget, but because the app/emulator kept crashing — each retry failed to recover from an app crash (e.g. did not recover after crash-recovery attempts) and burned its whole slice until the deadline. This is an app-stability/infrastructure issue, not a time shortfall, so raising the budget will not help (it would just spin longer). Re-run the review to try again; if it recurs, check the build-output.log + logcat in the drop-deep-uitests artifact for the app-startup crash.

🧪 UI Test Execution Results (deep, platform pool)

Category	Tests	Snapshot diffs
ViewBaseTests	118/119 ✓	—
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)