Exit image code if native control is destroyed (#9034)

* Exit image code if native control is destroyed * - add uitest * - ui test * - remove invalidate override * - remove extra invalidate
xamarin · Jan 5, 2020 · 239ba50 · 239ba50
1 parent a46e974
commit 239ba50
Show file tree

Hide file tree

Showing 12 changed files with 132 additions and 49 deletions.
diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8262.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8262.cs
@@ -0,0 +1,104 @@
+using System;
+using System.Collections.Generic;
+using System.Collections.ObjectModel;
+using System.Text;
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+using System.Linq;
+
+
+#if UITEST
+using Xamarin.UITest;
+using NUnit.Framework;
+using Xamarin.Forms.Core.UITests;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Github, 8262, "[Android] ImageRenderer still being accessed after control destroyed",
+		PlatformAffected.Android)]
+#if UITEST
+	[NUnit.Framework.Category(UITestCategories.LifeCycle)]
+	[NUnit.Framework.Category(UITestCategories.CollectionView)]
+#endif
+	public class Issue8262 : TestContentPage
+	{
+		public View WithBounds(View v, double x, double y, double w, double h)
+		{
+			AbsoluteLayout.SetLayoutBounds(v, new Rectangle(x, y, w, h));
+			return v;
+		}
+
+		protected override void Init()
+		{
+			IEnumerable<View> Select((string groupHeader, IEnumerable<int> items) t)
+			{
+				yield return new AbsoluteLayout
+				{
+					Children = {
+						WithBounds(new Label {
+							Text = t.groupHeader, HorizontalTextAlignment = TextAlignment.Center,
+							TextColor = Color.FromUint(0xff5a5a5a), FontSize = 10
+						}, 0, 21.1, 310, AbsoluteLayout.AutoSize) },
+					HorizontalOptions = LayoutOptions.FillAndExpand,
+					VerticalOptions = LayoutOptions.Start,
+					HeightRequest = 46
+				};
+				foreach (var item in t.items)
+				{
+					yield return new AbsoluteLayout
+					{
+						Children = {
+
+							WithBounds(new Image { Source = ImageSource.FromResource("Xamarin.Forms.Controls.GalleryPages.crimson.jpg", System.Reflection.Assembly.GetCallingAssembly()) }, 23.6, 14.5, 14.9, 20.7),
+
+							WithBounds(new Label { Text = item.ToString(), TextColor = Color.FromUint(0xff5a5a5a), FontSize = 10 }, 58, 18.2, AbsoluteLayout.AutoSize, AbsoluteLayout.AutoSize)
+						},
+						HorizontalOptions = LayoutOptions.FillAndExpand,
+						VerticalOptions = LayoutOptions.Start,
+						HeightRequest = 49.7
+					};
+				}
+			}
+
+			Content = new AbsoluteLayout
+			{
+				Children = {
+					new CollectionView {
+						ItemsSource =
+							new (string, Func<int, bool>)[] {
+								("odd", i => i % 2 == 1),
+								("even", i => i % 2 == 0),
+								("triple", i => i % 3 == 0),
+								("fives", i => i % 5 == 0) }
+							.Select(t => (t.Item1, Enumerable.Range(1, 100).Where(t.Item2)))
+							.SelectMany(Select),
+						ItemTemplate = new DataTemplate(() => {
+							var template = new ContentView();
+							template.SetBinding(ContentView.ContentProperty, ".");
+							return template;
+						}),
+						ItemsLayout = LinearItemsLayout.Vertical,
+						ItemSizingStrategy = ItemSizingStrategy.MeasureFirstItem,
+						AutomationId = "ScrollMe"
+					}
+				}
+			};
+		}
+
+#if UITEST
+		[Test]
+		public void ScrollingQuicklyOnCollectionViewDoesntCrashOnDestroyedImage()
+		{
+			RunningApp.WaitForElement("ScrollMe");
+			RunningApp.ScrollDown("ScrollMe", ScrollStrategy.Gesture, swipeSpeed: 20000);
+			RunningApp.ScrollUp("ScrollMe", ScrollStrategy.Gesture, swipeSpeed: 20000);
+			RunningApp.ScrollDown("ScrollMe", ScrollStrategy.Gesture, swipeSpeed: 20000);
+			RunningApp.ScrollUp("ScrollMe", ScrollStrategy.Gesture, swipeSpeed: 20000);
+			RunningApp.ScrollDown("ScrollMe", ScrollStrategy.Gesture, swipeSpeed: 20000);
+			RunningApp.WaitForElement("ScrollMe");
+		}
+#endif
+	}
+}
diff --git a/...ssues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/...ssues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
@@ -10,6 +10,7 @@
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="$(MSBuildThisFileDirectory)CollectionViewGroupTypeIssue.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Issue8262.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue8207.xaml.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue6362.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue7505.cs" />
@@ -1683,4 +1684,4 @@
       <Generator>MSBuild:UpdateDesignTimeXaml</Generator>
     </EmbeddedResource>
   </ItemGroup>
-</Project>
+</Project>
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/ButtonRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/ButtonRenderer.cs
@@ -220,6 +220,6 @@ void UpdateCharacterSpacing()
 		}
 
 		AppCompatButton IButtonLayoutRenderer.View => Control;
-		bool IDisposedState.IsDisposed => _isDisposed;
+		bool IDisposedState.IsDisposed => _isDisposed || !Control.IsAlive();
 	}
 }
diff --git a/Xamarin.Forms.Platform.Android/AppCompat/CheckBoxDesignerRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/CheckBoxDesignerRenderer.cs
@@ -27,7 +27,6 @@ sealed class CheckBoxDesignerRenderer :
 		CompoundButton.IOnCheckedChangeListener
 	{
 		bool _disposed;
-		bool _skipInvalidate;
 		int? _defaultLabelFor;
 		VisualElementTracker _tracker;
 		VisualElementRenderer _visualElementRenderer;
@@ -87,17 +86,6 @@ protected override void Dispose(bool disposing)
 			base.Dispose(disposing);
 		}
 
-		public override void Invalidate()
-		{
-			if (_skipInvalidate)
-			{
-				_skipInvalidate = false;
-				return;
-			}
-
-			base.Invalidate();
-		}
-
 		Size MinimumSize()
 		{
 			return new Size();

diff --git a/Xamarin.Forms.Platform.Android/AppCompat/CheckBoxRendererBase.cs b/Xamarin.Forms.Platform.Android/AppCompat/CheckBoxRendererBase.cs
@@ -23,7 +23,6 @@ public class CheckBoxRendererBase :
 		ITabStop
 	{
 		bool _disposed;
-		bool _skipInvalidate;
 		int? _defaultLabelFor;
 		VisualElementTracker _tracker;
 		VisualElementRenderer _visualElementRenderer;
@@ -85,17 +84,6 @@ protected override void Dispose(bool disposing)
 			base.Dispose(disposing);
 		}
 
-		public override void Invalidate()
-		{
-			if (_skipInvalidate)
-			{
-				_skipInvalidate = false;
-				return;
-			}
-
-			base.Invalidate();
-		}
-
 		Size MinimumSize()
 		{
 			return Size.Zero;

diff --git a/Xamarin.Forms.Platform.Android/AppCompat/ImageButtonRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/ImageButtonRenderer.cs
@@ -43,7 +43,7 @@ public class ImageButtonRenderer :
 		AView IVisualElementRenderer.View => this;
 		ViewGroup IVisualElementRenderer.ViewGroup => null;
 		VisualElementTracker IVisualElementRenderer.Tracker => _tracker;
-		bool IDisposedState.IsDisposed => _disposed;
+		bool IDisposedState.IsDisposed => ((IImageRendererController)this).IsDisposed;
 
 		public ImageButton Element
 		{
@@ -56,7 +56,7 @@ private set
 		}
 
 		void IImageRendererController.SkipInvalidate() => _skipInvalidate = true;
-		bool IImageRendererController.IsDisposed => _disposed;
+		bool IImageRendererController.IsDisposed => _disposed || !Control.IsAlive();
 
 		AppCompatImageButton Control => this;
 		public ImageButtonRenderer(Context context) : base(context)

diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/ImageElementManager.cs b/Xamarin.Forms.Platform.Android/FastRenderers/ImageElementManager.cs
@@ -51,13 +51,20 @@ async static void OnElementChanged(object sender, VisualElementChangedEventArgs
             var oldImageElementManager = e.OldElement as IImageElement;
             var rendererController = renderer as IImageRendererController;
 
-            await TryUpdateBitmap(rendererController, view, newImageElementManager, oldImageElementManager);
-            UpdateAspect(rendererController, view, newImageElementManager, oldImageElementManager);
+			if (rendererController.IsDisposed)
+				return;
 
-            if (!rendererController.IsDisposed)
-            {
-                ElevationHelper.SetElevation(view, renderer.Element);
-            }
+			await TryUpdateBitmap(rendererController, view, newImageElementManager, oldImageElementManager);
+
+			if (rendererController.IsDisposed)
+				return;
+
+			UpdateAspect(rendererController, view, newImageElementManager, oldImageElementManager);
+
+			if (rendererController.IsDisposed)
+				return;
+
+            ElevationHelper.SetElevation(view, renderer.Element);
         }
 
         async static void OnElementPropertyChanged(object sender, PropertyChangedEventArgs e)
@@ -136,6 +143,9 @@ async static Task TryUpdateBitmap(IImageRendererController rendererController, I
                     imageController.SetIsLoading(false);
             }
 
+			if (rendererController.IsDisposed)
+				return;
+
 			if (Control.Drawable is FormsAnimationDrawable updatedAnimation)
 			{
 				rendererController.SetFormsAnimationDrawable(updatedAnimation);

diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs b/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs
@@ -23,7 +23,7 @@ public class ImageRenderer : AImageView, IVisualElementRenderer, IImageRendererC
 		readonly MotionEventHelper _motionEventHelper = new MotionEventHelper();
 		IFormsAnimationDrawable _formsAnimationDrawable;
 
-		bool IImageRendererController.IsDisposed => _disposed;
+		bool IImageRendererController.IsDisposed => _disposed || !Control.IsAlive();
 		protected override void Dispose(bool disposing)
 		{
 			if (_disposed)

diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs b/Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs
@@ -249,7 +249,6 @@ protected virtual void OnElementChanged(ElementChangedEventArgs<Label> e)
 
 				e.NewElement.PropertyChanged += OnElementPropertyChanged;
 
-				SkipNextInvalidate();
 				UpdateText();
 				UpdateLineHeight();
 				UpdateCharacterSpacing();

diff --git a/Xamarin.Forms.Platform.Android/Renderers/FormsImageView.cs b/Xamarin.Forms.Platform.Android/Renderers/FormsImageView.cs
@@ -39,6 +39,6 @@ void IImageRendererController.SetFormsAnimationDrawable(IFormsAnimationDrawable
 		}
 
 
-		bool IImageRendererController.IsDisposed => false;
+		bool IImageRendererController.IsDisposed => false || !this.IsAlive();
 	}
 }
diff --git a/Xamarin.Forms.Platform.Android/Renderers/FormsTextView.cs b/Xamarin.Forms.Platform.Android/Renderers/FormsTextView.cs
@@ -8,34 +8,28 @@ namespace Xamarin.Forms.Platform.Android
 {
 	public class FormsTextView : TextView
 	{
-		bool _skip;
-
 		public FormsTextView(Context context) : base(context)
 		{
 		}
 
+		[Obsolete]
 		public FormsTextView(Context context, IAttributeSet attrs) : base(context, attrs)
 		{
 		}
 
+		[Obsolete]
 		public FormsTextView(Context context, IAttributeSet attrs, int defStyle) : base(context, attrs, defStyle)
 		{
 		}
 
+		[Obsolete]
 		protected FormsTextView(IntPtr javaReference, JniHandleOwnership transfer) : base(javaReference, transfer)
 		{
 		}
 
-		public override void Invalidate()
-		{
-			if (!_skip)
-				base.Invalidate();
-			_skip = false;
-		}
-
+		[Obsolete]
 		public void SkipNextInvalidate()
 		{
-			_skip = true;
 		}
 	}
 }
diff --git a/Xamarin.Forms.Platform.Android/Renderers/LabelRenderer.cs b/Xamarin.Forms.Platform.Android/Renderers/LabelRenderer.cs
@@ -124,8 +124,7 @@ protected override void OnElementChanged(ElementChangedEventArgs<Label> e)
 				UpdateMaxLines();
 			}
 			else
-			{
-				_view.SkipNextInvalidate();
+			{	
 				UpdateText();
 				if (e.OldElement.LineBreakMode != e.NewElement.LineBreakMode)
 					UpdateLineBreakMode();