Set the Id field for Android Views created by Forms (#1004)

* Repro of modal-over-map-crash issue

* Automated test for maps modal crash

* Generate Ids for all Renderer Views on Android

* Add Ids for PageContainer and PageRenderer

* Remove TODO comment

* Verify fast renderers aren't disposed before querying Id

19 files changed, 153 insertions, 29 deletions

diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/MapsModalCrash.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/MapsModalCrash.cs
new file mode 100644
index 00000000..fcae4844
--- /dev/null
+++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/MapsModalCrash.cs
@@ -0,0 +1,86 @@
+using System;
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+using Xamarin.Forms.Maps;
+#if UITEST
+using Xamarin.Forms.Core.UITests;
+using Xamarin.UITest;
+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+
+#if UITEST
+	[Category(UITestCategories.Maps)]
+	[Category(UITestCategories.ManualReview)]
+#endif
+
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.None, 1701, "Modal Page over Map crashes application", PlatformAffected.Android)]
+	public class MapsModalCrash : TestContentPage
+	{
+		const string StartTest = "Start Test";
+		const string DisplayModal = "Click Me";
+		const string Success = "SuccessLabel";
+
+		protected override void Init()
+		{
+			var button = new Button { Text = StartTest };
+			button.Clicked += (sender, args) =>
+			{
+				Application.Current.MainPage = MapPage();
+			};
+
+			Content = new StackLayout
+			{
+				Padding = new Thickness(0, 20, 0, 0),
+				Children =
+				{
+					button
+				}
+			};
+		}
+
+		static ContentPage MapPage()
+		{
+			var map = new Map();
+
+			var button = new Button { Text = DisplayModal };
+			button.Clicked += (sender, args) => button.Navigation.PushModalAsync(new NavigationPage(SuccessPage()));
+
+			return new ContentPage
+			{
+				Content = new StackLayout
+				{
+					Children =
+					{
+						map,
+						button
+					}
+				}
+			};
+		}
+
+		static ContentPage SuccessPage()
+		{
+			return new ContentPage
+			{
+				BackgroundColor = Color.LightBlue,
+				Content = new Label { Text = "If you're seeing this, then the test was a success.", AutomationId = Success }
+			};
+		}
+
+#if UITEST
+		[Test]
+		public void CanDisplayModalOverMap()
+		{
+			RunningApp.WaitForElement(StartTest);
+			RunningApp.Tap(StartTest);
+			RunningApp.WaitForElement(DisplayModal);
+			RunningApp.Tap(DisplayModal);
+			RunningApp.WaitForElement(Success);
+		}
+#endif
+	}
+}
\ 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 d77ed10d..a69dd79d 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
@@ -251,6 +251,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla32462.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla36681.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla36479.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)MapsModalCrash.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)ModalActivityIndicatorTest.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla37625.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla38658.cs" />
diff --git a/Xamarin.Forms.Controls/TestCases.cs b/Xamarin.Forms.Controls/TestCases.cs
index d559478f..cd49b08e 100644
--- a/Xamarin.Forms.Controls/TestCases.cs
+++ b/Xamarin.Forms.Controls/TestCases.cs
@@ -141,7 +141,7 @@ namespace Xamarin.Forms.Controls
 				var untrackedIssueCells = 
 					from issueModel in issueModels
 					where issueModel.IssueTracker == IssueTracker.None
-					orderby issueModel.Description 
+					orderby issueModel.IssueNumber descending, issueModel.Description 
 					select MakeIssueCell (issueModel.Name, issueModel.Description, issueModel.Action);
 
 				var issueCells = bugzillaIssueCells.Concat (githubIssueCells).Concat (untrackedIssueCells);
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs
index 766ee225..f50815d2 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs
@@ -94,7 +94,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 						LayoutParameters = new ViewGroup.LayoutParams(ViewGroup.LayoutParams.MatchParent, ViewGroup.LayoutParams.MatchParent),
 						Adapter = new FormsFragmentPagerAdapter<ContentPage>(e.NewElement, activity.SupportFragmentManager) { CountOverride = e.NewElement.Children.Count }
 					};
-				pager.Id = FormsAppCompatActivity.GetUniqueId();
+				pager.Id = Platform.GenerateViewId();
 				pager.AddOnPageChangeListener(this);
 
 				ViewGroup.AddView(pager);
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/FormsAppCompatActivity.cs b/Xamarin.Forms.Platform.Android/AppCompat/FormsAppCompatActivity.cs
index 91efaa1d..b3b59433 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/FormsAppCompatActivity.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/FormsAppCompatActivity.cs
@@ -461,23 +461,6 @@ namespace Xamarin.Forms.Platform.Android
 
 		public static int ToolbarResource { get; set; }
 
-		internal static int GetUniqueId()
-		{
-			// getting unique Id's is an art, and I consider myself the Jackson Pollock of the field
-			if ((int)Build.VERSION.SdkInt >= 17)
-				return global::Android.Views.View.GenerateViewId();
-
-			// Numbers higher than this range reserved for xml
-			// If we roll over, it can be exceptionally problematic for the user if they are still retaining things, android's internal implementation is
-			// basically identical to this except they do a lot of locking we don't have to because we know we only do this
-			// from the UI thread
-			if (s_id >= 0x00ffffff)
-				s_id = 0x00000400;
-			return s_id++;
-		}
-
-		static int s_id = 0x00000400;
-
 		#endregion
 	}
 }
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs
index 50840926..faaf7462 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs
@@ -19,7 +19,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 		public MasterDetailContainer(MasterDetailPage parent, bool isMaster, Context context) : base(parent, isMaster, context)
 		{
-			Id = FormsAppCompatActivity.GetUniqueId();
+			Id = Platform.GenerateViewId();
 			_parent = parent;
 			_isMaster = isMaster;
 		}
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs
index 8d17db84..eb2da7a0 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs
@@ -50,7 +50,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 		public NavigationPageRenderer()
 		{
 			AutoPackage = false;
-			Id = FormsAppCompatActivity.GetUniqueId();
+			Id = Platform.GenerateViewId();
 			Device.Info.PropertyChanged += DeviceInfoPropertyChanged;
 		}
 
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/Platform.cs b/Xamarin.Forms.Platform.Android/AppCompat/Platform.cs
index 03b23123..1ab45dfd 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/Platform.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/Platform.cs
@@ -347,6 +347,8 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 				Android.Platform.SetRenderer(modal, _renderer);
 
 				AddView(_renderer.View);
+
+				Id = Platform.GenerateViewId();
 			}
 
 			protected override void Dispose(bool disposing)
@@ -387,6 +389,11 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			}
 		}
 
+		internal static int GenerateViewId()
+		{
+			return Android.Platform.GenerateViewId();
+		}
+
 		#region Statics
 
 		public static implicit operator ViewGroup(Platform canvas)
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs
index bb14100d..78fc19cb 100644
--- a/Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs
@@ -166,7 +166,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 							LayoutParameters = new LayoutParams(LayoutParams.MatchParent, LayoutParams.MatchParent),
 							Adapter = new FormsFragmentPagerAdapter<Page>(e.NewElement, FragmentManager) { CountOverride = e.NewElement.Children.Count }
 						};
-					pager.Id = FormsAppCompatActivity.GetUniqueId();
+					pager.Id = Platform.GenerateViewId();
 					pager.AddOnPageChangeListener(this);
 
 					AddView(pager);
diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/AutomationPropertiesProvider.cs b/Xamarin.Forms.Platform.Android/FastRenderers/AutomationPropertiesProvider.cs
index 6cd158ff..e4118ac4 100644
--- a/Xamarin.Forms.Platform.Android/FastRenderers/AutomationPropertiesProvider.cs
+++ b/Xamarin.Forms.Platform.Android/FastRenderers/AutomationPropertiesProvider.cs
@@ -162,8 +162,8 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 			if (elemValue != null)
 			{
 				var id = Control.Id;
-				if (id == -1)
-					id = Control.Id = FormsAppCompatActivity.GetUniqueId();
+				if (id == global::Android.Views.View.NoId)
+					id = Control.Id = Platform.GenerateViewId();
 
 				var renderer = elemValue?.GetRenderer();
 				renderer?.SetLabelFor(id);
diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/ButtonRenderer.cs b/Xamarin.Forms.Platform.Android/FastRenderers/ButtonRenderer.cs
index 0bda9830..11f669e9 100644
--- a/Xamarin.Forms.Platform.Android/FastRenderers/ButtonRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/FastRenderers/ButtonRenderer.cs
@@ -37,7 +37,7 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 			_effectControlProvider = new EffectControlProvider(this);
             _textColorSwitcher = new Lazy<TextColorSwitcher>(() => new TextColorSwitcher(TextColors));
 
-            Initialize();
+			Initialize();
 		}
 
 		public VisualElement Element => Button;
@@ -216,6 +216,8 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 			}
 			if (e.NewElement != null && !_isDisposed)
 			{
+				this.EnsureId();
+
 				UpdateFont();
 				UpdateText();
 				UpdateBitmap();
diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/FrameRenderer.cs b/Xamarin.Forms.Platform.Android/FastRenderers/FrameRenderer.cs
index d3583cce..82637a3b 100644
--- a/Xamarin.Forms.Platform.Android/FastRenderers/FrameRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/FastRenderers/FrameRenderer.cs
@@ -145,6 +145,8 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 
 			if (e.NewElement != null)
 			{
+				this.EnsureId();
+
 				if (_visualElementTracker == null)
 				{
 					_visualElementTracker = new VisualElementTracker(this);
diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs b/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs
index 5ca1bd20..a86bbd25 100644
--- a/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs
@@ -62,11 +62,12 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 		{
 			await TryUpdateBitmap(e.OldElement);
 			UpdateAspect();
+			this.EnsureId();
 
 			ElementChanged?.Invoke(this, new VisualElementChangedEventArgs(e.OldElement, e.NewElement));
 		}
 
-        public override bool OnTouchEvent(MotionEvent e)
+		public override bool OnTouchEvent(MotionEvent e)
         {
             bool handled;
             var result = _visualElementRenderer.OnTouchEvent(e, Parent, out handled);
@@ -74,7 +75,7 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
             return handled ? result : base.OnTouchEvent(e);
         }
 
-        protected virtual Size MinimumSize()
+		protected virtual Size MinimumSize()
 		{
 			return new Size();
 		}
diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs b/Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs
index 54c7bb01..18a313e7 100644
--- a/Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs
@@ -177,6 +177,8 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 
 			if (e.NewElement != null)
 			{
+				this.EnsureId();
+
 				if (_visualElementTracker == null)
 				{
 					_visualElementTracker = new VisualElementTracker(this);
diff --git a/Xamarin.Forms.Platform.Android/Platform.cs b/Xamarin.Forms.Platform.Android/Platform.cs
index 467885ec..dbcd6374 100644
--- a/Xamarin.Forms.Platform.Android/Platform.cs
+++ b/Xamarin.Forms.Platform.Android/Platform.cs
@@ -1035,6 +1035,23 @@ namespace Xamarin.Forms.Platform.Android
 			}
 		}
 
+		internal static int GenerateViewId()
+		{
+			// getting unique Id's is an art, and I consider myself the Jackson Pollock of the field
+			if ((int)Build.VERSION.SdkInt >= 17)
+				return global::Android.Views.View.GenerateViewId();
+
+			// Numbers higher than this range reserved for xml
+			// If we roll over, it can be exceptionally problematic for the user if they are still retaining things, android's internal implementation is
+			// basically identical to this except they do a lot of locking we don't have to because we know we only do this
+			// from the UI thread
+			if (s_id >= 0x00ffffff)
+				s_id = 0x00000400;
+			return s_id++;
+		}
+
+		static int s_id = 0x00000400;
+
 		internal class DefaultRenderer : VisualElementRenderer<View>
 		{
 			bool _notReallyHandled;
diff --git a/Xamarin.Forms.Platform.Android/Renderers/PageContainer.cs b/Xamarin.Forms.Platform.Android/Renderers/PageContainer.cs
index a4e77361..435ee7cf 100644
--- a/Xamarin.Forms.Platform.Android/Renderers/PageContainer.cs
+++ b/Xamarin.Forms.Platform.Android/Renderers/PageContainer.cs
@@ -10,6 +10,7 @@ namespace Xamarin.Forms.Platform.Android
 			AddView(child.View);
 			Child = child;
 			IsInFragment = inFragment;
+			Id = Platform.GenerateViewId();
 		}
 
 		public IVisualElementRenderer Child { get; set; }
diff --git a/Xamarin.Forms.Platform.Android/Renderers/PageRenderer.cs b/Xamarin.Forms.Platform.Android/Renderers/PageRenderer.cs
index d1a20551..4f95e4e5 100644
--- a/Xamarin.Forms.Platform.Android/Renderers/PageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/Renderers/PageRenderer.cs
@@ -45,6 +45,11 @@ namespace Xamarin.Forms.Platform.Android
 			Page view = e.NewElement;
 			base.OnElementChanged(e);
 
+			if (Id == NoId)
+			{
+				Id = Platform.GenerateViewId();
+			}
+
 			UpdateBackgroundColor(view);
 			UpdateBackgroundImage(view);
 
diff --git a/Xamarin.Forms.Platform.Android/ViewExtensions.cs b/Xamarin.Forms.Platform.Android/ViewExtensions.cs
index 6afcf5c2..89146705 100644
--- a/Xamarin.Forms.Platform.Android/ViewExtensions.cs
+++ b/Xamarin.Forms.Platform.Android/ViewExtensions.cs
@@ -64,5 +64,18 @@ namespace Xamarin.Forms.Platform.Android
 				}
 			}
 		}
+
+		public static void EnsureId(this AView view)
+		{
+			if (view.IsDisposed())
+			{
+				return;
+			}
+
+			if (view.Id == AView.NoId)
+			{
+				view.Id = Platform.GenerateViewId();
+			}
+		}
 	}
 }
\ No newline at end of file
diff --git a/Xamarin.Forms.Platform.Android/ViewRenderer.cs b/Xamarin.Forms.Platform.Android/ViewRenderer.cs
index 1cf77594..dbd930b8 100644
--- a/Xamarin.Forms.Platform.Android/ViewRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/ViewRenderer.cs
@@ -290,6 +290,10 @@ namespace Xamarin.Forms.Platform.Android
 			_container = container;
 
 			Control = control;
+			if (Control.Id == NoId)
+			{
+				Control.Id = Platform.GenerateViewId();
+			}
 
 			AView toAdd = container == this ? control : (AView)container;
 			AddView(toAdd, LayoutParams.MatchParent);
@@ -310,8 +314,8 @@ namespace Xamarin.Forms.Platform.Android
 			if (elemValue != null)
 			{
 				var id = Control.Id;
-				if (id == -1)
-					id = Control.Id = FormsAppCompatActivity.GetUniqueId();
+				if (id == NoId)
+					id = Control.Id = Platform.GenerateViewId();
 
 				var renderer = elemValue?.GetRenderer();
 				renderer?.SetLabelFor(id);