Fix several Android memory leaks

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

@@ -0,0 +1,75 @@
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+using System.Threading;
+using System;
+using System.Threading.Tasks;
+using Xamarin.Forms.Maps;
+
+#if UITEST
+using Xamarin.UITest;
+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Bugzilla, 39489, "Memory leak when using NavigationPage with Maps", PlatformAffected.Android)]
+	public class Bugzilla39489 : TestNavigationPage
+	{
+		protected override void Init()
+		{
+			PushAsync(new Bz39489Content());
+		}
+
+		#if UITEST
+		[Test]
+		public async void Bugzilla39458Test()
+		{
+			// Original bug report (https://bugzilla.xamarin.com/show_bug.cgi?id=39489) had a crash (OOM) after 25-30
+			// page loads. Obviously it's going to depend heavily on the device and amount of available memory, but
+			// if this starts failing before 50 we'll know we've sprung another serious leak
+			int iterations = 50;
+
+			for (int n = 0; n < iterations; n++)
+			{
+				RunningApp.WaitForElement(q => q.Marked("New Page"));
+				RunningApp.Tap(q => q.Marked("New Page"));
+				RunningApp.WaitForElement(q => q.Marked("New Page"));
+				await Task.Delay(1000);
+				RunningApp.Back();
+			}
+		}
+		#endif
+	}
+
+	[Preserve(AllMembers = true)]
+	public class Bz39489Content : ContentPage
+	{
+		public Bz39489Content()
+		{
+			var button = new Button { Text = "New Page" };
+
+			var gcbutton = new Button { Text = "GC" };
+
+			var map = new Map();
+
+			button.Clicked += Button_Clicked;
+			gcbutton.Clicked += GCbutton_Clicked;
+
+			Content = new StackLayout { Children = { button, gcbutton, map } };
+		}
+
+		void GCbutton_Clicked(object sender, EventArgs e)
+		{
+			System.Diagnostics.Debug.WriteLine(">>>>>>>> Running Garbage Collection");
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+			System.Diagnostics.Debug.WriteLine($">>>>>>>> GC.GetTotalMemory = {GC.GetTotalMemory(true):n0}");
+		}
+
+		void Button_Clicked(object sender, EventArgs e)
+		{
+			Navigation.PushAsync(new Bz39489Content());
+		}
+	}
+}

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

@@ -437,6 +437,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Issue55555.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla41029.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla39908.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Bugzilla39489.cs" />
   </ItemGroup>
   <ItemGroup>
     <EmbeddedResource Include="$(MSBuildThisFileDirectory)Bugzilla22229.xaml">

Xamarin.Forms.Maps.Android/MapRenderer.cs

@@ -59,25 +59,30 @@ protected override MapView CreateNativeControl()
 
 		protected override void Dispose(bool disposing)
 		{
-			if (disposing && !_disposed)
+			if (_disposed)
 			{
-				_disposed = true;
+				return;
+			}
 
-				Map mapModel = Element;
-				if (mapModel != null)
+			_disposed = true;
+
+			if (disposing)
+			{
+				if (Element != null)
 				{
 					MessagingCenter.Unsubscribe<Map, MapSpan>(this, MoveMessageName);
-					((ObservableCollection<Pin>)mapModel.Pins).CollectionChanged -= OnCollectionChanged;
+					((ObservableCollection<Pin>)Element.Pins).CollectionChanged -= OnCollectionChanged;
 				}
 
-				GoogleMap gmap = NativeMap;
-				if (gmap == null)
-				{
-					return;
-				}
-				gmap.MyLocationEnabled = false;
-				gmap.InfoWindowClick -= MapOnMarkerClick;
-				gmap.Dispose();
+				if (NativeMap != null)
+ 				{
+ 					NativeMap.MyLocationEnabled = false;
+ 					NativeMap.SetOnCameraChangeListener(null);
+ 					NativeMap.InfoWindowClick -= MapOnMarkerClick;
+ 					NativeMap.Dispose();
+  				}		  				
+
+				Control?.OnDestroy();
 			}
 
 			base.Dispose(disposing);
@@ -265,6 +270,7 @@ void MoveToRegion(MapSpan span, bool animate)
 			catch (IllegalStateException exc)
 			{
 				System.Diagnostics.Debug.WriteLine("MoveToRegion exception: " + exc);
+				Log.Warning("Xamarin.Forms MapRenderer", $"MoveToRegion exception: {exc}");
 			}
 		}
 

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

@@ -71,6 +71,13 @@ protected override void Dispose(bool disposing)
 
 			if (disposing)
 			{
+				if (Control != null)
+				{
+					Control.SetOnClickListener(null);
+					Control.RemoveOnAttachStateChangeListener(this);
+					Control.Tag = null;
+					_textColorSwitcher = null;
+				}
 			}
 
 			base.Dispose(disposing);

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

@@ -1,9 +1,9 @@
 using System;
 using Android.OS;
 using Android.Runtime;
-using Android.Support.V4.App;
 using Android.Views;
 using AView = Android.Views.View;
+using Fragment = Android.Support.V4.App.Fragment;
 
 namespace Xamarin.Forms.Platform.Android.AppCompat
 {
@@ -75,7 +75,7 @@ public override AView OnCreateView(LayoutInflater inflater, ViewGroup container,
 
 			return null;
 		}
-		
+
 		public override void OnDestroyView()
 		{
 			if (Page != null)
@@ -90,18 +90,16 @@ public override void OnDestroyView()
 					_visualElementRenderer.Dispose();
 				}
 
-				if (_pageContainer != null && _pageContainer.Handle != IntPtr.Zero)
-				{
-					_pageContainer.RemoveFromParent();
-					_pageContainer.Dispose();
-				}
+				// We do *not* eagerly dispose of the _pageContainer here; doing so  causes a memory leak 
+				// if animated fragment transitions are enabled (it removes some info that the animation's 
+				// onAnimationEnd handler requires to properly clean things up)
+				// Instead, we let the garbage collector pick it up later, when we can be sure it's safe
 
 				Page?.ClearValue(Android.Platform.RendererProperty);
 			}
 
 			_onCreateCallback = null;
 			_visualElementRenderer = null;
-			_pageContainer = null;
 
 			base.OnDestroyView();
 		}

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

@@ -41,6 +41,9 @@ public class NavigationPageRenderer : VisualElementRenderer<NavigationPage>, IMa
 		ToolbarTracker _toolbarTracker;
 		bool _toolbarVisible;
 
+		// The following is based on https://android.googlesource.com/platform/frameworks/support/+/refs/heads/master/v4/java/android/support/v4/app/FragmentManager.java#849
+		const int TransitionDuration = 220;
+
 		public NavigationPageRenderer()
 		{
 			AutoPackage = false;
@@ -71,7 +74,7 @@ Page Current
 			}
 		}
 
-		FragmentManager FragmentManager => _fragmentManager ?? (_fragmentManager = ((FormsAppCompatActivity)Context).SupportFragmentManager);
+		FragmentManager FragmentManager	=> _fragmentManager ?? (_fragmentManager = ((FormsAppCompatActivity)Context).SupportFragmentManager);
 
 		IPageController PageController => Element as IPageController;
 
@@ -132,14 +135,14 @@ protected override void Dispose(bool disposing)
 
 				if (Element != null)
 				{
-					foreach(Element element in PageController.InternalChildren)
+					foreach (Element element in PageController.InternalChildren)
 					{
 						var child = element as VisualElement;
 						if (child == null)
 						{
 							continue;
 						}
-							
+
 						IVisualElementRenderer renderer = Android.Platform.GetRenderer(child);
 						renderer?.Dispose();
 					}
@@ -320,7 +323,7 @@ protected virtual void SetupPageTransition(FragmentTransaction transaction, bool
 			else
 				transaction.SetTransition((int)FragmentTransit.FragmentClose);
 		}
-
+		
 		internal int GetNavBarHeight()
 		{
 			if (!ToolbarVisible)
@@ -501,7 +504,7 @@ void RegisterToolbar()
 				ToolbarNavigationClickListener = new ClickListener(Element)
 			};
 
-			if (_drawerListener != null) 
+			if (_drawerListener != null)
 			{
 				_drawerLayout.RemoveDrawerListener(_drawerListener);
 			}
@@ -569,6 +572,12 @@ Task<bool> SwitchContentAsync(Page view, bool animated, bool removed = false, bo
 			var tcs = new TaskCompletionSource<bool>();
 			Fragment fragment = FragmentContainer.CreateInstance(view);
 			FragmentManager fm = FragmentManager;
+
+#if DEBUG
+			// Enables logging of moveToState operations to logcat
+			FragmentManager.EnableDebugLogging(true);
+#endif
+
 			List<Fragment> fragments = _fragmentStack;
 
 			Current = view;
@@ -599,7 +608,7 @@ Task<bool> SwitchContentAsync(Page view, bool animated, bool removed = false, bo
 						transaction.Remove(currentToRemove);
 						popPage = popToRoot;
 					}
-
+					
 					Fragment toShow = fragments.Last();
 					// Execute pending transactions so that we can be sure the fragment list is accurate.
 					fm.ExecutePendingTransactions();
@@ -621,7 +630,7 @@ Task<bool> SwitchContentAsync(Page view, bool animated, bool removed = false, bo
 
 			// The fragment transitions don't really SUPPORT telling you when they end
 			// There are some hacks you can do, but they actually are worse than just doing this:
-			
+
 			if (animated)
 			{
 				if (!removed)
@@ -633,12 +642,15 @@ Task<bool> SwitchContentAsync(Page view, bool animated, bool removed = false, bo
 				else if (_drawerToggle != null && ((INavigationPageController)Element).StackDepth == 2)
 					AnimateArrowOut();
 
-				Device.StartTimer(TimeSpan.FromMilliseconds(200), () =>
+				Device.StartTimer(TimeSpan.FromMilliseconds(TransitionDuration), () =>
 				{
 					tcs.TrySetResult(true);
-					fragment.UserVisibleHint = true;
+					fragment.UserVisibleHint = !removed;
 					if (removed)
+					{
 						UpdateToolbar();
+					}
+
 					return false;
 				});
 			}
@@ -647,16 +659,17 @@ Task<bool> SwitchContentAsync(Page view, bool animated, bool removed = false, bo
 				Device.StartTimer(TimeSpan.FromMilliseconds(1), () =>
 				{
 					tcs.TrySetResult(true);
-					fragment.UserVisibleHint = true;
+					fragment.UserVisibleHint = !removed;
 					UpdateToolbar();
+
 					return false;
 				});
 			}
 
 			Context.HideKeyboard(this);
 			((Platform)Element.Platform).NavAnimationInProgress = false;
 
-			// 200ms is how long the animations are, and they are "reversible" in the sense that starting another one slightly before it's done is fine
+			// TransitionDuration is how long the built-in animations are, and they are "reversible" in the sense that starting another one slightly before it's done is fine
 
 			return tcs.Task;
 		}
@@ -821,4 +834,4 @@ public void OnDrawerStateChanged(int newState)
 			}
 		}
 	}
-}
+}

Xamarin.Forms.Platform.Android/ViewRenderer.cs

@@ -76,7 +76,8 @@ protected override void Dispose(bool disposing)
 			{
 				if (Control != null && ManageNativeControlLifetime)
 				{
-					Control.RemoveFromParent();
+					Control.OnFocusChangeListener = null;
+					RemoveView(Control);
 					Control.Dispose();
 					Control = null;
 				}

Xamarin.Forms.Platform.Android/VisualElementRenderer.cs

@@ -229,6 +229,9 @@ protected override void Dispose(bool disposing)
 
 			if (disposing)
 			{
+				SetOnClickListener(null);
+				SetOnTouchListener(null);
+
 				if (Tracker != null)
 				{
 					Tracker.Dispose();
@@ -273,6 +276,8 @@ protected override void Dispose(bool disposing)
 					if (Platform.GetRenderer(Element) == this)
 						Platform.SetRenderer(Element, null);
 
+					(Element as IElementController).EffectControlProvider = null;
+
 					Element = null;
 				}
 			}