-
Notifications
You must be signed in to change notification settings - Fork 2k
[net11.0][Android] Fix StackNavigationManager binary break and ShellItemWrapperFragment OnDestroyView gap (follow-up #34758) #36133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5a8a3f9
00964fb
34ac183
5a20117
80f99b3
4d0879f
4615b7f
c8d3fb2
7a26611
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,114 @@ public override void OnViewCreated(AView view, Bundle? savedInstanceState) | |
| } | ||
| } | ||
|
|
||
| public override void OnDestroyView() | ||
|
Vignesh-SF3580 marked this conversation as resolved.
Vignesh-SF3580 marked this conversation as resolved.
|
||
| { | ||
| // Null out view references so we don't hold on to detached views | ||
| // when the fragment is placed on the back stack (view destroyed, fragment alive). | ||
| // Also null _adapter so SetupViewPagerAdapter re-creates it and assigns it | ||
| // to the new ViewPager2 on the next OnViewCreated (adapter reset path). | ||
| if (_handler is not null) | ||
|
Vignesh-SF3580 marked this conversation as resolved.
Vignesh-SF3580 marked this conversation as resolved.
|
||
| { | ||
| // Unregister appearance observer before view is gone. | ||
| // RegisterAppearanceObserver() runs again on OnViewCreated via bare List.Add — | ||
| // without removal here, each back-stack return stacks a duplicate registration. | ||
| if (_handler._registeredShell is not null) | ||
| { | ||
| ((IShellController)_handler._registeredShell).RemoveAppearanceObserver(_handler); | ||
| _handler._registeredShell = null; | ||
| } | ||
|
|
||
| // Unregister displayed-page observer before view is gone. | ||
| // SwitchToSection() only removes the observer when _shellSection != newSection — | ||
| // on same-section view recreation, no removal happens, causing AddDisplayedPageObserver | ||
| // to stack a duplicate entry on each back-stack return. | ||
| if (_handler._shellSection is not null) | ||
| { | ||
| ((IShellSectionController)_handler._shellSection).RemoveDisplayedPageObserver(_handler); | ||
|
Vignesh-SF3580 marked this conversation as resolved.
|
||
| _handler._shellSection = null; | ||
|
Vignesh-SF3580 marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Reset _displayedPage so UpdateDisplayedPage re-runs fully on back-stack return | ||
| // instead of early-returning due to the same page reference, leaving the toolbar unconfigured. | ||
| _handler._displayedPage = null; | ||
|
|
||
| // Null _bottomNavigationView so appearance callbacks don't update stale views | ||
| // while the fragment sits on the back stack. | ||
| _handler._bottomNavigationView = null; | ||
|
|
||
| // Tear down TabbedViewManager before view recreation. | ||
| // SetupTabbedViewManager overwrites _tabbedViewManager/_shellItemAdapter silently — | ||
| // without teardown here, old BNV listeners and event subscriptions leak. | ||
| // Note: SetElement(null) already calls RemoveTabs() internally — no need to call it explicitly. | ||
| if (_handler._tabbedViewManager is not null) | ||
| { | ||
| _handler._tabbedViewManager.SetElement(null); | ||
| _handler._tabbedViewManager = null; | ||
| } | ||
| _handler._shellItemAdapter = null; | ||
|
|
||
| // Tear down toolbar to prevent duplicate toolbars and tracker leaks on view recreation. | ||
| // SetupToolbar() runs again on OnViewCreated, so old trackers must be disposed | ||
| // and old toolbar removed from _appBarLayout before that happens. | ||
| _handler._toolbarAppearanceTracker?.Dispose(); | ||
| _handler._toolbarAppearanceTracker = null; | ||
|
|
||
| _handler._toolbarTracker?.Dispose(); | ||
| _handler._toolbarTracker = null; | ||
|
|
||
| if (_handler._toolbar?.Parent is ViewGroup toolbarParent) | ||
| { | ||
| toolbarParent.RemoveView(_handler._toolbar); | ||
| } | ||
|
|
||
| _handler._toolbar = null; | ||
|
|
||
| // Null _shellToolbar — mirrors DisconnectHandler's cleanup. | ||
| // SetupToolbar() creates a new one on OnViewCreated. | ||
| _handler._shellToolbar = null; | ||
|
|
||
| // Null _appBarLayout — set in SetupToolbar() from OnViewCreated. | ||
| // Leaving it non-null holds a detached view reference after view destruction. | ||
| _handler._appBarLayout = null; | ||
|
|
||
| // Unregister page-change callback before nulling _viewPager. | ||
| // SetupViewPagerAdapter guards on `if (_pageChangeCallback is null)` — | ||
| // if not nulled here, the new ViewPager2 never receives the callback | ||
| // and toolbar/top-tab sync silently breaks on back-stack return. | ||
| if (_handler._pageChangeCallback is not null && _handler._viewPager is not null) | ||
| { | ||
| _handler._viewPager.UnregisterOnPageChangeCallback(_handler._pageChangeCallback); | ||
| } | ||
| _handler._pageChangeCallback = null; | ||
|
|
||
| // Clear adapter before nulling _viewPager to detach FragmentStateAdapter. | ||
| // FragmentStateAdapter unregisters lifecycle/adapter observers only when cleared, | ||
| // not when the view is destroyed — mirrors DisconnectHandler's adapter/viewpager cleanup. | ||
| if (_handler._viewPager is not null) | ||
| { | ||
| _handler._viewPager.Adapter = null; | ||
| } | ||
| _handler._viewPager = null; | ||
|
Vignesh-SF3580 marked this conversation as resolved.
Vignesh-SF3580 marked this conversation as resolved.
Vignesh-SF3580 marked this conversation as resolved.
|
||
| _handler._adapter = null; | ||
|
Vignesh-SF3580 marked this conversation as resolved.
Vignesh-SF3580 marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Memory Leak Prevention — This drops the ShellSectionFragmentAdapter without disposing the ShellSection renderers it owns. Those renderers call ShellSectionHandler.ConnectHandler(), which subscribes to SectionController.ItemsCollectionChanged and registers Shell appearance observers; the only cleanup path is ShellSectionHandlerAdapter.Dispose(). Concrete scenario: a ShellItem with top tabs is put on the Android back stack, OnDestroyView clears _adapter, then OnViewCreated creates a new adapter/renderers while the old handlers remain subscribed and can keep updating/leaking stale toolbar/top-tab state. Dispose all renderers (or add an adapter teardown helper) before nulling the adapter. |
||
| } | ||
|
|
||
| // Remove window insets listener before nulling _rootLayout. | ||
| // Dispose guards on `_rootLayout is not null` — if nulled here first, | ||
| // Dispose skips the removal and the listener is never unregistered. | ||
| if (_rootLayout is not null) | ||
| { | ||
| MauiWindowInsetListener.RemoveViewWithLocalListener(_rootLayout); | ||
| } | ||
|
|
||
| _rootLayout = null; | ||
|
Vignesh-SF3580 marked this conversation as resolved.
Vignesh-SF3580 marked this conversation as resolved.
|
||
|
|
||
| // _backPressedCallback is auto-removed by ViewLifecycleOwner when the view is destroyed. | ||
| // Null here for symmetry — OnViewCreated recreates it. | ||
| _backPressedCallback = null; | ||
|
|
||
| base.OnDestroyView(); | ||
| } | ||
|
|
||
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| using Microsoft.Maui.Controls; | ||
|
|
||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| // Regression guard for #36108 — Android Shell handler OnDestroyView defensive cleanup. | ||
| // Tests that after switching between FlyoutItems: | ||
| // 1. Toolbar title is correctly configured (UpdateDisplayedPage ran — not early-returned on stale _displayedPage) | ||
| // 2. Tab switching still updates the toolbar (ViewPager2 page callback registered correctly) | ||
| [Issue(IssueTracker.Github, 36108, "Android Shell handler — OnDestroyView defensive cleanup regression guard", PlatformAffected.Android)] | ||
| public class Issue36108 : TestShell | ||
| { | ||
| public const string FlyoutItemATitle = "Section A"; | ||
| public const string FlyoutItemBTitle = "Section B"; | ||
| public const string Tab1Title = "Tab 1"; | ||
| public const string Tab2Title = "Tab 2"; | ||
| public const string Tab1LabelId = "Tab1Label"; | ||
| public const string Tab2LabelId = "Tab2Label"; | ||
| public const string SectionBLabelId = "SectionBLabel"; | ||
|
|
||
| protected override void Init() | ||
| { | ||
| var tab1Page = new ContentPage | ||
| { | ||
| Title = Tab1Title, | ||
| Content = new Label | ||
| { | ||
| Text = "Tab 1 Content", | ||
| AutomationId = Tab1LabelId, | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| } | ||
| }; | ||
|
|
||
| var tab2Page = new ContentPage | ||
| { | ||
| Title = Tab2Title, | ||
| Content = new Label | ||
| { | ||
| Text = "Tab 2 Content", | ||
| AutomationId = Tab2LabelId, | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| } | ||
| }; | ||
|
|
||
| var sectionBPage = new ContentPage | ||
| { | ||
| Title = FlyoutItemBTitle, | ||
| Content = new Label | ||
| { | ||
| Text = "Section B Content", | ||
| AutomationId = SectionBLabelId, | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| } | ||
| }; | ||
|
|
||
| // FlyoutItem A — has two bottom tabs | ||
| var flyoutItemA = new FlyoutItem | ||
| { | ||
| Title = FlyoutItemATitle, | ||
| Route = "SectionA", | ||
| Items = | ||
| { | ||
| new Tab | ||
| { | ||
| Title = Tab1Title, | ||
| Route = "Tab1", | ||
| AutomationId = Tab1Title, | ||
| Items = { new ShellContent { Content = tab1Page } } | ||
| }, | ||
| new Tab | ||
| { | ||
| Title = Tab2Title, | ||
| Route = "Tab2", | ||
| AutomationId = Tab2Title, | ||
| Items = { new ShellContent { Content = tab2Page } } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // FlyoutItem B — simple page | ||
| var flyoutItemB = new FlyoutItem | ||
| { | ||
| Title = FlyoutItemBTitle, | ||
| Route = "SectionB", | ||
| Items = | ||
| { | ||
| new Tab | ||
| { | ||
| Items = { new ShellContent { Content = sectionBPage } } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| Items.Add(flyoutItemA); | ||
| Items.Add(flyoutItemB); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| // Regression guard for https://github.com/dotnet/maui/issues/36108 | ||
| // Verifies that after navigating between FlyoutItems (A → B → A), the Shell tab bar | ||
| // and page content are still correctly rendered — guarding against the stale | ||
| // _displayedPage guard in UpdateDisplayedPage skipping toolbar/tab configuration. | ||
| public class Issue36108 : _IssuesUITest | ||
| { | ||
| public Issue36108(TestDevice device) : base(device) | ||
| { | ||
| } | ||
|
|
||
| public override string Issue => "Android Shell handler — OnDestroyView defensive cleanup regression guard"; | ||
|
Vignesh-SF3580 marked this conversation as resolved.
|
||
|
|
||
| // Navigate Section A → Section B → Section A and verify Tab 1 content is shown. | ||
| // If _displayedPage is not reset during ShellItem switch, UpdateDisplayedPage | ||
| // early-returns on the stale same-page reference and the tab bar is misconfigured. | ||
| [Test, Order(0)] | ||
| [Category(UITestCategories.Shell)] | ||
| public void TabContentVisibleAfterFlyoutItemRoundTrip() | ||
|
Vignesh-SF3580 marked this conversation as resolved.
|
||
| { | ||
| App.WaitForElement("Tab1Label"); | ||
|
|
||
| // Open flyout and navigate to Section B | ||
| App.TapShellFlyoutIcon(); | ||
| App.WaitForElement("Section B"); | ||
| App.Tap("Section B"); | ||
| App.WaitForElement("SectionBLabel"); | ||
|
|
||
| // Open flyout and navigate back to Section A | ||
| App.TapShellFlyoutIcon(); | ||
| App.WaitForElement("Section A"); | ||
| App.Tap("Section A"); | ||
|
|
||
| // Tab 1 content must be visible — UpdateDisplayedPage must not early-return | ||
| App.WaitForElement("Tab1Label"); | ||
|
Vignesh-SF3580 marked this conversation as resolved.
|
||
|
|
||
| // Defensive: verify tab bar is functional after round-trip. | ||
| // If UpdateTabBarVisibility was skipped (due to stale _displayedPage early-return), | ||
| // the bottom nav tabs may be hidden or misconfigured — Tab 2 would be unreachable. | ||
| App.WaitForElement("Tab 2"); | ||
| } | ||
|
|
||
| // After navigating A → B → A, verify that tapping Tab 2 correctly shows Tab 2 content. | ||
| // This guards against ViewPager2 page-change callbacks being dropped when the | ||
| // ShellItem view is recreated after returning from Section B. | ||
| [Test, Order(1)] | ||
| [Category(UITestCategories.Shell)] | ||
| public void TabSwitchingWorksAfterFlyoutItemRoundTrip() | ||
| { | ||
| App.WaitForElement("Tab1Label"); | ||
|
|
||
| // Round-trip A → B → A | ||
| App.TapShellFlyoutIcon(); | ||
| App.WaitForElement("Section B"); | ||
| App.Tap("Section B"); | ||
| App.WaitForElement("SectionBLabel"); | ||
|
|
||
| App.TapShellFlyoutIcon(); | ||
| App.WaitForElement("Section A"); | ||
| App.Tap("Section A"); | ||
| App.WaitForElement("Tab1Label"); | ||
|
|
||
| // Switch to Tab 2 — must work correctly after the round-trip | ||
| App.Tap("Tab 2"); | ||
| App.WaitForElement("Tab2Label"); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.