[Android] Correctly dispose TabbedPageRenderer (#4974)

* Correctly dispose TabbedPageRenderer.
The fragments created are now removed and the listeners are now correctly cleaned during dispose.

* Added issue UI test.

* Fix carousel fragment manager and pager disposing.

* Fix test 2338 implementation.

* Apply review.

* Set to null fragmentManager on dispose.

* Use GetFragmentManager extension method.

Co-Authored-By: Shane Neuville <shane94@hotmail.com>

* Revert some changes.

* Apply review comment.

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2338.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Diagnostics;
 using System.Threading.Tasks;
 using Xamarin.Forms.CustomAttributes;
@@ -157,7 +157,6 @@ namespace Xamarin.Forms.Controls.Issues
 				protected override void OnAppearing() => Debug.WriteLine($"OnAppearing: {_permutations}");
 				protected override void OnDisappearing() => Debug.WriteLine($"OnDisappearing: {_permutations}");
 			}
-
 		}
 
 		[Preserve(AllMembers = true)]
@@ -170,7 +169,6 @@ namespace Xamarin.Forms.Controls.Issues
 				PushAsync(new InternalPage(30));
 
 				var otherPage = new InternalPage(40);
-				PushAsync(otherPage);
 
 				otherPage.Appearing += async (object sender, EventArgs e) =>
 				{
@@ -182,7 +180,9 @@ namespace Xamarin.Forms.Controls.Issues
 					Application.Current.MainPage.BindingContext = new object();
 				};
 
+				PushAsync(otherPage);
 			}
+
 			protected override void OnAppearing() => Debug.WriteLine($"OnAppearing: Issue2338");
 			protected override void OnDisappearing() => Debug.WriteLine($"OnDisappearing: Issue2338");
 
@@ -336,10 +336,6 @@ namespace Xamarin.Forms.Controls.Issues
 					await Task.Delay(500);
 					var contentPage = new ContentPage();
 
-#pragma warning disable 4014
-					Detail.Navigation.PushAsync(contentPage);
-#pragma warning restore 4014
-
 					contentPage.Appearing += (_, __) =>
 					{
 						var navPage = new NavigationPage(new ContentPage() { Title = "Details" });
@@ -350,6 +346,10 @@ namespace Xamarin.Forms.Controls.Issues
 
 						navPage.PushAsync(new ContentPage() { Title = "Details 2" });
 					};
+
+#pragma warning disable 4014
+					Detail.Navigation.PushAsync(contentPage);
+#pragma warning restore 4014
 				}
 			}
 		}

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue4973.cs

@@ -0,0 +1,74 @@
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+
+#if UITEST
+using System;
+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Github, 4973, "TabbedPage nav tests", PlatformAffected.Android)]
+	public class Issue4973 : TestTabbedPage
+	{
+		protected override void Init()
+		{
+			Children.Add(new TabbedPage
+			{
+				Title = "Tab1",
+				Children =
+				{
+					new ContentPage
+					{
+						Title = "InnerTab1"
+					},
+					new ContentPage
+					{
+						Title = "InnerTab2"
+					}
+				}
+			});
+
+			Children.Add(new ContentPage
+			{
+				Title = "Tab2"
+			});
+
+			Children.Add(new ContentPage
+			{
+				Title = "Tab3"
+			});
+
+			Children.Add(new ContentPage
+			{
+				Title = "Tab4"
+			});
+
+			Children.Add(new ContentPage
+			{
+				Title = "Tab5",
+				Content = new Label
+				{
+					Text = "Test"
+				}
+			});
+		}
+
+#if UITEST && __ANDROID__
+		[Test]
+		public void Issue4973Test()
+		{
+			RunningApp.Tap(q => q.Text("Tab5"));
+
+			RunningApp.WaitForElement(q => q.Text("Test"));
+
+			GarbageCollectionHelper.Collect();
+
+			RunningApp.Tap(q => q.Text("Tab1"));
+
+			RunningApp.Tap(q => q.Text("Tab2"));
+		}
+#endif
+	}
+}

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems

@@ -513,6 +513,8 @@
       <SubType>Code</SubType>
     </Compile>
     <Compile Include="$(MSBuildThisFileDirectory)Issue4600.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Issue4973.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Issue5252.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue5057.xaml.cs">
       <DependentUpon>Issue5057.xaml</DependentUpon>
       <SubType>Code</SubType>

Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs

@@ -2,16 +2,18 @@ using System;
 using System.Collections.Specialized;
 using System.ComponentModel;
 using Android.Content;
+using Android.Support.V4.App;
 using Android.Support.V4.View;
 using Android.Views;
 
 namespace Xamarin.Forms.Platform.Android.AppCompat
 {
-	public class CarouselPageRenderer : VisualElementRenderer<CarouselPage>, ViewPager.IOnPageChangeListener
+	public class CarouselPageRenderer : VisualElementRenderer<CarouselPage>, ViewPager.IOnPageChangeListener, IManageFragments
 	{
 		bool _disposed;
 		FormsViewPager _viewPager;
 		Page _previousPage;
+		FragmentManager _fragmentManager;
 
 		public CarouselPageRenderer(Context context) : base(context)
 		{
@@ -35,6 +37,14 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 		IPageController PageController => Element as IPageController;
 
+		FragmentManager FragmentManager => _fragmentManager ?? (_fragmentManager = Context.GetFragmentManager());
+
+		void IManageFragments.SetFragmentManager(FragmentManager childFragmentManager)
+		{
+			if (_fragmentManager == null)
+				_fragmentManager = childFragmentManager;
+		}
+
 		void ViewPager.IOnPageChangeListener.OnPageSelected(int position)
 		{
 			Element.CurrentPage = Element.Children[position];
@@ -68,7 +78,8 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 				RemoveAllViews();
 
 				_previousPage = null;
-			
+				_fragmentManager = null;
+
 				if (Element?.Children != null)
 				{
 					foreach (ContentPage pageToRemove in Element.Children)
@@ -117,15 +128,22 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 			if (e.NewElement != null)
 			{
-				FormsViewPager pager =
-					_viewPager =
-					new FormsViewPager(activity)
-					{
-						OverScrollMode = OverScrollMode.Never,
-						EnableGesture = true,
-						LayoutParameters = new ViewGroup.LayoutParams(ViewGroup.LayoutParams.MatchParent, ViewGroup.LayoutParams.MatchParent),
-						Adapter = new FormsFragmentPagerAdapter<ContentPage>(e.NewElement, activity.SupportFragmentManager) { CountOverride = e.NewElement.Children.Count }
-					};
+				if (_viewPager != null)
+				{
+					_viewPager.RemoveOnPageChangeListener(this);
+
+					ViewGroup.RemoveView(_viewPager);
+
+					_viewPager.Dispose();
+				}
+
+				FormsViewPager pager = _viewPager = new FormsViewPager(activity)
+				{
+					OverScrollMode = OverScrollMode.Never,
+					EnableGesture = true,
+					LayoutParameters = new LayoutParams(LayoutParams.MatchParent, LayoutParams.MatchParent),
+					Adapter = new FormsFragmentPagerAdapter<ContentPage>(e.NewElement, FragmentManager) { CountOverride = e.NewElement.Children.Count }
+				};
 				pager.Id = Platform.GenerateViewId();
 				pager.AddOnPageChangeListener(this);
 
@@ -145,7 +163,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 		{
 			base.OnElementPropertyChanged(sender, e);
 
-			if (e.PropertyName == "CurrentPage")
+			if (e.PropertyName == nameof(Element.CurrentPage))
 				ScrollToCurrentPage();
 		}
 
@@ -180,4 +198,4 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			_viewPager.SetCurrentItem(Element.Children.IndexOf(Element.CurrentPage), true);
 		}
 	}
-}
+}

Xamarin.Forms.Platform.Android/AppCompat/FormsFragmentPagerAdapter.cs

@@ -1,17 +1,24 @@
-using Android.OS;
+using System.Collections.Generic;
+using Android.OS;
 using Android.Support.V4.App;
 using Java.Lang;
 using Xamarin.Forms.Internals;
+using FragmentTransit = Android.App.FragmentTransit;
 
 namespace Xamarin.Forms.Platform.Android.AppCompat
 {
 	internal class FormsFragmentPagerAdapter<T> : FragmentPagerAdapter where T : Page
 	{
 		MultiPage<T> _page;
+		FragmentManager _fragmentManager;
+		List<Fragment> _fragments;
+		bool _disposed;
 
 		public FormsFragmentPagerAdapter(MultiPage<T> page, FragmentManager fragmentManager) : base(fragmentManager)
 		{
 			_page = page;
+			_fragmentManager = fragmentManager;
+			_fragments = new List<Fragment>();
 		}
 
 		public override int Count => CountOverride;
@@ -20,7 +27,11 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 		public override Fragment GetItem(int position)
 		{
-			return FragmentContainer.CreateInstance(_page.Children[position]);
+			var fragment = FragmentContainer.CreateInstance(_page.Children[position]);
+
+			_fragments.Add(fragment);
+
+			return fragment;
 		}
 
 		public override long GetItemId(int position)
@@ -31,12 +42,17 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 		public override int GetItemPosition(Object objectValue)
 		{
 			var fragContainer = objectValue as FragmentContainer;
-			if (fragContainer != null && fragContainer.Page != null)
+			
+			if (fragContainer?.Page != null)
 			{
 				int index = _page.Children.IndexOf(fragContainer.Page);
+				
 				if (index >= 0)
 					return index;
 			}
+
+			_fragments.Remove(fragContainer);
+
 			return PositionNone;
 		}
 
@@ -52,8 +68,36 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 		protected override void Dispose(bool disposing)
 		{
+			if (_disposed)
+			{
+				return;
+			}
+
 			if (disposing)
+			{
+				_disposed = true;
+
 				_page = null;
+
+				if (!_fragmentManager.IsDestroyed)
+				{
+					FragmentTransaction transaction = _fragmentManager.BeginTransactionEx();
+			
+					foreach (Fragment fragment in _fragments)
+					{
+						transaction.RemoveEx(fragment);
+						transaction.SetTransitionEx((int)FragmentTransit.None);
+					}
+
+					transaction.CommitAllowingStateLossEx();
+
+					_fragmentManager.ExecutePendingTransactionsEx();
+
+					_fragments = null;
+					_fragmentManager = null;
+				}
+			}
+
 			base.Dispose(disposing);
 		}
 	}