[Android] Memory leak when MasterDetailPage Detail set to NavigationPage (#239)

* Create repro

* Remove unnecessary cast

* Add null checks on weak references in PageContainer

* Remove master/detail fragments from manager when switching master/detail pages
Separate renderer ViewGroup removal from renderer disposal in FragmentContainer
Separate PageContainer disposal from renderer disposal in FragmentContainer
Remove Drawer Listener for NavigationPageRenderer in Dispose

* Fix missing spaces;
Add explicit SPACE_BEFORE_IF_PARENTHESES settings to DotSettings file

* Remove javascript rules

* Remove usage of .ForEach()

9 files changed, 234 insertions, 32 deletions

diff --git a/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj b/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj
index 1c32b5cb..25f896f8 100644
--- a/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj
+++ b/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj
@@ -45,7 +45,6 @@
     <AndroidUseSharedRuntime>False</AndroidUseSharedRuntime>
     <AndroidLinkMode>Full</AndroidLinkMode>
     <JavaMaximumHeapSize>1G</JavaMaximumHeapSize>
-    <AndroidLinkSkip />
     <EmbedAssembliesIntoApk>True</EmbedAssembliesIntoApk>
     <BundleAssemblies>False</BundleAssemblies>
     <CustomCommands>
diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs
new file mode 100644
index 00000000..543a4cab
--- /dev/null
+++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs
@@ -0,0 +1,140 @@
+using System;
+using System.Collections.Generic;
+using System.Diagnostics;
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Bugzilla, 40955, "Memory leak with FormsAppCompatActivity and NavigationPage", PlatformAffected.Android)]
+	public class Bugzilla40955 : TestMasterDetailPage
+	{
+		const string DestructorMessage = "NavigationPageEx Destructor called";
+		const string Page1Title = "Page1";
+		const string Page2Title = "Page2";
+		const string Page3Title = "Page3";
+
+		protected override void Init()
+		{
+			var masterPage = new MasterPage();
+			Master = masterPage;
+			masterPage.ListView.ItemSelected += (sender, e) =>
+			{
+				var item = e.SelectedItem as MasterPageItem;
+				if (item != null)
+				{
+					Detail = new NavigationPageEx((Page)Activator.CreateInstance(item.TargetType));
+					masterPage.ListView.SelectedItem = null;
+					IsPresented = false;
+				}
+			};
+
+			Detail = new NavigationPageEx(new _409555_Page1());
+		}
+
+		[Preserve(AllMembers = true)]
+		public class MasterPageItem
+		{
+			public string IconSource { get; set; }
+
+			public Type TargetType { get; set; }
+
+			public string Title { get; set; }
+		}
+
+		[Preserve(AllMembers = true)]
+		public class MasterPage : ContentPage
+		{
+			public MasterPage()
+			{
+				Title = "Menu";
+				ListView = new ListView { VerticalOptions = LayoutOptions.FillAndExpand, SeparatorVisibility = SeparatorVisibility.None };
+
+				ListView.ItemTemplate = new DataTemplate(() =>
+				{
+					var ic = new ImageCell();
+					ic.SetBinding(TextCell.TextProperty, "Title");
+					return ic;
+				});
+
+				Content = new StackLayout
+				{
+					Children = { ListView }
+				};
+
+				var masterPageItems = new List<MasterPageItem>();
+				masterPageItems.Add(new MasterPageItem
+				{
+					Title = Page1Title,
+					TargetType = typeof(_409555_Page1)
+				});
+				masterPageItems.Add(new MasterPageItem
+				{
+					Title = Page2Title,
+					TargetType = typeof(_409555_Page2)
+				});
+				masterPageItems.Add(new MasterPageItem
+				{
+					Title = Page3Title,
+					TargetType = typeof(_409555_Page3)
+				});
+
+				ListView.ItemsSource = masterPageItems;
+			}
+
+			public ListView ListView { get; }
+		}
+
+		[Preserve(AllMembers = true)]
+		public class NavigationPageEx : NavigationPage
+		{
+			public NavigationPageEx(Page root) : base(root)
+			{
+			}
+
+			~NavigationPageEx()
+			{
+				Debug.WriteLine(DestructorMessage);
+			}
+		}
+
+		[Preserve(AllMembers = true)]
+		public class _409555_Page1 : ContentPage
+		{
+			public _409555_Page1()
+			{
+				Title = Page1Title;
+				Content = new StackLayout { Children = { new Label { Text = "Open the drawer menu and select Page2" } } };
+			}
+		}
+
+		[Preserve(AllMembers = true)]
+		public class _409555_Page2 : ContentPage
+		{
+			public _409555_Page2()
+			{
+				Title = Page2Title;
+				Content = new StackLayout { Children = { new Label { Text = "Open the drawer menu and select Page3" } } };
+			}
+		}
+
+		[Preserve(AllMembers = true)]
+		public class _409555_Page3 : ContentPage
+		{
+			public _409555_Page3()
+			{
+				Title = Page3Title;
+				Content = new StackLayout { Children = { new Label { Text = $"The console should have displayed the text '{DestructorMessage}' at least once. If not, this test has failed." } } };
+			}
+
+			protected override void OnAppearing()
+			{
+				base.OnAppearing();
+				GC.Collect();
+				GC.Collect();
+				GC.Collect();
+			}
+		}
+	}
+}
\ No newline at end of file
diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
index eac49a1e..af052536 100644
--- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
+++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
@@ -104,6 +104,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40185.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40333.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla31806.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40955.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla41078.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40998.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla41424.cs" />
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs b/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs
index 34abfda8..3d85ee07 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs
@@ -75,25 +75,31 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 			return null;
 		}
-
+		
 		public override void OnDestroyView()
 		{
 			if (Page != null)
 			{
-				IVisualElementRenderer renderer = _visualElementRenderer;
-				PageContainer container = _pageContainer;
-
-				if (container.Handle != IntPtr.Zero && renderer.ViewGroup.Handle != IntPtr.Zero)
+				if (_visualElementRenderer != null)
 				{
-					container.RemoveFromParent();
-					renderer.ViewGroup.RemoveFromParent();
-					Page.ClearValue(Android.Platform.RendererProperty);
+					if (_visualElementRenderer.ViewGroup.Handle != IntPtr.Zero)
+					{
+						_visualElementRenderer.ViewGroup.RemoveFromParent();
+					}
 
-					container.Dispose();
-					renderer.Dispose();
+					_visualElementRenderer.Dispose();
 				}
+
+				if (_pageContainer != null && _pageContainer.Handle != IntPtr.Zero)
+				{
+					_pageContainer.RemoveFromParent();
+					_pageContainer.Dispose();
+				}
+
+				Page?.ClearValue(Android.Platform.RendererProperty);
 			}
 
+			_onCreateCallback = null;
 			_visualElementRenderer = null;
 			_pageContainer = null;
 
@@ -108,9 +114,9 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 				return;
 
 			if (hidden)
-				PageController.SendDisappearing();
+				PageController?.SendDisappearing();
 			else
-				PageController.SendAppearing();
+				PageController?.SendAppearing();
 		}
 
 		public override void OnPause()
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs
index e001389f..102bac53 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs
@@ -12,6 +12,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 		FragmentManager _fragmentManager;
 		readonly bool _isMaster;
 		readonly MasterDetailPage _parent;
+		Fragment _currentFragment;
 
 		public MasterDetailContainer(MasterDetailPage parent, bool isMaster, Context context) : base(parent, isMaster, context)
 		{
@@ -59,17 +60,30 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 			if (page == null)
 			{
-				// Not a NavigationPage or TabbedPage? Just do the normal thing
+				// The thing we're adding is not a NavigationPage or TabbedPage, so we can just use the old AddChildView 
+
+				if (_currentFragment != null)
+				{
+					// But first, if the previous occupant of this container was a fragment, we need to remove it properly
+					FragmentTransaction transaction = FragmentManager.BeginTransaction();
+					transaction.DisallowAddToBackStack();
+					transaction.Remove(_currentFragment);
+					transaction.SetTransition((int)FragmentTransit.None);
+					transaction.Commit();
+
+					_currentFragment = null;
+				}
+				
 				base.AddChildView(childView);
 			}
 			else
 			{
 				// The renderers for NavigationPage and TabbedPage both host fragments, so they need to be wrapped in a 
 				// FragmentContainer in order to get isolated fragment management
-
 				Fragment fragment = FragmentContainer.CreateInstance(page);
 				
 				var fc = fragment as FragmentContainer;
+
 				fc?.SetOnCreateCallback(pc =>
 				{
 					_pageContainer = pc;
@@ -78,9 +92,17 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 				FragmentTransaction transaction = FragmentManager.BeginTransaction();
 				transaction.DisallowAddToBackStack();
+
+				if (_currentFragment != null)
+				{
+					transaction.Remove(_currentFragment);
+				}
+
 				transaction.Add(Id, fragment);
 				transaction.SetTransition((int)FragmentTransit.FragmentOpen);
 				transaction.Commit();
+
+				_currentFragment = fragment;
 			}
 		}
 
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs
index 50e99e50..6740285c 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs
@@ -268,7 +268,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 				UpdateMaster();
 			else if (e.PropertyName == "Detail")
 				UpdateDetail();
-			else if (e.PropertyName == "IsGestureEnabled")
+			else if (e.PropertyName == MasterDetailPage.IsGestureEnabledProperty.PropertyName)
 				SetGestureState();
 			else if (e.PropertyName == MasterDetailPage.IsPresentedProperty.PropertyName)
 			{
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs
index cb4fcdbb..c833e0c5 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs
@@ -132,11 +132,14 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 				if (Element != null)
 				{
-					for (var i = 0; i < PageController.InternalChildren.Count; i++)
+					foreach(Element element in PageController.InternalChildren)
 					{
-						var child = PageController.InternalChildren[i] as VisualElement;
+						var child = element as VisualElement;
 						if (child == null)
+						{
 							continue;
+						}
+							
 						IVisualElementRenderer renderer = Android.Platform.GetRenderer(child);
 						renderer?.Dispose();
 					}
@@ -148,7 +151,6 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 					navController.PopToRootRequested -= OnPoppedToRoot;
 					navController.InsertPageBeforeRequested -= OnInsertPageBeforeRequested;
 					navController.RemovePageRequested -= OnRemovePageRequested;
-					PageController.SendDisappearing();
 				}
 
 				if (_toolbarTracker != null)
@@ -165,6 +167,13 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 					_toolbar = null;
 				}
 
+				if (_drawerLayout != null && _drawerListener != null)
+				{
+					_drawerLayout.RemoveDrawerListener(_drawerListener);
+				}
+
+				_drawerToggle = null;
+
 				Current = null;
 
 				Device.Info.PropertyChanged -= DeviceInfoPropertyChanged;
@@ -237,7 +246,10 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 				navController.RemovePageRequested += OnRemovePageRequested;
 
 				// If there is already stuff on the stack we need to push it
-				((INavigationPageController)e.NewElement).StackCopy.Reverse().ForEach(p => PushViewAsync(p, false));
+				foreach (Page page in navController.StackCopy.Reverse())
+				{
+					PushViewAsync(page, false);
+				}
 			}
 		}
 
@@ -447,6 +459,9 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			RemovePage(e.Page);
 		}
 
+		private DrawerMultiplexedListener _drawerListener;
+		private DrawerLayout _drawerLayout;
+
 		void RegisterToolbar()
 		{
 			Context context = Context;
@@ -478,13 +493,20 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			if (renderer == null)
 				return;
 
-			var drawerLayout = (DrawerLayout)renderer;
-			_drawerToggle = new ActionBarDrawerToggle((Activity)context, drawerLayout, bar, global::Android.Resource.String.Ok, global::Android.Resource.String.Ok)
+			_drawerLayout = (DrawerLayout)renderer;
+			_drawerToggle = new ActionBarDrawerToggle((Activity)context, _drawerLayout, bar, global::Android.Resource.String.Ok, global::Android.Resource.String.Ok)
 			{
 				ToolbarNavigationClickListener = new ClickListener(Element)
 			};
 
-			drawerLayout.AddDrawerListener(new DrawerMultiplexedListener { Listeners = { _drawerToggle, renderer } });
+			if (_drawerListener != null) 
+			{
+				_drawerLayout.RemoveDrawerListener(_drawerListener);
+			}
+
+			_drawerListener = new DrawerMultiplexedListener { Listeners = { _drawerToggle, renderer } };
+			_drawerLayout.AddDrawerListener(_drawerListener);
+
 			_drawerToggle.DrawerIndicatorEnabled = true;
 		}
 
@@ -542,7 +564,6 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 		Task<bool> SwitchContentAsync(Page view, bool animated, bool removed = false, bool popToRoot = false)
 		{
-			var activity = (FormsAppCompatActivity)Context;
 			var tcs = new TaskCompletionSource<bool>();
 			Fragment fragment = FragmentContainer.CreateInstance(view);
 			FragmentManager fm = FragmentManager;
diff --git a/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs b/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs
index f326cb3c..3fbfa096 100644
--- a/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs
@@ -14,6 +14,7 @@ namespace Xamarin.Forms.Platform.Android
 		static ViewPropertyAnimator s_currentAnimation;
 
 		Page _current;
+		bool _disposed;
 
 		public NavigationRenderer()
 		{
@@ -39,17 +40,24 @@ namespace Xamarin.Forms.Platform.Android
 
 		protected override void Dispose(bool disposing)
 		{
-			if (disposing)
+			if (disposing && !_disposed)
 			{
-				foreach (VisualElement child in PageController.InternalChildren)
-				{
-					IVisualElementRenderer renderer = Platform.GetRenderer(child);
-					if (renderer != null)
-						renderer.Dispose();
-				}
+				_disposed = true;
 
 				if (Element != null)
 				{
+					foreach (Element element in PageController.InternalChildren)
+					{
+						var child = (VisualElement)element;
+						if (child == null)
+						{
+							continue;
+						}
+
+						IVisualElementRenderer renderer = Platform.GetRenderer(child);
+						renderer?.Dispose();
+					}
+
 					var navController = (INavigationPageController)Element;
 
 					navController.PushRequested -= OnPushed;
@@ -101,7 +109,10 @@ namespace Xamarin.Forms.Platform.Android
 			newNavController.RemovePageRequested += OnRemovePageRequested;
 
 			// If there is already stuff on the stack we need to push it
-			newNavController.StackCopy.Reverse().ForEach(p => PushViewAsync(p, false));
+			foreach(Page page in newNavController.StackCopy.Reverse())
+			{
+				PushViewAsync(page, false);
+			}
 		}
 
 		protected override void OnLayout(bool changed, int l, int t, int r, int b)
diff --git a/Xamarin.Forms.sln.DotSettings b/Xamarin.Forms.sln.DotSettings
index 3fc7e736..597fd716 100644
--- a/Xamarin.Forms.sln.DotSettings
+++ b/Xamarin.Forms.sln.DotSettings
@@ -17,6 +17,8 @@
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/PLACE_CATCH_ON_NEW_LINE/@EntryValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/PLACE_FINALLY_ON_NEW_LINE/@EntryValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_AFTER_TYPECAST_PARENTHESES/@EntryValue">False</s:Boolean>
+	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_IF_PARENTHESES/@EntryValue">True</s:Boolean>
+	
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_LOCK_PARENTHESES/@EntryValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_METHOD_CALL_PARENTHESES/@EntryValue">False</s:Boolean>
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_EMPTY_METHOD_CALL_PARENTHESES/@EntryValue">False</s:Boolean>