fix(nativeframepresenter): fix !AndroidUnloadInactivePages optimizations

src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_Frame.cs

@@ -1,9 +1,12 @@
 using System;
+using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.UI.Xaml;
 using Private.Infrastructure;
 using Microsoft.UI.Xaml.Controls;
-using Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls;
 using Microsoft.UI.Xaml.Navigation;
+using Uno.Disposables;
+using Uno.UI.RuntimeTests.Helpers;
 
 namespace Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls;
 
@@ -489,6 +492,87 @@ public async Task When_Exception_In_OnNavigatedTo()
 		Assert.IsTrue(navigationFailed);
 	}
 
+#if HAS_UNO
+	[TestMethod]
+#if !__ANDROID__
+	[Ignore("This test specifically tests Android's NativeFramePresenter")]
+#endif
+	[DataRow(false)]
+	[DataRow(true)]
+	public async Task When_Navigating_NativeFrame_Pages_Get_Collected(bool backAndForth)
+	{
+		// to clean up residual pages from a previous test run
+		for (int i = 0; i < 10; i++)
+		{
+			GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
+			GC.WaitForPendingFinalizers();
+		}
+
+		FinalizeCounterPage.Reset();
+		var oldUseWinUIBehavior = FeatureConfiguration.Frame.UseWinUIBehavior;
+		var oldIsPoolingEnabled = FeatureConfiguration.Page.IsPoolingEnabled;
+		FeatureConfiguration.Frame.UseWinUIBehavior = false; // WinUI behaviour doesn't cache pages
+		FeatureConfiguration.Page.IsPoolingEnabled = false;
+		using var _ = Disposable.Create(() =>
+		{
+			FeatureConfiguration.Page.IsPoolingEnabled = oldIsPoolingEnabled;
+			FeatureConfiguration.Frame.UseWinUIBehavior = oldUseWinUIBehavior;
+			FinalizeCounterPage.Reset();
+		});
+
+		var SUT = new Frame
+		{
+			Style = Application.Current.Resources["NativeDefaultFrame"] as Style,
+			Width = 200,
+			Height = 200
+		};
+
+		await UITestHelper.Load(SUT);
+
+		// WaitingForIdle doesn't work here, most probably because of native animations. Instead, we use a 3 sec delay
+		SUT.Navigate(typeof(FinalizeCounterPage));
+		await Task.Delay(TimeSpan.FromSeconds(3));
+		SUT.Navigate(typeof(FinalizeCounterPage));
+		await Task.Delay(TimeSpan.FromSeconds(3));
+		SUT.GoBack();
+		await Task.Delay(TimeSpan.FromSeconds(3));
+
+		for (int i = 0; i < 10; i++)
+		{
+			if (backAndForth)
+			{
+				if (i % 2 == 0)
+				{
+					SUT.GoForward();
+				}
+				else
+				{
+					SUT.GoBack();
+				}
+			}
+			else
+			{
+				SUT.Navigate(typeof(FinalizeCounterPage));
+				await Task.Delay(TimeSpan.FromSeconds(3));
+				SUT.BackStack.Clear();
+			}
+
+			await Task.Delay(TimeSpan.FromSeconds(3));
+		}
+
+		for (int i = 0; i < 10; i++)
+		{
+			GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
+			GC.WaitForPendingFinalizers();
+		}
+
+		Assert.AreEqual(backAndForth ? 2 : 12, FinalizeCounterPage.ConstructorCalls);
+		// 10 and not 11 because NativeFramePresenter won't immediately clear its "cache" on
+		// SUT.BackStack.Clear(), but waits for the next SUT.Navigate()
+		Assert.AreEqual(backAndForth ? 0 : 10, FinalizeCounterPage.FinalizerCalls);
+	}
+#endif
+
 	[TestCleanup]
 	public void Cleanup()
 	{
@@ -690,3 +774,28 @@ override void OnNavigatedTo(NavigationEventArgs e)
 		throw new NotSupportedException("Crashed");
 	}
 }
+
+public partial class FinalizeCounterPage : Page
+{
+	private static int _finalizerCalls;
+	public static int FinalizerCalls => _finalizerCalls;
+
+	public static int ConstructorCalls { get; private set; }
+
+	public static void Reset()
+	{
+		Interlocked.Exchange(ref _finalizerCalls, 0);
+		ConstructorCalls = 0;
+	}
+
+	public FinalizeCounterPage()
+	{
+		ConstructorCalls++;
+		Content = new TextBlock { Text = $"FinalizeCounterPage {ConstructorCalls}" };
+	}
+
+	~FinalizeCounterPage()
+	{
+		Interlocked.Increment(ref _finalizerCalls);
+	}
+}

src/Uno.UI/NativeFramePresenter.Android.cs

@@ -3,22 +3,15 @@
 using System.Collections.ObjectModel;
 using System.Collections.Specialized;
 using System.Linq;
-using System.Text;
-using System.Threading;
 using System.Threading.Tasks;
 
-using Windows.UI.Core;
 using Microsoft.UI.Xaml;
 using Microsoft.UI.Xaml.Controls;
 using Microsoft.UI.Xaml.Navigation;
 using Microsoft.UI.Xaml.Media.Animation;
-using Android.App;
 using Android.Views.Animations;
-using Android.Views;
-using Uno.Extensions;
-using Uno.Extensions.Specialized;
-using Uno.Foundation.Logging;
 using Uno.Disposables;
+using Uno.Extensions;
 using Uno.UI.Extensions;
 
 namespace Uno.UI.Controls
@@ -27,15 +20,14 @@ public partial class NativeFramePresenter : Grid // Inheriting from Grid is a ha
 	{
 		private static DependencyProperty BackButtonVisibilityProperty = ToolkitHelper.GetProperty("Uno.UI.Toolkit.CommandBarExtensions", "BackButtonVisibility");
 
-		private readonly Grid _pageStack;
 		private Frame _frame;
 		private bool _isUpdatingStack;
-		private PageStackEntry _currentEntry;
-		private Queue<(PageStackEntry pageEntry, NavigationEventArgs args)> _stackUpdates = new Queue<(PageStackEntry, NavigationEventArgs)>();
+		private (Page page, NavigationTransitionInfo transitionInfo) _currentPage;
+		private readonly Queue<(Page page, NavigationEventArgs args)> _stackUpdates = new Queue<(Page, NavigationEventArgs)>();
+		private CompositeDisposable _subscriptions;
 
 		public NativeFramePresenter()
 		{
-			_pageStack = this;
 		}
 
 		private protected override void OnLoaded()
@@ -52,20 +44,33 @@ private void Initialize(Frame frame)
 				return;
 			}
 
+			global::System.Diagnostics.Debug.Assert(_subscriptions is null);
+			_subscriptions = new CompositeDisposable();
+
 			_frame = frame;
 			_frame.Navigated += OnNavigated;
+			_subscriptions.Add(Disposable.Create(() => _frame.Navigated -= OnNavigated));
+
 			if (_frame.BackStack is ObservableCollection<PageStackEntry> backStack)
 			{
 				backStack.CollectionChanged += OnBackStackChanged;
+				_subscriptions.Add(Disposable.Create(() => backStack.CollectionChanged -= OnBackStackChanged));
 			}
 
 			if (_frame.Content is Page startPage)
 			{
-				_stackUpdates.Enqueue((_frame.CurrentEntry, new NavigationEventArgs(_frame.Content, NavigationMode.New, null, null, null, null)));
+				_stackUpdates.Enqueue((_frame.Content as Page, new NavigationEventArgs(_frame.Content, NavigationMode.New, null, null, null, null)));
 				_ = InvalidateStack();
 			}
 		}
 
+		private protected override void OnUnloaded()
+		{
+			base.OnUnloaded();
+			_subscriptions?.Dispose();
+			_subscriptions = null;
+		}
+
 		private void OnBackStackChanged(object sender, NotifyCollectionChangedEventArgs e)
 		{
 			UpdateBackButtonVisibility();
@@ -88,7 +93,7 @@ private void UpdateBackButtonVisibility()
 
 		private void OnNavigated(object sender, NavigationEventArgs e)
 		{
-			_stackUpdates.Enqueue((_frame.CurrentEntry, e));
+			_stackUpdates.Enqueue((_frame.Content as Page, e));
 
 			_ = InvalidateStack();
 		}
@@ -105,99 +110,77 @@ private async Task InvalidateStack()
 			while (_stackUpdates.Any())
 			{
 				var navigation = _stackUpdates.Dequeue();
-				await UpdateStack(navigation.pageEntry, navigation.args);
+				await UpdateStack(navigation.page, navigation.args.NavigationTransitionInfo);
 			}
 
 			_isUpdatingStack = false;
 		}
 
-		private async Task UpdateStack(PageStackEntry entry, NavigationEventArgs e)
+		private async Task UpdateStack(Page newPage, NavigationTransitionInfo transitionInfo)
 		{
-			var oldEntry = _currentEntry;
-			var newEntry = entry;
-
-			var newPage = newEntry?.Instance;
-			var oldPage = oldEntry?.Instance;
-
-			if (newPage == null || newPage == oldPage)
+			// When AndroidUnloadInactivePages is false, we keep the pages that are still a part of the navigation history
+			// (i.e. in BackStack or ForwardStack) as children and make then invisible instead of removing them. The order
+			// of these "hidden" children is not necessarily similar to BackStack.Concat(ForwardStack), since the
+			// back and forward stacks can be manipulated explicitly beside navigating. We could attempt to maintain
+			// a correspondence between the "hidden" children and the back and forward stacks by listening to their
+			// CollectionChanged events, but this breaks our optimization attempts since these events fire before
+			// Navigated events are fired. For example, in a GoBack action, the BackStack is updated first and we would
+			// remove the element that corresponds to the previously-last element in the BackStack, and then respond to
+			// the Navigated event by making the newly-navigated-to page visible, except that the element we just removed
+			// is the one we want. Therefore, we treat the "hidden" children as a list that we have to walk through to
+			// remove items that are no longer a part of the navigation history. Although costly, navigation is not
+			// a heavily-automated action and is mostly bottlenecked by human reaction times, so it's fine.
+
+			var oldPage = _currentPage.page;
+			var oldTransitionInfo = _currentPage.transitionInfo;
+			_currentPage = (newPage, transitionInfo);
+
+			if (oldPage is not null)
 			{
-				return;
+				if (GetIsAnimated(oldTransitionInfo))
+				{
+					await oldPage.AnimateAsync(GetExitAnimation());
+					oldPage.ClearAnimation();
+				}
+				if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages)
+				{
+					Children.Remove(oldPage);
+				}
+				else
+				{
+					oldPage.Visibility = Visibility.Collapsed;
+				}
 			}
 
-			switch (e.NavigationMode)
+			if (newPage is not null)
 			{
-				case NavigationMode.Forward:
-				case NavigationMode.New:
-				case NavigationMode.Refresh:
-					_pageStack.Children.Add(newPage);
-					if (GetIsAnimated(newEntry))
-					{
-						await newPage.AnimateAsync(GetEnterAnimation());
-						newPage.ClearAnimation();
-					}
-					if (oldPage is not null)
-					{
-						if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages)
-						{
-							_pageStack.Children.Remove(oldPage);
-						}
-						else
-						{
-							oldPage.Visibility = Visibility.Collapsed;
-						}
-					}
-					break;
-				case NavigationMode.Back:
-					if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages)
-					{
-						_pageStack.Children.Insert(0, newPage);
-					}
-					else
-					{
-						newPage.Visibility = Visibility.Visible;
-					}
-					if (GetIsAnimated(oldEntry))
-					{
-						await oldPage.AnimateAsync(GetExitAnimation());
-						oldPage.ClearAnimation();
-					}
-
-					if (oldPage != null)
-					{
-						_pageStack.Children.Remove(oldPage);
-					}
-
-					if (!FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages)
-					{
-						// Remove pages from the grid that may have been removed from the BackStack list
-						// Those items are not removed on BackStack list changes to avoid interfering with the GoBack method's behavior.
-						for (var pageIndex = _pageStack.Children.Count - 1; pageIndex >= 0; pageIndex--)
-						{
-							var page = _pageStack.Children[pageIndex];
-							if (page == newPage)
-							{
-								break;
-							}
-
-							_pageStack.Children.Remove(page);
-						}
-
-						//In case we cleared the whole stack. This should never happen
-						if (_pageStack.Children.Count == 0)
-						{
-							_pageStack.Children.Insert(0, newPage);
-						}
-					}
-
-					break;
+				if (Children.Contains(newPage))
+				{
+					newPage.Visibility = Visibility.Visible;
+				}
+				else
+				{
+					Children.Add(newPage);
+				}
+				if (GetIsAnimated(transitionInfo))
+				{
+					await newPage.AnimateAsync(GetEnterAnimation());
+					newPage.ClearAnimation();
+				}
 			}
 
-			_currentEntry = newEntry;
+			if (!FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages)
+			{
+				var pagesStillInHistory = _frame.BackStack.Select(entry => entry.Instance).ToHashSet();
+				pagesStillInHistory.AddRange(_frame.ForwardStack.Select(entry => entry.Instance));
+				pagesStillInHistory.Add(newPage);
+				Children.Remove(element => !pagesStillInHistory.Contains(element));
+			}
 		}
 
-		private static bool GetIsAnimated(PageStackEntry entry)
+		private static bool GetIsAnimated(NavigationTransitionInfo transitionInfo)
 		{
-			return !(entry.NavigationTransitionInfo is SuppressNavigationTransitionInfo);
+			return !(transitionInfo is SuppressNavigationTransitionInfo);
 		}
 
 		private static Animation GetEnterAnimation()